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

[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@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 */

Re: [PATCH] CIFS: make cifsd (more) signal-safe

[Christoph Hellwig]

On Tue, Jun 05, 2007 at 03:23:40PM -0400, Jeff Layton wrote:
> 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.

The right way to fix this is to stop sending signals at all and have
a kernel-internal way to get out of kernel_recvmsg.  Uses of signals by
kernel thread generally are bugs.

[PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled

[Jeff Layton]

This one's sort of outside my normal area of expertise so sending this
as an RFC to gather feedback on the idea.

Some background:

The cifs_mount() and cifs_umount() functions currently send a signal to
the cifsd kthread prior to calling kthread_stop on it. The reasoning is
apparently that it's likely that cifsd will have called kernel_recvmsg()
and if it doesn't do this there can be a rather long delay when a
filesystem is unmounted.

The following patch is a first stab at removing this need. It makes it
so that in tcp_recvmsg() we also check kthread_should_stop() at any
point where we currently check to see if the task was signalled. If
that returns true, then it acts as if it were signalled and returns to
the calling function.

I've tested this on a fairly recent kernel with a cifs module that
doesn't send signals on unmount and it seems to work as expected. I'm
just not clear on whether it will have any adverse side-effects.

Obviously if this approach is OK then we'll probably also want to fix
up other recvmsg functions (udp_recvmsg, etc).

Anyone care to comment?

Thanks,

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

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index bd4c295..1ad91fa 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -258,6 +258,7 @@
 #include <linux/cache.h>
 #include <linux/err.h>
 #include <linux/crypto.h>
+#include <linux/kthread.h>
 
 #include <net/icmp.h>
 #include <net/tcp.h>
@@ -1154,7 +1155,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 		if (tp->urg_data && tp->urg_seq == *seq) {
 			if (copied)
 				break;
-			if (signal_pending(current)) {
+			if (signal_pending(current) || kthread_should_stop()) {
 				copied = timeo ? sock_intr_errno(timeo) : -EAGAIN;
 				break;
 			}
@@ -1197,6 +1198,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 			    (sk->sk_shutdown & RCV_SHUTDOWN) ||
 			    !timeo ||
 			    signal_pending(current) ||
+			    kthread_should_stop() ||
 			    (flags & MSG_PEEK))
 				break;
 		} else {
@@ -1227,7 +1229,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 				break;
 			}
 
-			if (signal_pending(current)) {
+			if (signal_pending(current) || kthread_should_stop()) {
 				copied = sock_intr_errno(timeo);
 				break;
 			}

Re: [PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled

[Herbert Xu]

Please cc networking patches to netdev@vger.kernel.org.

Jeff Layton <jlayton@redhat.com> wrote:
> 
> The following patch is a first stab at removing this need. It makes it
> so that in tcp_recvmsg() we also check kthread_should_stop() at any
> point where we currently check to see if the task was signalled. If
> that returns true, then it acts as if it were signalled and returns to
> the calling function.

This just doesn't seem to fit.  Why should networking care about kthreads?

Perhaps you can get kthread_stop to send a signal instead?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled

[Jeff Layton]

On Sat, 09 Jun 2007 11:30:04 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Please cc networking patches to netdev@vger.kernel.org.
> 
> Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > The following patch is a first stab at removing this need. It makes it
> > so that in tcp_recvmsg() we also check kthread_should_stop() at any
> > point where we currently check to see if the task was signalled. If
> > that returns true, then it acts as if it were signalled and returns to
> > the calling function.
> 
> This just doesn't seem to fit.  Why should networking care about kthreads?
> 
> Perhaps you can get kthread_stop to send a signal instead?
> 

The problem there is that we still have to make the kthread let signals
through. The nice thing about this approach is that we can make the
kthread ignore signals, but still allow it to break out of kernel_recvmsg
when a kthread_stop is done.

Though I will confess that you have a point about this feeling like a
layering violation...

-- 
Jeff Layton <jlayton@redhat.com>

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

[Jeff Layton]

On Wed, 6 Jun 2007 09:55:50 +0100
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jun 05, 2007 at 03:23:40PM -0400, Jeff Layton wrote:
> > 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.
> 
> The right way to fix this is to stop sending signals at all and have
> a kernel-internal way to get out of kernel_recvmsg.  Uses of signals by
> kernel thread generally are bugs.
> 

I've looked at different ways to do this and haven't seen a clean way
to do this. I made an initial stab at having tcp_recvmsg check
kthread_stop and break out of the loop if it sees it. Herbert Xu stated
that he didn't think that was right -- after all, why should
tcp_recvmsg care about khthreads?

As I see it we're left with using signals, or setting up some other
signal-type infrastructure that's just available in the kernel. That
seems redundant to me, and I'm not clear as to why use of signals by
kthreads is a bad thing.

So, here's a second attempt at this. This changes cifsd to ignore all
signals and changes the cifs mount/umount code to use force_sig()
instead of send_sig(). I don't think this is any worse than what we're
doing today and it insulates cifsd from signals sent from userspace.

This should apply to the current cifs-2.6 git tree.

Seem reasonable?

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

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f4e9266..27c1ebe 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -348,7 +348,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
 	int isMultiRsp;
 	int reconnect;
 
-	allow_signal(SIGKILL);
 	current->flags |= PF_MEMALLOC;
 	server->tsk = current;	/* save process info to wake at shutdown */
 	cFYI(1, ("Demultiplex PID: %d", current->pid));
@@ -2074,7 +2073,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 				   always wake up processes blocked in
 				   tcp in recv_mesg then we could remove the
 				   send_sig call */
-				send_sig(SIGKILL,srvTcp->tsk,1);
+				force_sig(SIGKILL,srvTcp->tsk);
 				tsk = srvTcp->tsk;
 				if(tsk)
 					kthread_stop(tsk);
@@ -2093,7 +2092,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 					if ((temp_rc == -ESHUTDOWN) &&
 					   (pSesInfo->server) && (pSesInfo->server->tsk)) {
 						struct task_struct *tsk;
-						send_sig(SIGKILL,pSesInfo->server->tsk,1);
+						force_sig(SIGKILL,pSesInfo->server->tsk);
 						tsk = pSesInfo->server->tsk;
 						if (tsk)
 							kthread_stop(tsk);
@@ -3345,7 +3344,7 @@ cifs_umount(struct super_block *sb, struct cifs_sb_info *cifs_sb)
 			} else if (rc == -ESHUTDOWN) {
 				cFYI(1,("Waking up socket by sending it signal"));
 				if (cifsd_task) {
-					send_sig(SIGKILL,cifsd_task,1);
+					force_sig(SIGKILL,cifsd_task);
 					kthread_stop(cifsd_task);
 				}
 				rc = 0;