Re: linux-next NFSD: NULL pointer dereference at nfsd_svc()

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Aug 06, 2010 at 06:05:37PM -0400, J. Bruce Fields wrote:
> On Fri, Aug 06, 2010 at 05:27:28PM -0400, J. Bruce Fields wrote:
> > Bah, so what you were hitting was simple--I just moved the
> > nfsd_reset_versions() call to the wrong place; the below should fix it.
> > 
> > There's also a couple other bugs in the area.

And also there was one more problem with my original "nfsd: fix
startup/shutdown order bug": it doesn't work to use sv_nrthreads
changing from zero to nonzero as the signal for when to do all this
startup, because write_pool_threads() adjusts the number of threads
without calling nfsd_svc().  (Maybe that should be fixed.)

For now, just use the nfsd_up variable to keep track of this (which is a
little closer to Jeff's original solution).

This is a replacement.

--b.

commit 4cd7eb015e92f7cefb43eaab3e111d1b3c7b3cbf
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Mon Aug 2 14:12:44 2010 -0400

    nfsd: fix startup/shutdown order bug
    
    We must create the server before we can call init_socks or check the
    number of threads.
    
    Symptoms were a NULL pointer dereference in nfsd_svc().  Problem
    identified by Jeff Layton.
    
    Also fix a minor cleanup-on-error case in nfsd_startup().
    
    Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 92173bd..2a20f89 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -204,6 +204,9 @@ static bool nfsd_up = false;
 static int nfsd_startup(unsigned short port, int nrservs)
 {
 	int ret;
+
+	if (nfsd_up)
+		return 0;
 	/*
 	 * Readahead param cache - will no-op if it already exists.
 	 * (Note therefore results will be suboptimal if number of
@@ -217,7 +220,7 @@ static int nfsd_startup(unsigned short port, int nrservs)
 		goto out_racache;
 	ret = lockd_up();
 	if (ret)
-		return ret;
+		goto out_racache;
 	ret = nfs4_state_start();
 	if (ret)
 		goto out_lockd;
@@ -420,7 +423,7 @@ int
 nfsd_svc(unsigned short port, int nrservs)
 {
 	int	error;
-	bool	first_thread;
+	bool	nfsd_up_before;
 
 	mutex_lock(&nfsd_mutex);
 	dprintk("nfsd: creating service\n");
@@ -432,29 +435,29 @@ nfsd_svc(unsigned short port, int nrservs)
 	if (nrservs == 0 && nfsd_serv == NULL)
 		goto out;
 
-	first_thread = (nfsd_serv->sv_nrthreads == 0) && (nrservs != 0);
-
-	if (first_thread) {
-		error = nfsd_startup(port, nrservs);
-		if (error)
-			goto out;
-	}
 	error = nfsd_create_serv();
 	if (error)
-		goto out_shutdown;
-	error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
+		goto out;
+
+	nfsd_up_before = nfsd_up;
+
+	error = nfsd_startup(port, nrservs);
 	if (error)
 		goto out_destroy;
+	}
+	error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
+	if (error)
+		goto out_shutdown;
 	/* We are holding a reference to nfsd_serv which
 	 * we don't want to count in the return value,
 	 * so subtract 1
 	 */
 	error = nfsd_serv->sv_nrthreads - 1;
-out_destroy:
-	svc_destroy(nfsd_serv);		/* Release server */
 out_shutdown:
-	if (error < 0 && first_thread)
+	if (error < 0 && !nfsd_up_before)
 		nfsd_shutdown();
+out_destroy:
+	svc_destroy(nfsd_serv);		/* Release server */
 out:
 	mutex_unlock(&nfsd_mutex);
 	return error;