[linux-cifs-client] [PATCH] CIFS: make cifsd (more) signal-safe

[Jeff Layton]

I recently sent a similar, smaller patch for this problem. After some
discussion with Steve and Shaggy, I think I better understand why cifsd
allows signals through, and I realize that my earlier patch wasn't
comprehensive enough

The mount and unmount calls will send a KILL signal to cifsd to wake it
up if it happens to be blocked in kernel_recvmsg. The problem is that
it doesn't distinguish between "legitimate" signals sent for this
reason and spurious signals sent by a userspace process (for instance).
While this is definitely a "don't do that" sort of situation, we might
as well try to have cifsd be as signal-safe as possible.

The following patch does this by making sure that we set tcpStatus to
CifsExiting before sending cifsd a signal, and having cifsd check for
that when it sees that it's been signalled. If the tcpStatus is not set
correctly, it ignores it, flushes signals and moves on.

I've tested a similar backported version of this on an earlier kernel,
but have not tested this particular patch as of yet.

Signed-off-by: Jeff Layton <jlayton at redhat.com>

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f4e9266..461fbaa 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -197,6 +197,11 @@ cifs_reconnect(struct TCP_Server_Info *server)
 					server->server_RFC1001_name);
 		}
 		if(rc) {
+			if (rc == -ERESTARTSYS) {
+				cFYI(1,("reconnect interrupted by signal"));
+				flush_signals(server->tsk);
+				continue;
+			}
 			cFYI(1,("reconnect error %d",rc));
 			msleep(3000);
 		} else {
@@ -425,8 +430,14 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 				break;
 			}
 			if (!try_to_freeze() && (length == -EINTR)) {
-				cFYI(1,("cifsd thread killed"));
-				break;
+				if (server->tcpStatus == CifsExiting) {
+					cFYI(1,("cifsd thread killed"));
+					break;
+				} else {
+					cFYI(1,("cifsd caught spurious signal, ignoring it"));
+					flush_signals(server->tsk);
+					continue;
+				}
 			}
 			cFYI(1,("Reconnect after unexpected peek error %d",
 				length));
@@ -526,11 +537,15 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 		     total_read += length) {
 			length = kernel_recvmsg(csocket, &smb_msg, &iov, 1,
 						pdu_length - total_read, 0);
-			if( kthread_should_stop() ||
-			    (length == -EINTR)) {
-				/* then will exit */
+			if (kthread_should_stop() ||
+			    (length == -EINTR &&
+			     server->tcpStatus == CifsExiting)) {
 				reconnect = 2;
 				break;
+			} else if (length == -EINTR) {
+				cFYI(1, ("cifsd caught spurious signal, ignoring it"));
+				flush_signals(server->tsk);
+				continue;
 			} else if (server->tcpStatus == CifsNeedReconnect) {
 				cifs_reconnect(server);
 				csocket = server->ssocket;
@@ -1692,6 +1707,26 @@ void reset_cifs_unix_caps(int xid, struct cifsTconInfo * tcon,
 	}
 }
 
+static inline void stop_cifsd(struct TCP_Server_Info *srv)
+{
+	struct task_struct *tsk = srv->tsk;
+
+	cFYI(1,("shutting down cifsd thread"));
+
+	spin_lock(&GlobalMid_Lock);
+	srv->tcpStatus = CifsExiting;
+	spin_unlock(&GlobalMid_Lock);
+
+	/* If we could verify that kthread_stop would
+	 * always wake up processes blocked in
+	 * tcp in recv_mesg then we could remove the
+	 * send_sig call */
+	send_sig(SIGKILL, tsk, 1);
+
+	if (tsk)
+		kthread_stop(tsk);
+}
+
 int
 cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 	   char *mount_data, const char *devname)
@@ -2064,22 +2099,9 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 	if (rc) {
 		/* if session setup failed, use count is zero but
 		we still need to free cifsd thread */
-		if (atomic_read(&srvTcp->socketUseCount) == 0) {
-			spin_lock(&GlobalMid_Lock);
-			srvTcp->tcpStatus = CifsExiting;
-			spin_unlock(&GlobalMid_Lock);
-			if (srvTcp->tsk) {
-				struct task_struct *tsk;
-				/* If we could verify that kthread_stop would
-				   always wake up processes blocked in
-				   tcp in recv_mesg then we could remove the
-				   send_sig call */
-				send_sig(SIGKILL,srvTcp->tsk,1);
-				tsk = srvTcp->tsk;
-				if(tsk)
-					kthread_stop(tsk);
-			}
-		}
+		if (atomic_read(&srvTcp->socketUseCount) == 0)
+			stop_cifsd(srvTcp);
+
 		 /* If find_unc succeeded then rc == 0 so we can not end */
 		if (tcon)  /* up accidently freeing someone elses tcon struct */
 			tconInfoFree(tcon);
@@ -2091,13 +2113,8 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 					temp_rc = CIFSSMBLogoff(xid, pSesInfo);
 					/* if the socketUseCount is now zero */
 					if ((temp_rc == -ESHUTDOWN) &&
-					   (pSesInfo->server) && (pSesInfo->server->tsk)) {
-						struct task_struct *tsk;
-						send_sig(SIGKILL,pSesInfo->server->tsk,1);
-						tsk = pSesInfo->server->tsk;
-						if (tsk)
-							kthread_stop(tsk);
-					}
+					   (pSesInfo->server) && (pSesInfo->server->tsk))
+						stop_cifsd(pSesInfo->server);
 				} else
 					cFYI(1, ("No session or bad tcon"));
 				sesInfoFree(pSesInfo);
@@ -3321,7 +3338,7 @@ cifs_umount(struct super_block *sb, struct cifs_sb_info *cifs_sb)
 	int rc = 0;
 	int xid;
 	struct cifsSesInfo *ses = NULL;
-	struct task_struct *cifsd_task;
+	struct TCP_Server_Info *server;
 	char * tmp;
 
 	xid = GetXid();
@@ -3335,19 +3352,15 @@ cifs_umount(struct super_block *sb, struct cifs_sb_info *cifs_sb)
 		}
 		tconInfoFree(cifs_sb->tcon);
 		if ((ses) && (ses->server)) {
-			/* save off task so we do not refer to ses later */
-			cifsd_task = ses->server->tsk;
+			/* save off server so we do not refer to ses later */
+			server = ses->server;
 			cFYI(1, ("About to do SMBLogoff "));
 			rc = CIFSSMBLogoff(xid, ses);
 			if (rc == -EBUSY) {
 				FreeXid(xid);
 				return 0;
 			} else if (rc == -ESHUTDOWN) {
-				cFYI(1,("Waking up socket by sending it signal"));
-				if (cifsd_task) {
-					send_sig(SIGKILL,cifsd_task,1);
-					kthread_stop(cifsd_task);
-				}
+				stop_cifsd(server);
 				rc = 0;
 			} /* else - we have an smb session
 				left on this socket do not kill cifsd */