[PATCH] nfsd: make sure all sv_permsocks have lockd references

[PATCH] nfsd: make sure all sv_permsocks have lockd references

[Jeff Layton]

This patchset should apply cleanly to the other patches that I've sent
recently. They fix some (theoretical) problems with the text-based write
ports and nfsd_init_socks interfaces.

The problem is that nfsd_last_thread expects that every socket on
sv_permsocks has a corresponding lockd reference, but there a couple of
ways to get sockets onto that list without getting a lockd reference.

This patch fixes all that I could spot. I suspect that this, along with
the other patches that I proposed earlier will fix the problem that
Chris Vine reported.

I've done some basic smoke testing with them and they seem to work
as expected.

I've also put the entire patch stack in my git tree on kernel.org:

http://git.kernel.org/?p=linux/kernel/git/jlayton/linux.git;a=summary

...in the nfsd-error branch. It may be more convenient to pull them
from there.

Jeff Layton (2):
  nfsd: shut down transport in nfsd_init_socks if no lockd reference
  nfsd: have __write_ports_add/delxprt get/put lockd references for
    transports

 fs/nfsd/nfsctl.c |   59 ++++++++++++++++++++++++++++++++++++++---------------
 fs/nfsd/nfssvc.c |   21 +++++++++++++++---
 2 files changed, 59 insertions(+), 21 deletions(-)

[PATCH] nfsd: shut down transport in nfsd_init_socks if no lockd reference

[Jeff Layton]

