linux-kernel - Re: [PATCH 40/41] ncpfs: Use set_current

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1313589483.2311.24.camel@mfleming-mobl1.ger.corp.intel.com>
Date:	Wed, 17 Aug 2011 14:58:03 +0100
From:	Matt Fleming <matt@...sole-pimps.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	linux-kernel@...r.kernel.org, Petr Vandrovec <petr@...drovec.name>,
	Al Viro <viro@...iv.linux.org.uk>,
	Arnd Bergmann <arnd@...db.de>, linux-fsdevel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

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@...el.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@...el.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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/