Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

[Matt Fleming]

On Wed, 2011-08-17 at 14:04 +0200, Oleg Nesterov wrote:
> On 08/16, Matt Fleming wrote:
> >
> > > the sighand->action[] checks are racy anyway in the mt case, siglock
> > > can't help.
> >
> > Hmm.. really? I thought that ->siglock serialised modifications to
> > sighand->action[] even in the mt case, no?
> 
> Sure. But another thread can change sighand->action[] right after we
> drop ->siglock. So how can this lock help? We simply read the word,
> this is atomic and doesn't need the locking.

Oh right, in the scenario in ncp_do_request(), sure I understand that. I
thought you were saying that in the general case ->siglock doesn't
protect sighand->action[]! That's why I was confused ;-)

OK, how about this patch (instead of 40/41) which gets rid of all the
nasties? I've Cc'd linux-fsdevel so people can hopefully OK this from a
file system perspective.

Some Tested-by's would be good too.

--------8<--------

>From bb1650295054bdfa96f8f4ff61507d314be8296a Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming@intel.com>
Date: Wed, 17 Aug 2011 13:59:12 +0100
Subject: [PATCH] ncpfs: Don't attempt to mask signals during
 do_ncp_rpc_call()

Delete the code in ncp_do_request() that attempts to mask signals
across the call to do_ncp_rpc_call(). This code was racy because it
dropped ->siglock across do_ncp_rpc_call() so it was possible for
another thread to modify the signal handlers, which made the code
pointless.

Instead of fixing the code to hold the lock across the call let's just
delete it because, as the FIXME comment (which has been around since
the beginning of git history) says, trying to block signals doesn't
seem right at all.

Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 fs/ncpfs/sock.c |   32 +-------------------------------
 1 files changed, 1 insertions(+), 31 deletions(-)

diff --git a/fs/ncpfs/sock.c b/fs/ncpfs/sock.c
index 3a15872..6618402 100644
--- a/fs/ncpfs/sock.c
+++ b/fs/ncpfs/sock.c
@@ -748,38 +748,8 @@ static int ncp_do_request(struct ncp_server *server, int size,
 	if (!ncp_conn_valid(server)) {
 		return -EIO;
 	}
-	{
-		sigset_t old_set;
-		unsigned long mask, flags;
-
-		spin_lock_irqsave(&current->sighand->siglock, flags);
-		old_set = current->blocked;
-		if (current->flags & PF_EXITING)
-			mask = 0;
-		else
-			mask = sigmask(SIGKILL);
-		if (server->m.flags & NCP_MOUNT_INTR) {
-			/* FIXME: This doesn't seem right at all.  So, like,
-			   we can't handle SIGINT and get whatever to stop?
-			   What if we've blocked it ourselves?  What about
-			   alarms?  Why, in fact, are we mucking with the
-			   sigmask at all? -- r~ */
-			if (current->sighand->action[SIGINT - 1].sa.sa_handler == SIG_DFL)
-				mask |= sigmask(SIGINT);
-			if (current->sighand->action[SIGQUIT - 1].sa.sa_handler == SIG_DFL)
-				mask |= sigmask(SIGQUIT);
-		}
-		siginitsetinv(&current->blocked, mask);
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
-		
-		result = do_ncp_rpc_call(server, size, reply, max_reply_size);
 
-		spin_lock_irqsave(&current->sighand->siglock, flags);
-		current->blocked = old_set;
-		recalc_sigpending();
-		spin_unlock_irqrestore(&current->sighand->siglock, flags);
-	}
+	result = do_ncp_rpc_call(server, size, reply, max_reply_size);
 
 	DDPRINTK("do_ncp_rpc_call returned %d\n", result);
 
-- 
1.7.4.4

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

[Oleg Nesterov]

On 08/17, Matt Fleming wrote:
>
> On Wed, 2011-08-17 at 14:04 +0200, Oleg Nesterov wrote:
> > On 08/16, Matt Fleming wrote:
> > >
> > > > the sighand->action[] checks are racy anyway in the mt case, siglock
> > > > can't help.
> > >
> > > Hmm.. really? I thought that ->siglock serialised modifications to
> > > sighand->action[] even in the mt case, no?
> >
> > Sure. But another thread can change sighand->action[] right after we
> > drop ->siglock. So how can this lock help? We simply read the word,
> > this is atomic and doesn't need the locking.
>
> Oh right, in the scenario in ncp_do_request(), sure I understand that. I
> thought you were saying that in the general case ->siglock doesn't
> protect sighand->action[]! That's why I was confused ;-)
>
> OK, how about this patch (instead of 40/41) which gets rid of all the
> nasties? I've Cc'd linux-fsdevel so people can hopefully OK this from a
> file system perspective.

Well, of course I am in no position to ack this change ;)

But obviously I like the idea to kill the obviously wrong code.
In particular, the PF_EXITING/SIGKILL logic looks as "must die
in any case" to me.

