NFSd: set nfsd_serv to NULL after service destruction

PATCH Superseded Stanislav Kinsbursky 2 participants 2 messages May 18, 2012 – May 23, 2012

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Stanislav Kinsbursky

author

2012-05-18 15:26 UTC #0 lore raw

PATCH 1/1 NFSd: set nfsd_serv to NULL after service destruction

Otherwise we will get NULL pointer dereference after svc_shutdown_net->sv_shutdown (nfsd_last_thread) call.
This looks safe, because all such operations (svc_destroy) are performed with nfsd_mutex being held.

Signed-off-by Stanislav Kinsbursky <[email protected]>

---
 fs/nfsd/nfsctl.c |    4 ++--
 fs/nfsd/nfsd.h   |    6 ++++++
 fs/nfsd/nfssvc.c |   10 ++++------
 3 files changed, 12 insertions(+), 8 deletions(-)

fs/nfsd/nfsctl.c +2 -2

index c55298e..82cca1e 100644
@@ -675,7 +675,7 @@ static ssize_t __write_ports_addfd(char *buf)
 	if (err < 0) {
 		if (nfsd_serv->sv_nrthreads == 1)
 			svc_shutdown_net(nfsd_serv, net);
-		svc_destroy(nfsd_serv);
+		nfsd_destroy();
 		return err;
 	}

@@ -746,7 +746,7 @@ out_close:
 out_err:
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);
+	nfsd_destroy();
 	return err;
 }

fs/nfsd/nfsd.h +6 -0

index 1671429..35db369 100644
@@ -73,6 +73,12 @@ int		nfsd_nrpools(void);
 int		nfsd_get_nrthreads(int n, int *);
 int		nfsd_set_nrthreads(int n, int *);
 
+static inline void nfsd_destroy(void)
+{
+	svc_destroy(nfsd_serv);
+	nfsd_serv = NULL;
+}
+
 #if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
 #ifdef CONFIG_NFSD_V2_ACL
 extern struct svc_version nfsd_acl_version2;

fs/nfsd/nfssvc.c +4 -6

index ee709fc..ab89fe0 100644
@@ -254,8 +254,6 @@ static void nfsd_shutdown(void)
 
 static void nfsd_last_thread(struct svc_serv *serv, struct net *net)
 {
-	/* When last nfsd thread exits we need to do some clean-up */
-	nfsd_serv = NULL;
 	nfsd_shutdown();
 
 	svc_rpcb_cleanup(serv, net);

@@ -348,7 +346,7 @@ int nfsd_create_serv(void)
 
 	error = svc_bind(nfsd_serv, current->nsproxy->net_ns);
 	if (error < 0) {
-		svc_destroy(nfsd_serv);
+		nfsd_destroy();
 		return error;
 	}

@@ -430,7 +428,7 @@ int nfsd_set_nrthreads(int n, int *nthreads)
 
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);
+	nfsd_destroy();
 
 	return err;
 }

@@ -480,7 +478,7 @@ out_shutdown:
 out_destroy:
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);		/* Release server */
+	nfsd_destroy();		/* Release server */
 out:
 	mutex_unlock(&nfsd_mutex);
 	return error;

@@ -684,7 +682,7 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
 	/* this function really, really should have been called svc_put() */
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);
+	nfsd_destroy();
 	mutex_unlock(&nfsd_mutex);
 	return ret;
 }

J. Bruce Fields

2012-05-23 21:52 UTC #1 lore raw

Quoted 17 lines · Stanislav Kinsbursky

Otherwise we will get NULL pointer dereference after svc_shutdown_net->sv_shutdown (nfsd_last_thread) call.
This looks safe, because all such operations (svc_destroy) are performed with nfsd_mutex being held.
Signed-off-by Stanislav Kinsbursky <[email protected]>
---
 fs/nfsd/nfsctl.c |    4 ++--
 fs/nfsd/nfsd.h   |    6 ++++++
 fs/nfsd/nfssvc.c |   10 ++++------
 3 files changed, 12 insertions(+), 8 deletions(-)
fs/nfsd/nfsctl.c +0 -0
index c55298e..82cca1e 100644

fs/nfsd/nfsctl.c static ssize_t __write_ports_addfd(char *buf) · line 678

Quoted 5 lines

fs/nfsd/nfsctl.c +1 -1

@@ -675,7 +675,7 @@ static ssize_t __write_ports_addfd(char *buf)
 	if (err < 0) {
 		if (nfsd_serv->sv_nrthreads == 1)
 			svc_shutdown_net(nfsd_serv, net);
-		svc_destroy(nfsd_serv);
+		nfsd_destroy();

The server could still be running here. We don't want to set nfsd_serv to NULL.

--b.

Quoted 79 lines

fs/nfsd/nfsctl.c +1 -1

 		return err;
 	}

@@ -746,7 +746,7 @@ out_close:
 out_err:
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);
+	nfsd_destroy();
 	return err;
 }

fs/nfsd/nfsd.h +6 -0

index 1671429..35db369 100644
@@ -73,6 +73,12 @@ int		nfsd_nrpools(void);
 int		nfsd_get_nrthreads(int n, int *);
 int		nfsd_set_nrthreads(int n, int *);
 
+static inline void nfsd_destroy(void)
+{
+	svc_destroy(nfsd_serv);
+	nfsd_serv = NULL;
+}
+
 #if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
 #ifdef CONFIG_NFSD_V2_ACL
 extern struct svc_version nfsd_acl_version2;

fs/nfsd/nfssvc.c +4 -6

index ee709fc..ab89fe0 100644
@@ -254,8 +254,6 @@ static void nfsd_shutdown(void)
 
 static void nfsd_last_thread(struct svc_serv *serv, struct net *net)
 {
-	/* When last nfsd thread exits we need to do some clean-up */
-	nfsd_serv = NULL;
 	nfsd_shutdown();
 
 	svc_rpcb_cleanup(serv, net);

@@ -348,7 +346,7 @@ int nfsd_create_serv(void)
 
 	error = svc_bind(nfsd_serv, current->nsproxy->net_ns);
 	if (error < 0) {
-		svc_destroy(nfsd_serv);
+		nfsd_destroy();
 		return error;
 	}

@@ -430,7 +428,7 @@ int nfsd_set_nrthreads(int n, int *nthreads)
 
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);
+	nfsd_destroy();
 
 	return err;
 }

@@ -480,7 +478,7 @@ out_shutdown:
 out_destroy:
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);		/* Release server */
+	nfsd_destroy();		/* Release server */
 out:
 	mutex_unlock(&nfsd_mutex);
 	return error;

@@ -684,7 +682,7 @@ int nfsd_pool_stats_release(struct inode *inode, struct file *file)
 	/* this function really, really should have been called svc_put() */
 	if (nfsd_serv->sv_nrthreads == 1)
 		svc_shutdown_net(nfsd_serv, net);
-	svc_destroy(nfsd_serv);
+	nfsd_destroy();
 	mutex_unlock(&nfsd_mutex);
 	return ret;
 }