nfsd_last_thread expects that any socket on sv_permsocks will hold a
lockd reference. Fix nfsd_init_socks to close down any transport that
it creates for which it can't get a lockd reference.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfssvc.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index a06ea99..f83e6f9 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -278,28 +278,41 @@ int nfsd_create_serv(void)
 static int nfsd_init_socks(int port)
 {
 	int error;
+	char *transport;
+	struct svc_xprt *xprt;
+
 	if (!list_empty(&nfsd_serv->sv_permsocks))
 		return 0;
 
-	error = svc_create_xprt(nfsd_serv, "udp", PF_INET, port,
+	transport = "udp";
+	error = svc_create_xprt(nfsd_serv, transport, PF_INET, port,
 					SVC_SOCK_DEFAULTS);
 	if (error < 0)
 		return error;
 
 	error = lockd_up();
 	if (error < 0)
-		return error;
+		goto out_close_xprt;
 
-	error = svc_create_xprt(nfsd_serv, "tcp", PF_INET, port,
+	transport = "tcp";
+	error = svc_create_xprt(nfsd_serv, transport, PF_INET, port,
 					SVC_SOCK_DEFAULTS);
 	if (error < 0)
 		return error;
 
 	error = lockd_up();
 	if (error < 0)
-		return error;
+		goto out_close_xprt;
 
 	return 0;
+out_close_xprt:
+	/* shut down any transport that didn't get a lockd reference */
+	xprt = svc_find_xprt(nfsd_serv, transport, PF_INET, port);
+	if (xprt != NULL) {
+		svc_close_xprt(xprt);
+		svc_xprt_put(xprt);
+	}
+	return error;
 }
 
 int nfsd_nrpools(void)
-- 
1.5.5.6

[PATCH] nfsd: have __write_ports_add/delxprt get/put lockd references for transports

[Jeff Layton]

nfsd_last_thread expects that any socket on sv_permsocks will hold a
lockd reference. Make sure that __write_ports_addxprt takes a lockd
reference for each socket it creates and have __write_ports_delxprt
release references when it closes a socket.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfsctl.c |   59 ++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 9e8645a..365aee0 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -989,6 +989,21 @@ static ssize_t __write_ports_delfd(char *buf)
 	return len;
 }
 
+static int
+nfsd_shutdown_xprt(const char *transport, const unsigned short family,
+		     const int port)
+{
+	struct svc_xprt *xprt;
+
+	xprt = svc_find_xprt(nfsd_serv, transport, family, port);
+	if (xprt == NULL)
+		return -ENOTCONN;
+
+	svc_close_xprt(xprt);
+	svc_xprt_put(xprt);
+	return 0;
+}
+
 /*
  * A transport listener is added by writing it's transport name and
  * a port number.
@@ -996,7 +1011,6 @@ static ssize_t __write_ports_delfd(char *buf)
 static ssize_t __write_ports_addxprt(char *buf)
 {
 	char transport[16];
-	struct svc_xprt *xprt;
 	int port, err;
 
 	if (sscanf(buf, "%15s %4u", transport, &port) != 2)
@@ -1014,20 +1028,29 @@ static ssize_t __write_ports_addxprt(char *buf)
 	if (err < 0)
 		goto out_err;
 
+	err = lockd_up();
+	if (err != 0) {
+		nfsd_shutdown_xprt(transport, PF_INET, port);
+		goto out_err;
+	}
+
 	err = svc_create_xprt(nfsd_serv, transport,
 				PF_INET6, port, SVC_SOCK_ANONYMOUS);
-	if (err < 0 && err != -EAFNOSUPPORT)
-		goto out_close;
+	if (err < 0 && err != -EAFNOSUPPORT) {
+		nfsd_shutdown_xprt(transport, PF_INET, port);
+		goto out_err;
+	}
+
+	err = lockd_up();
+	if (err != 0) {
+		nfsd_shutdown_xprt(transport, PF_INET6, port);
+		nfsd_shutdown_xprt(transport, PF_INET, port);
+		goto out_err;
+	}
 
 	/* Decrease the count, but don't shut down the service */
 	nfsd_serv->sv_nrthreads--;
 	return 0;
-out_close:
-	xprt = svc_find_xprt(nfsd_serv, transport, PF_INET, port);
-	if (xprt != NULL) {
-		svc_close_xprt(xprt);
-		svc_xprt_put(xprt);
-	}
 out_err:
 	svc_destroy(nfsd_serv);
 	return err;
@@ -1039,9 +1062,8 @@ out_err:
  */
 static ssize_t __write_ports_delxprt(char *buf)
 {
-	struct svc_xprt *xprt;
 	char transport[16];
-	int port;
+	int port, err;
 
 	if (sscanf(&buf[1], "%15s %4u", transport, &port) != 2)
 		return -EINVAL;
@@ -1049,13 +1071,16 @@ static ssize_t __write_ports_delxprt(char *buf)
 	if (port < 1 || port > USHRT_MAX || nfsd_serv == NULL)
 		return -EINVAL;
 
-	xprt = svc_find_xprt(nfsd_serv, transport, AF_UNSPEC, port);
-	if (xprt == NULL)
-		return -ENOTCONN;
+	/* PF_INET6 sockets are considered optional */
+	err = nfsd_shutdown_xprt(transport, PF_INET6, port);
+	if (err == 0)
+		lockd_down();
 
-	svc_close_xprt(xprt);
-	svc_xprt_put(xprt);
-	return 0;
+	err = nfsd_shutdown_xprt(transport, PF_INET, port);
+	if (err == 0)
+		lockd_down();
+
+	return err;
 }
 
 static ssize_t __write_ports(struct file *file, char *buf, size_t size)
-- 
1.5.5.6

Re: [PATCH] nfsd: have __write_ports_add/delxprt get/put lockd references for transports

[Jeff Layton]

On Thu, 17 Jun 2010 10:17:47 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> nfsd_last_thread expects that any socket on sv_permsocks will hold a
> lockd reference. Make sure that __write_ports_addxprt takes a lockd
> reference for each socket it creates and have __write_ports_delxprt
> release references when it closes a socket.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfsctl.c |   59 ++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 9e8645a..365aee0 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -989,6 +989,21 @@ static ssize_t __write_ports_delfd(char *buf)
>  	return len;
>  }
>  
> +static int
> +nfsd_shutdown_xprt(const char *transport, const unsigned short family,
> +		     const int port)
> +{
> +	struct svc_xprt *xprt;
> +
> +	xprt = svc_find_xprt(nfsd_serv, transport, family, port);
> +	if (xprt == NULL)
> +		return -ENOTCONN;
> +
> +	svc_close_xprt(xprt);
> +	svc_xprt_put(xprt);
> +	return 0;
> +}
> +
>  /*
>   * A transport listener is added by writing it's transport name and
>   * a port number.
> @@ -996,7 +1011,6 @@ static ssize_t __write_ports_delfd(char *buf)
>  static ssize_t __write_ports_addxprt(char *buf)
>  {
>  	char transport[16];
> -	struct svc_xprt *xprt;
>  	int port, err;
>  
>  	if (sscanf(buf, "%15s %4u", transport, &port) != 2)
> @@ -1014,20 +1028,29 @@ static ssize_t __write_ports_addxprt(char *buf)
>  	if (err < 0)
>  		goto out_err;
>  
> +	err = lockd_up();
> +	if (err != 0) {
> +		nfsd_shutdown_xprt(transport, PF_INET, port);
> +		goto out_err;
> +	}
> +
>  	err = svc_create_xprt(nfsd_serv, transport,
>  				PF_INET6, port, SVC_SOCK_ANONYMOUS);
> -	if (err < 0 && err != -EAFNOSUPPORT)
> -		goto out_close;
> +	if (err < 0 && err != -EAFNOSUPPORT) {
> +		nfsd_shutdown_xprt(transport, PF_INET, port);
> +		goto out_err;
> +	}
> +
> +	err = lockd_up();
> +	if (err != 0) {
> +		nfsd_shutdown_xprt(transport, PF_INET6, port);
> +		nfsd_shutdown_xprt(transport, PF_INET, port);

Oops, I just realized I made a mistake here. We need to release the
lockd reference that was taken earlier here. Let me respin and I'll
resend...

> +		goto out_err;
> +	}
>  
>  	/* Decrease the count, but don't shut down the service */
>  	nfsd_serv->sv_nrthreads--;
>  	return 0;
> -out_close:
> -	xprt = svc_find_xprt(nfsd_serv, transport, PF_INET, port);
> -	if (xprt != NULL) {
> -		svc_close_xprt(xprt);
> -		svc_xprt_put(xprt);
> -	}
>  out_err:
>  	svc_destroy(nfsd_serv);
>  	return err;
> @@ -1039,9 +1062,8 @@ out_err:
>   */
>  static ssize_t __write_ports_delxprt(char *buf)
>  {
> -	struct svc_xprt *xprt;
>  	char transport[16];
> -	int port;
> +	int port, err;
>  
>  	if (sscanf(&buf[1], "%15s %4u", transport, &port) != 2)
>  		return -EINVAL;
> @@ -1049,13 +1071,16 @@ static ssize_t __write_ports_delxprt(char *buf)
>  	if (port < 1 || port > USHRT_MAX || nfsd_serv == NULL)
>  		return -EINVAL;
>  
> -	xprt = svc_find_xprt(nfsd_serv, transport, AF_UNSPEC, port);
> -	if (xprt == NULL)
> -		return -ENOTCONN;
> +	/* PF_INET6 sockets are considered optional */
> +	err = nfsd_shutdown_xprt(transport, PF_INET6, port);
> +	if (err == 0)
> +		lockd_down();
>  
> -	svc_close_xprt(xprt);
> -	svc_xprt_put(xprt);
> -	return 0;
> +	err = nfsd_shutdown_xprt(transport, PF_INET, port);
> +	if (err == 0)
> +		lockd_down();
> +
> +	return err;
>  }
>  
>  static ssize_t __write_ports(struct file *file, char *buf, size_t size)


-- 
Jeff Layton <jlayton@redhat.com>