If maintainers object, you can remove ->siglock and convert the code
to use set_current_blocked(). IOW, simplify your original patch.

Oleg.

> From bb1650295054bdfa96f8f4ff61507d314be8296a Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming@intel.com>
> Date: Wed, 17 Aug 2011 13:59:12 +0100
> Subject: [PATCH] ncpfs: Don't attempt to mask signals during
>  do_ncp_rpc_call()
> 
> Delete the code in ncp_do_request() that attempts to mask signals
> across the call to do_ncp_rpc_call(). This code was racy because it
> dropped ->siglock across do_ncp_rpc_call() so it was possible for
> another thread to modify the signal handlers, which made the code
> pointless.
> 
> Instead of fixing the code to hold the lock across the call let's just
> delete it because, as the FIXME comment (which has been around since
> the beginning of git history) says, trying to block signals doesn't
> seem right at all.
> 
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  fs/ncpfs/sock.c |   32 +-------------------------------
>  1 files changed, 1 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/ncpfs/sock.c b/fs/ncpfs/sock.c
> index 3a15872..6618402 100644
> --- a/fs/ncpfs/sock.c
> +++ b/fs/ncpfs/sock.c
> @@ -748,38 +748,8 @@ static int ncp_do_request(struct ncp_server *server, int size,
>  	if (!ncp_conn_valid(server)) {
>  		return -EIO;
>  	}
> -	{
> -		sigset_t old_set;
> -		unsigned long mask, flags;
> -
> -		spin_lock_irqsave(&current->sighand->siglock, flags);
> -		old_set = current->blocked;
> -		if (current->flags & PF_EXITING)
> -			mask = 0;
> -		else
> -			mask = sigmask(SIGKILL);
> -		if (server->m.flags & NCP_MOUNT_INTR) {
> -			/* FIXME: This doesn't seem right at all.  So, like,
> -			   we can't handle SIGINT and get whatever to stop?
> -			   What if we've blocked it ourselves?  What about
> -			   alarms?  Why, in fact, are we mucking with the
> -			   sigmask at all? -- r~ */
> -			if (current->sighand->action[SIGINT - 1].sa.sa_handler == SIG_DFL)
> -				mask |= sigmask(SIGINT);
> -			if (current->sighand->action[SIGQUIT - 1].sa.sa_handler == SIG_DFL)
> -				mask |= sigmask(SIGQUIT);
> -		}
> -		siginitsetinv(&current->blocked, mask);
> -		recalc_sigpending();
> -		spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -		
> -		result = do_ncp_rpc_call(server, size, reply, max_reply_size);
>  
> -		spin_lock_irqsave(&current->sighand->siglock, flags);
> -		current->blocked = old_set;
> -		recalc_sigpending();
> -		spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -	}
> +	result = do_ncp_rpc_call(server, size, reply, max_reply_size);
>  
>  	DDPRINTK("do_ncp_rpc_call returned %d\n", result);
>  
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

[Oleg Nesterov]

On 08/17, Petr Vandrovec wrote:
>
> I do not care about 'intr' support, but regular code path blocking all
> signals has to stay.

I do not care about SIGINT/SIGQUIT check, just I simply can't understand
the logic.

But PF_EXITING/SIGKILL code looks absolutely strange.

> PF_EXITING code is there because in the past something was BUG_ON-ing that
> nobody should touch signals while exiting,

Not sure I understand... do you mean we have a kernel bug? it should be
fixed then.

> and same code made sure you
> cannot send signals to exiting process.

Again, can't understand.

A PF_EXITING doesn't need to block the signals at all. See wants_siganl().

But, otoh, even sigblock(SIGKILL) can't protect from SIGKILL or another
fatal signal if this thread has a live subthread.

However, blocking SIGKILL makes the difference if it exits with the pending
SIGKILL. In this case recalc_sigpending() clears TIF_SIGPENDING, this "helps"
to actually sleep in TASK_INTERRUPTIBLE.

At the same time, ncp_do_request() can unblock, say, SIGINT. And this
means that if this task exits because it was killed by SIGINT it won't
block anyway.

And I can't understand why it if fine to kill the live task, but not fine
to interrupt the exiting task. If this was intended. And if you want to
block all signals, why can't you do wait_event_uninterruptible() instead?
Or interruptible/uninterruptible depending on PF_EXITING.


Now suppose it is not PF_EXITING. In this case it is pointless to block
any fatal signal unless this task is single-threaded.


So far I think this code does not understand what it does ;)


OK. I guess nobody cares. Matt, could you redo your original patch?
just remove ->siglock and convert it to use set_current_blocked()
leaving this logic alone.

Oleg.

Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

[Matt Fleming]

On Thu, 2011-08-18 at 19:05 +0200, Oleg Nesterov wrote:
> 
> OK. I guess nobody cares. Matt, could you redo your original patch?
> just remove ->siglock and convert it to use set_current_blocked()
> leaving this logic alone.

Sure, will do.

-- 
Matt Fleming, Intel Open Source Technology Center