From c76c6582f010c3121137dca7c24a280c34f22b92 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Thu, 22 Feb 2018 10:29:25 -0800 Subject: [PATCH] scoutfs: release server conn under mutex I was rarely seeing null derefs during unmount. The per-mount listening scoutfs_server_func() was seeing null sock->ops as it called kernel_sock_shutdown() to shutdown the connected client sockets. sock_release() sets the ops to null. We're not supposed to use a socket after we call it. The per-connection scoutfs_server_recv_func() calls sock_release() as it tears down its connection. But it does this before it removes the connection from the listener's list. There's a brief window where the connection's socket has been released but is still visible on the list. If the listener tries to shutdown during this time it will crash. Hitting this window depends on scheduling races during unmount. The unmount path has the client close its connection to the server then the server closes all its connected clients. If the local mount is the server then it will have recv work see an error as the client disconnects and it will be racing to shut down the connection with the listening thread during unmount. I think I only saw this in my guests because they're running slower debug kernels on my slower laptop. The window of vulnerability while the released socket is on the list is longer. The fix is to release the socket while we hold the mutex and are removing the connection from the list. A released socket is never visible on the list. While we're at it don't use list_for_each_entry_safe() to iterate over the connection list. We're not modifying it. This is an lingering artifact from previous versions of the server code. Signed-off-by: Zach Brown --- kmod/src/server.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/kmod/src/server.c b/kmod/src/server.c index 91146e81..ff1ccc69 100644 --- a/kmod/src/server.c +++ b/kmod/src/server.c @@ -867,10 +867,9 @@ out: destroy_workqueue(req_wq); } - sock_release(conn->sock); - /* process_one_work explicitly allows freeing work in its func */ mutex_lock(&server->mutex); + sock_release(conn->sock); list_del_init(&conn->head); kfree(conn); smp_mb(); @@ -920,7 +919,6 @@ static void scoutfs_server_func(struct work_struct *work) struct socket *sock = NULL; struct scoutfs_lock *lock = NULL; struct server_connection *conn; - struct server_connection *conn_tmp; struct pending_seq *ps; struct pending_seq *ps_tmp; DECLARE_WAIT_QUEUE_HEAD(waitq); @@ -1043,7 +1041,7 @@ static void scoutfs_server_func(struct work_struct *work) /* shutdown send and recv on all accepted sockets */ mutex_lock(&server->mutex); - list_for_each_entry_safe(conn, conn_tmp, &conn_list, head) + list_for_each_entry(conn, &conn_list, head) kernel_sock_shutdown(conn->sock, SHUT_RDWR); mutex_unlock(&server->mutex);