linux-kernel - Re: [PATCH 3/6] NLM: Initialize completion variable in lockd

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]

Date:	Sun, 13 Jan 2008 18:17:43 +0000
From:	Christoph Hellwig <hch@...radead.org>
To:	Jeff Layton <jlayton@...hat.com>
Cc:	Christoph Hellwig <hch@...radead.org>, akpm@...ux-foundation.org,
	neilb@...e.de, linux-nfs@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up

On Sun, Jan 13, 2008 at 08:27:18AM -0500, Jeff Layton wrote:
>    I've been hitting an intermittent null pointer dereference ever
> since I've made this change:

The first thing lockd does is to call lock_kernel().  This may either
block (or spin) when it is contended and thus delay updating
nlmsvc_serv.  Now lockd_up checks for nlmsvc_task which already is
non-NULL and happily dereference nlmsvc_serv.   The patch below
updates nlmsvc_serv in lockd_up where it is protected by nlmsvc_mutex
and also checks for nlmsvc_serv beeing set instead of nlmsvc_task to
fix this problem.

The patch hasn't actually been tested but I'm sure it will fix this
issue.

Btw, lockd() takes BKL just after starting up and only implicitly drops
it when blocking.  This seems very dangerous to me and badly wants
updating to some real locking scheme..


Signed-off-by: Christoph Hellwig <hch@....de>

Index: linux-2.6/fs/lockd/svc.c
===================================================================
--- linux-2.6.orig/fs/lockd/svc.c	2008-01-13 19:07:17.000000000 +0100
+++ linux-2.6/fs/lockd/svc.c	2008-01-13 19:13:23.000000000 +0100
@@ -118,7 +118,6 @@ lockd(void *vrqstp)
 
 	/* set up kernel thread */
 	lock_kernel();
-	nlmsvc_serv = rqstp->rq_server;
 	set_freezable();
 
 	/* Allow SIGKILL to tell lockd to drop all of its locks */
@@ -253,7 +252,7 @@ lockd_up(int proto) /* Maybe add a 'fami
 	/*
 	 * Check whether we're already up and running.
 	 */
-	if (nlmsvc_task) {
+	if (nlmsvc_serv) {
 		if (proto)
 			error = make_socks(nlmsvc_serv, proto);
 		goto out;
@@ -290,6 +289,9 @@ lockd_up(int proto) /* Maybe add a 'fami
 	}
 
 	svc_sock_update_bufs(serv);
+
+	nlmsvc_serv = rqstp->rq_server;
+
 	nlmsvc_task = kthread_run(lockd, rqstp, serv->sv_name);
 	if (IS_ERR(nlmsvc_task)) {
 		error = PTR_ERR(nlmsvc_task);
--
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/