[RFC PATCH 0/5] sunrpc: change handling of use-gss-proxy file

[RFC PATCH 0/5] sunrpc: change handling of use-gss-proxy file

[Jeff Layton]

Consider this patch a RFC at this point. This changes how the
use-gss-proxy file works, and alters the kernel upcall to delay a little
bit before giving up on it if gssproxy isn't up yet.

It's not heavily tested yet, but it seems to do the right thing...

I think the first patch isn't too controversial. The big question is
whether the initial upcall delay is desirable.

Thoughts?

Jeff Layton (5):
  sunrpc: don't wait for write before allowing reads from use-gss-proxy
    file
  sunrpc: don't hang indefinitely in wait_for_gss_proxy
  sunrpc: wait for gssproxy to start on initial upcall attempt before
    falling back to legacy upcall
  sunrpc: fix potential race between setting use_gss_proxy and the
    upcall rpc_clnt
  sunrpc: allow gssproxy to be explicitly disabled from userland

 net/sunrpc/auth_gss/svcauth_gss.c | 72 +++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 33 deletions(-)

-- 
1.8.4.2

[RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Jeff Layton]

It doesn't make much sense to make reads from this procfile hang. As
far as I can tell, only gssproxy itself will open this file and it
never reads from it. Change it to just give the present setting of
sn->use_gss_proxy without waiting for anything.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 008cdad..5e9323e 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1355,16 +1355,12 @@ static ssize_t read_gssp(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
 	struct net *net = PDE_DATA(file_inode(file));
+	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
 	unsigned long p = *ppos;
 	char tbuf[10];
 	size_t len;
-	int ret;
-
-	ret = wait_for_gss_proxy(net, file);
-	if (ret)
-		return ret;
 
-	snprintf(tbuf, sizeof(tbuf), "%d\n", use_gss_proxy(net));
+	snprintf(tbuf, sizeof(tbuf), "%d\n", sn->use_gss_proxy);
 	len = strlen(tbuf);
 	if (p >= len)
 		return 0;
-- 
1.8.4.2

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[J. Bruce Fields]

On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> It doesn't make much sense to make reads from this procfile hang. As
> far as I can tell, only gssproxy itself will open this file and it
> never reads from it. Change it to just give the present setting of
> sn->use_gss_proxy without waiting for anything.

I think my *only* reason for doing this was to give a simple way to wait
for gss-proxy to start (just wait for a read to return).

As long as gss-proxy has some way to say "I'm up and running", and as
long as that comes after writing to use-gss-proxy, we're fine.

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  net/sunrpc/auth_gss/svcauth_gss.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 008cdad..5e9323e 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1355,16 +1355,12 @@ static ssize_t read_gssp(struct file *file, char __user *buf,
>  			 size_t count, loff_t *ppos)
>  {
>  	struct net *net = PDE_DATA(file_inode(file));
> +	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
>  	unsigned long p = *ppos;
>  	char tbuf[10];
>  	size_t len;
> -	int ret;
> -
> -	ret = wait_for_gss_proxy(net, file);
> -	if (ret)
> -		return ret;
>  
> -	snprintf(tbuf, sizeof(tbuf), "%d\n", use_gss_proxy(net));
> +	snprintf(tbuf, sizeof(tbuf), "%d\n", sn->use_gss_proxy);
>  	len = strlen(tbuf);
>  	if (p >= len)
>  		return 0;
> -- 
> 1.8.4.2
> 

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Jeff Layton]

On Thu, 2 Jan 2014 16:21:50 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > It doesn't make much sense to make reads from this procfile hang. As
> > far as I can tell, only gssproxy itself will open this file and it
> > never reads from it. Change it to just give the present setting of
> > sn->use_gss_proxy without waiting for anything.
> 
> I think my *only* reason for doing this was to give a simple way to wait
> for gss-proxy to start (just wait for a read to return).
> 

What wasn't clear to me is what would be doing this read.

I'll take it from your comment then that patch #1 is acceptable?

> As long as gss-proxy has some way to say "I'm up and running", and as
> long as that comes after writing to use-gss-proxy, we're fine.
> 

I'll let Simo confirm that that's what gssproxy does, but yes that is
the desired behavior. Typically this is done by ensuring that the parent
process when daemon()-izing doesn't exit until everything is ready.

If gssproxy does need to be changed for that, we have a library routine
now in nfs-utils that does this that you can likely copy (see
mydaemon()).

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[J. Bruce Fields]

On Thu, Jan 02, 2014 at 05:26:51PM -0500, Jeff Layton wrote:
> On Thu, 2 Jan 2014 16:21:50 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > It doesn't make much sense to make reads from this procfile hang. As
> > > far as I can tell, only gssproxy itself will open this file and it
> > > never reads from it. Change it to just give the present setting of
> > > sn->use_gss_proxy without waiting for anything.
> > 
> > I think my *only* reason for doing this was to give a simple way to wait
> > for gss-proxy to start (just wait for a read to return).
> > 
> 
> What wasn't clear to me is what would be doing this read.
> 
> I'll take it from your comment then that patch #1 is acceptable?

Yes.  Thanks!

> > As long as gss-proxy has some way to say "I'm up and running", and as
> > long as that comes after writing to use-gss-proxy, we're fine.
> > 
> 
> I'll let Simo confirm that that's what gssproxy does, but yes that is
> the desired behavior. Typically this is done by ensuring that the parent
> process when daemon()-izing doesn't exit until everything is ready.
> 
> If gssproxy does need to be changed for that, we have a library routine
> now in nfs-utils that does this that you can likely copy (see
> mydaemon()).

>From a quick skim: looks like gss-proxy does this in init_server().  So
I think it might need something like the below?

(Untested.  I spent a total of maybe five minutes looking at this code
so have no idea what I'm doing.)

--b.

diff --git a/proxy/src/gssproxy.c b/proxy/src/gssproxy.c
index 1fca922..a7cbd7c 100644
--- a/proxy/src/gssproxy.c
+++ b/proxy/src/gssproxy.c
@@ -97,6 +97,12 @@ int main(int argc, const char *argv[])
         exit(EXIT_FAILURE);
     }
 
+    /*
+     * special call to tell the Linux kernel gss-proxy is available.
+     * Note this must be done before nfsd is started.
+     */
+    init_proc_nfsd(gpctx->config);
+
     init_server(gpctx->config->daemonize);
 
     write_pid();
@@ -139,9 +145,6 @@ int main(int argc, const char *argv[])
         }
     }
 
-    /* special call to tell the Linux kernel gss-proxy is available */
-    init_proc_nfsd(gpctx->config);
-
     ret = gp_workers_init(gpctx);
     if (ret) {
         exit(EXIT_FAILURE);

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Jeff Layton]

On Thu, 2 Jan 2014 17:40:10 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Jan 02, 2014 at 05:26:51PM -0500, Jeff Layton wrote:
> > On Thu, 2 Jan 2014 16:21:50 -0500
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > far as I can tell, only gssproxy itself will open this file and it
> > > > never reads from it. Change it to just give the present setting of
> > > > sn->use_gss_proxy without waiting for anything.
> > > 
> > > I think my *only* reason for doing this was to give a simple way to wait
> > > for gss-proxy to start (just wait for a read to return).
> > > 
> > 
> > What wasn't clear to me is what would be doing this read.
> > 
> > I'll take it from your comment then that patch #1 is acceptable?
> 
> Yes.  Thanks!
> 
> > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > long as that comes after writing to use-gss-proxy, we're fine.
> > > 
> > 
> > I'll let Simo confirm that that's what gssproxy does, but yes that is
> > the desired behavior. Typically this is done by ensuring that the parent
> > process when daemon()-izing doesn't exit until everything is ready.
> > 
> > If gssproxy does need to be changed for that, we have a library routine
> > now in nfs-utils that does this that you can likely copy (see
> > mydaemon()).
> 
> From a quick skim: looks like gss-proxy does this in init_server().  So
> I think it might need something like the below?
> 
> (Untested.  I spent a total of maybe five minutes looking at this code
> so have no idea what I'm doing.)
> 
> --b.
> 
> diff --git a/proxy/src/gssproxy.c b/proxy/src/gssproxy.c
> index 1fca922..a7cbd7c 100644
> --- a/proxy/src/gssproxy.c
> +++ b/proxy/src/gssproxy.c
> @@ -97,6 +97,12 @@ int main(int argc, const char *argv[])
>          exit(EXIT_FAILURE);
>      }
>  
> +    /*
> +     * special call to tell the Linux kernel gss-proxy is available.
> +     * Note this must be done before nfsd is started.
> +     */
> +    init_proc_nfsd(gpctx->config);
> +
>      init_server(gpctx->config->daemonize);
>  
>      write_pid();
> @@ -139,9 +145,6 @@ int main(int argc, const char *argv[])
>          }
>      }
>  
> -    /* special call to tell the Linux kernel gss-proxy is available */
> -    init_proc_nfsd(gpctx->config);
> -
>      ret = gp_workers_init(gpctx);
>      if (ret) {
>          exit(EXIT_FAILURE);

That doesn't look quite right to me. That has it calling init_proc_nfsd
before any of the unix sockets are set up.

I think gss-proxy will probably need to do something similar to what
mydaemon does; set up a pipe, and have the parent just block reading
from it until the child writes to it. At that point the parent can exit
and the pipe can be closed in the child.

See:

    http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=support/nfs/mydaemon.c;h=e885d60f3a808299730a4d62dec49142fdc3ba5f;hb=HEAD

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Simo Sorce]

On Thu, 2014-01-02 at 18:27 -0500, Jeff Layton wrote:
> On Thu, 2 Jan 2014 17:40:10 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Thu, Jan 02, 2014 at 05:26:51PM -0500, Jeff Layton wrote:
> > > On Thu, 2 Jan 2014 16:21:50 -0500
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > > far as I can tell, only gssproxy itself will open this file and it
> > > > > never reads from it. Change it to just give the present setting of
> > > > > sn->use_gss_proxy without waiting for anything.
> > > > 
> > > > I think my *only* reason for doing this was to give a simple way to wait
> > > > for gss-proxy to start (just wait for a read to return).
> > > > 
> > > 
> > > What wasn't clear to me is what would be doing this read.
> > > 
> > > I'll take it from your comment then that patch #1 is acceptable?
> > 
> > Yes.  Thanks!
> > 
> > > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > > long as that comes after writing to use-gss-proxy, we're fine.
> > > > 
> > > 
> > > I'll let Simo confirm that that's what gssproxy does, but yes that is
> > > the desired behavior. Typically this is done by ensuring that the parent
> > > process when daemon()-izing doesn't exit until everything is ready.
> > > 
> > > If gssproxy does need to be changed for that, we have a library routine
> > > now in nfs-utils that does this that you can likely copy (see
> > > mydaemon()).
> > 
> > From a quick skim: looks like gss-proxy does this in init_server().  So
> > I think it might need something like the below?
> > 
> > (Untested.  I spent a total of maybe five minutes looking at this code
> > so have no idea what I'm doing.)
> > 
> > --b.
> > 
> > diff --git a/proxy/src/gssproxy.c b/proxy/src/gssproxy.c
> > index 1fca922..a7cbd7c 100644
> > --- a/proxy/src/gssproxy.c
> > +++ b/proxy/src/gssproxy.c
> > @@ -97,6 +97,12 @@ int main(int argc, const char *argv[])
> >          exit(EXIT_FAILURE);
> >      }
> >  
> > +    /*
> > +     * special call to tell the Linux kernel gss-proxy is available.
> > +     * Note this must be done before nfsd is started.
> > +     */
> > +    init_proc_nfsd(gpctx->config);
> > +
> >      init_server(gpctx->config->daemonize);
> >  
> >      write_pid();
> > @@ -139,9 +145,6 @@ int main(int argc, const char *argv[])
> >          }
> >      }
> >  
> > -    /* special call to tell the Linux kernel gss-proxy is available */
> > -    init_proc_nfsd(gpctx->config);
> > -
> >      ret = gp_workers_init(gpctx);
> >      if (ret) {
> >          exit(EXIT_FAILURE);
> 
> That doesn't look quite right to me. That has it calling init_proc_nfsd
> before any of the unix sockets are set up.
> 
> I think gss-proxy will probably need to do something similar to what
> mydaemon does; set up a pipe, and have the parent just block reading
> from it until the child writes to it. At that point the parent can exit
> and the pipe can be closed in the child.
> 
> See:
> 
>     http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=support/nfs/mydaemon.c;h=e885d60f3a808299730a4d62dec49142fdc3ba5f;hb=HEAD
> 

I'll take a look tomorrow, I created upstream ticket #114 to track this.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[J. Bruce Fields]

On Fri, Jan 03, 2014 at 03:14:03AM -0500, Simo Sorce wrote:
> On Thu, 2014-01-02 at 18:27 -0500, Jeff Layton wrote:
> > On Thu, 2 Jan 2014 17:40:10 -0500
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Thu, Jan 02, 2014 at 05:26:51PM -0500, Jeff Layton wrote:
> > > > On Thu, 2 Jan 2014 16:21:50 -0500
> > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > 
> > > > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > > > far as I can tell, only gssproxy itself will open this file and it
> > > > > > never reads from it. Change it to just give the present setting of
> > > > > > sn->use_gss_proxy without waiting for anything.
> > > > > 
> > > > > I think my *only* reason for doing this was to give a simple way to wait
> > > > > for gss-proxy to start (just wait for a read to return).
> > > > > 
> > > > 
> > > > What wasn't clear to me is what would be doing this read.
> > > > 
> > > > I'll take it from your comment then that patch #1 is acceptable?
> > > 
> > > Yes.  Thanks!
> > > 
> > > > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > > > long as that comes after writing to use-gss-proxy, we're fine.
> > > > > 
> > > > 
> > > > I'll let Simo confirm that that's what gssproxy does, but yes that is
> > > > the desired behavior. Typically this is done by ensuring that the parent
> > > > process when daemon()-izing doesn't exit until everything is ready.
> > > > 
> > > > If gssproxy does need to be changed for that, we have a library routine
> > > > now in nfs-utils that does this that you can likely copy (see
> > > > mydaemon()).
> > > 
> > > From a quick skim: looks like gss-proxy does this in init_server().  So
> > > I think it might need something like the below?
> > > 
> > > (Untested.  I spent a total of maybe five minutes looking at this code
> > > so have no idea what I'm doing.)
> > > 
> > > --b.
> > > 
> > > diff --git a/proxy/src/gssproxy.c b/proxy/src/gssproxy.c
> > > index 1fca922..a7cbd7c 100644
> > > --- a/proxy/src/gssproxy.c
> > > +++ b/proxy/src/gssproxy.c
> > > @@ -97,6 +97,12 @@ int main(int argc, const char *argv[])
> > >          exit(EXIT_FAILURE);
> > >      }
> > >  
> > > +    /*
> > > +     * special call to tell the Linux kernel gss-proxy is available.
> > > +     * Note this must be done before nfsd is started.
> > > +     */
> > > +    init_proc_nfsd(gpctx->config);
> > > +
> > >      init_server(gpctx->config->daemonize);
> > >  
> > >      write_pid();
> > > @@ -139,9 +145,6 @@ int main(int argc, const char *argv[])
> > >          }
> > >      }
> > >  
> > > -    /* special call to tell the Linux kernel gss-proxy is available */
> > > -    init_proc_nfsd(gpctx->config);
> > > -
> > >      ret = gp_workers_init(gpctx);
> > >      if (ret) {
> > >          exit(EXIT_FAILURE);
> > 
> > That doesn't look quite right to me. That has it calling init_proc_nfsd
> > before any of the unix sockets are set up.
> > 
> > I think gss-proxy will probably need to do something similar to what
> > mydaemon does; set up a pipe, and have the parent just block reading
> > from it until the child writes to it. At that point the parent can exit
> > and the pipe can be closed in the child.
> > 
> > See:
> > 
> >     http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=support/nfs/mydaemon.c;h=e885d60f3a808299730a4d62dec49142fdc3ba5f;hb=HEAD
> > 
> 
> I'll take a look tomorrow, I created upstream ticket #114 to track this.

Thanks!

I notice there's also sd_notify(3) which avoids the double-fork and
self-pipe, but you might consider that too much of a systemd dependency.

--b.

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Simo Sorce]

On Fri, 2014-01-03 at 11:23 -0500, J. Bruce Fields wrote:
> On Fri, Jan 03, 2014 at 03:14:03AM -0500, Simo Sorce wrote:
> > On Thu, 2014-01-02 at 18:27 -0500, Jeff Layton wrote:
> > > On Thu, 2 Jan 2014 17:40:10 -0500
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Thu, Jan 02, 2014 at 05:26:51PM -0500, Jeff Layton wrote:
> > > > > On Thu, 2 Jan 2014 16:21:50 -0500
> > > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > > 
> > > > > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > > > > far as I can tell, only gssproxy itself will open this file and it
> > > > > > > never reads from it. Change it to just give the present setting of
> > > > > > > sn->use_gss_proxy without waiting for anything.
> > > > > > 
> > > > > > I think my *only* reason for doing this was to give a simple way to wait
> > > > > > for gss-proxy to start (just wait for a read to return).
> > > > > > 
> > > > > 
> > > > > What wasn't clear to me is what would be doing this read.
> > > > > 
> > > > > I'll take it from your comment then that patch #1 is acceptable?
> > > > 
> > > > Yes.  Thanks!
> > > > 
> > > > > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > > > > long as that comes after writing to use-gss-proxy, we're fine.
> > > > > > 
> > > > > 
> > > > > I'll let Simo confirm that that's what gssproxy does, but yes that is
> > > > > the desired behavior. Typically this is done by ensuring that the parent
> > > > > process when daemon()-izing doesn't exit until everything is ready.
> > > > > 
> > > > > If gssproxy does need to be changed for that, we have a library routine
> > > > > now in nfs-utils that does this that you can likely copy (see
> > > > > mydaemon()).
> > > > 
> > > > From a quick skim: looks like gss-proxy does this in init_server().  So
> > > > I think it might need something like the below?
> > > > 
> > > > (Untested.  I spent a total of maybe five minutes looking at this code
> > > > so have no idea what I'm doing.)
> > > > 
> > > > --b.
> > > > 
> > > > diff --git a/proxy/src/gssproxy.c b/proxy/src/gssproxy.c
> > > > index 1fca922..a7cbd7c 100644
> > > > --- a/proxy/src/gssproxy.c
> > > > +++ b/proxy/src/gssproxy.c
> > > > @@ -97,6 +97,12 @@ int main(int argc, const char *argv[])
> > > >          exit(EXIT_FAILURE);
> > > >      }
> > > >  
> > > > +    /*
> > > > +     * special call to tell the Linux kernel gss-proxy is available.
> > > > +     * Note this must be done before nfsd is started.
> > > > +     */
> > > > +    init_proc_nfsd(gpctx->config);
> > > > +
> > > >      init_server(gpctx->config->daemonize);
> > > >  
> > > >      write_pid();
> > > > @@ -139,9 +145,6 @@ int main(int argc, const char *argv[])
> > > >          }
> > > >      }
> > > >  
> > > > -    /* special call to tell the Linux kernel gss-proxy is available */
> > > > -    init_proc_nfsd(gpctx->config);
> > > > -
> > > >      ret = gp_workers_init(gpctx);
> > > >      if (ret) {
> > > >          exit(EXIT_FAILURE);
> > > 
> > > That doesn't look quite right to me. That has it calling init_proc_nfsd
> > > before any of the unix sockets are set up.
> > > 
> > > I think gss-proxy will probably need to do something similar to what
> > > mydaemon does; set up a pipe, and have the parent just block reading
> > > from it until the child writes to it. At that point the parent can exit
> > > and the pipe can be closed in the child.
> > > 
> > > See:
> > > 
> > >     http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=support/nfs/mydaemon.c;h=e885d60f3a808299730a4d62dec49142fdc3ba5f;hb=HEAD
> > > 
> > 
> > I'll take a look tomorrow, I created upstream ticket #114 to track this.
> 
> Thanks!
> 
> I notice there's also sd_notify(3) which avoids the double-fork and
> self-pipe, but you might consider that too much of a systemd dependency.

I'd like to use sd_notify, but preferred a more conservative approach
for wider distribution portability.

Patch here waiting for review upstream:
http://fedorapeople.org/cgit/simo/public_git/gss-proxy.git/commit/?h=usermode&id=ddc5eb950bbd2050dc76b4783f3d3383cd89bccf

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[J. Bruce Fields]

On Fri, Jan 03, 2014 at 05:06:00PM -0500, Simo Sorce wrote:
> I'd like to use sd_notify, but preferred a more conservative approach
> for wider distribution portability.
> 
> Patch here waiting for review upstream:
> http://fedorapeople.org/cgit/simo/public_git/gss-proxy.git/commit/?h=usermode&id=ddc5eb950bbd2050dc76b4783f3d3383cd89bccf

Thanks.  If it's not too much verbage, it might be helpful to document
the reason for the ordering; something like:

	/*
	 * We need to tell nfsd gss-proxy is available before it starts,
	 * as nfsd may need to know this the moment it receives the
	 * first init_sec_context call.
	 *
	 * So now it is safe to tell the init system that we're done
	 * starting up and that it can continue with nfsd startup.
	 */

?

--b.

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Simo Sorce]

On Fri, 2014-01-03 at 17:34 -0500, J. Bruce Fields wrote:
> On Fri, Jan 03, 2014 at 05:06:00PM -0500, Simo Sorce wrote:
> > I'd like to use sd_notify, but preferred a more conservative approach
> > for wider distribution portability.
> > 
> > Patch here waiting for review upstream:
> > http://fedorapeople.org/cgit/simo/public_git/gss-proxy.git/commit/?h=usermode&id=ddc5eb950bbd2050dc76b4783f3d3383cd89bccf
> 
> Thanks.  If it's not too much verbage, it might be helpful to document
> the reason for the ordering; something like:
> 
> 	/*
> 	 * We need to tell nfsd gss-proxy is available before it starts,
> 	 * as nfsd may need to know this the moment it receives the
> 	 * first init_sec_context call.
> 	 *
> 	 * So now it is safe to tell the init system that we're done
> 	 * starting up and that it can continue with nfsd startup.
> 	 */
> 
> ?

Good idea, I changed the comments loosely after your example:
http://fedorapeople.org/cgit/simo/public_git/gss-proxy.git/commit/?h=usermode&id=393570b45816b690cb16fd1286d0705142ef2d62

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[J. Bruce Fields]

On Sat, Jan 04, 2014 at 10:28:22AM -0500, Simo Sorce wrote:
> On Fri, 2014-01-03 at 17:34 -0500, J. Bruce Fields wrote:
> > On Fri, Jan 03, 2014 at 05:06:00PM -0500, Simo Sorce wrote:
> > > I'd like to use sd_notify, but preferred a more conservative approach
> > > for wider distribution portability.
> > > 
> > > Patch here waiting for review upstream:
> > > http://fedorapeople.org/cgit/simo/public_git/gss-proxy.git/commit/?h=usermode&id=ddc5eb950bbd2050dc76b4783f3d3383cd89bccf
> > 
> > Thanks.  If it's not too much verbage, it might be helpful to document
> > the reason for the ordering; something like:
> > 
> > 	/*
> > 	 * We need to tell nfsd gss-proxy is available before it starts,
> > 	 * as nfsd may need to know this the moment it receives the
> > 	 * first init_sec_context call.
> > 	 *
> > 	 * So now it is safe to tell the init system that we're done
> > 	 * starting up and that it can continue with nfsd startup.
> > 	 */
> > 
> > ?
> 
> Good idea, I changed the comments loosely after your example:
> http://fedorapeople.org/cgit/simo/public_git/gss-proxy.git/commit/?h=usermode&id=393570b45816b690cb16fd1286d0705142ef2d62

I like how you've done it.  Thanks!

--b.

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Jeff Layton]

On Fri, 03 Jan 2014 17:06:00 -0500
Simo Sorce <simo@redhat.com> wrote:

> On Fri, 2014-01-03 at 11:23 -0500, J. Bruce Fields wrote:
> > On Fri, Jan 03, 2014 at 03:14:03AM -0500, Simo Sorce wrote:
> > > On Thu, 2014-01-02 at 18:27 -0500, Jeff Layton wrote:
> > > > On Thu, 2 Jan 2014 17:40:10 -0500
> > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > 
> > > > > On Thu, Jan 02, 2014 at 05:26:51PM -0500, Jeff Layton wrote:
> > > > > > On Thu, 2 Jan 2014 16:21:50 -0500
> > > > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > > > 
> > > > > > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > > > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > > > > > far as I can tell, only gssproxy itself will open this file and it
> > > > > > > > never reads from it. Change it to just give the present setting of
> > > > > > > > sn->use_gss_proxy without waiting for anything.
> > > > > > > 
> > > > > > > I think my *only* reason for doing this was to give a simple way to wait
> > > > > > > for gss-proxy to start (just wait for a read to return).
> > > > > > > 
> > > > > > 
> > > > > > What wasn't clear to me is what would be doing this read.
> > > > > > 
> > > > > > I'll take it from your comment then that patch #1 is acceptable?
> > > > > 
> > > > > Yes.  Thanks!
> > > > > 
> > > > > > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > > > > > long as that comes after writing to use-gss-proxy, we're fine.
> > > > > > > 
> > > > > > 
> > > > > > I'll let Simo confirm that that's what gssproxy does, but yes that is
> > > > > > the desired behavior. Typically this is done by ensuring that the parent
> > > > > > process when daemon()-izing doesn't exit until everything is ready.
> > > > > > 
> > > > > > If gssproxy does need to be changed for that, we have a library routine
> > > > > > now in nfs-utils that does this that you can likely copy (see
> > > > > > mydaemon()).
> > > > > 
> > > > > From a quick skim: looks like gss-proxy does this in init_server().  So
> > > > > I think it might need something like the below?
> > > > > 
> > > > > (Untested.  I spent a total of maybe five minutes looking at this code
> > > > > so have no idea what I'm doing.)
> > > > > 
> > > > > --b.
> > > > > 
> > > > > diff --git a/proxy/src/gssproxy.c b/proxy/src/gssproxy.c
> > > > > index 1fca922..a7cbd7c 100644
> > > > > --- a/proxy/src/gssproxy.c
> > > > > +++ b/proxy/src/gssproxy.c
> > > > > @@ -97,6 +97,12 @@ int main(int argc, const char *argv[])
> > > > >          exit(EXIT_FAILURE);
> > > > >      }
> > > > >  
> > > > > +    /*
> > > > > +     * special call to tell the Linux kernel gss-proxy is available.
> > > > > +     * Note this must be done before nfsd is started.
> > > > > +     */
> > > > > +    init_proc_nfsd(gpctx->config);
> > > > > +
> > > > >      init_server(gpctx->config->daemonize);
> > > > >  
> > > > >      write_pid();
> > > > > @@ -139,9 +145,6 @@ int main(int argc, const char *argv[])
> > > > >          }
> > > > >      }
> > > > >  
> > > > > -    /* special call to tell the Linux kernel gss-proxy is available */
> > > > > -    init_proc_nfsd(gpctx->config);
> > > > > -
> > > > >      ret = gp_workers_init(gpctx);
> > > > >      if (ret) {
> > > > >          exit(EXIT_FAILURE);
> > > > 
> > > > That doesn't look quite right to me. That has it calling init_proc_nfsd
> > > > before any of the unix sockets are set up.
> > > > 
> > > > I think gss-proxy will probably need to do something similar to what
> > > > mydaemon does; set up a pipe, and have the parent just block reading
> > > > from it until the child writes to it. At that point the parent can exit
> > > > and the pipe can be closed in the child.
> > > > 
> > > > See:
> > > > 
> > > >     http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blob;f=support/nfs/mydaemon.c;h=e885d60f3a808299730a4d62dec49142fdc3ba5f;hb=HEAD
> > > > 
> > > 
> > > I'll take a look tomorrow, I created upstream ticket #114 to track this.
> > 
> > Thanks!
> > 
> > I notice there's also sd_notify(3) which avoids the double-fork and
> > self-pipe, but you might consider that too much of a systemd dependency.
> 
> I'd like to use sd_notify, but preferred a more conservative approach
> for wider distribution portability.
> 
> Patch here waiting for review upstream:
> http://fedorapeople.org/cgit/simo/public_git/gss-proxy.git/commit/?h=usermode&id=ddc5eb950bbd2050dc76b4783f3d3383cd89bccf
> 
> Simo.
> 

Looks good to me, but I'll ditto Bruce's request for comments...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]

On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
wrote:

> On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > It doesn't make much sense to make reads from this procfile hang. As
> > far as I can tell, only gssproxy itself will open this file and it
> > never reads from it. Change it to just give the present setting of
> > sn->use_gss_proxy without waiting for anything.
> 
> I think my *only* reason for doing this was to give a simple way to wait
> for gss-proxy to start (just wait for a read to return).
> 
> As long as gss-proxy has some way to say "I'm up and running", and as
> long as that comes after writing to use-gss-proxy, we're fine.
> 


Only tangentially related to the above email .....

I had a look at this new-fangled gssproxy thing and while it mostly seems
like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
kernel source .... disturbing.
You never know when some user-space might want to change that - maybe to
"/run/gssproxy.sock" (unlikely I know - but possible).

Probably the easiest would be to hand the path to the kernel.

e.g. instead  of writing '1' to "use-gss-proxy", we could 
  echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy

Then you could even use an 'abstract' socket name if you wanted. i.e. one
starting with a nul and which doesn't exist anywhere in the filesystem.
I would feel a lot more comfortable with that than with the current
hard-coding.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[J. Bruce Fields]

On Mon, Jan 06, 2014 at 09:37:44AM +1100, NeilBrown wrote:
> On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> wrote:
> 
> > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > It doesn't make much sense to make reads from this procfile hang. As
> > > far as I can tell, only gssproxy itself will open this file and it
> > > never reads from it. Change it to just give the present setting of
> > > sn->use_gss_proxy without waiting for anything.
> > 
> > I think my *only* reason for doing this was to give a simple way to wait
> > for gss-proxy to start (just wait for a read to return).
> > 
> > As long as gss-proxy has some way to say "I'm up and running", and as
> > long as that comes after writing to use-gss-proxy, we're fine.
> > 
> 
> 
> Only tangentially related to the above email .....
> 
> I had a look at this new-fangled gssproxy thing and while it mostly seems
> like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
> kernel source .... disturbing.
> You never know when some user-space might want to change that - maybe to
> "/run/gssproxy.sock" (unlikely I know - but possible).
> 
> Probably the easiest would be to hand the path to the kernel.
> 
> e.g. instead  of writing '1' to "use-gss-proxy", we could 
>   echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy
> 
> Then you could even use an 'abstract' socket name if you wanted. i.e. one
> starting with a nul and which doesn't exist anywhere in the filesystem.
> I would feel a lot more comfortable with that than with the current
> hard-coding.

See also RPCBIND_SOCK_PATHNAME.  (I *think* that's completely hardcoded,
not just a default.)

I get the general principle.  I have a hard time seeing how it would be
a problem in practice.

If we wanted to do as you suggest, I suppose we could even special-case
the string "1" (at least for a while) to make the change
backwards-compatible.

I'd like to see the same argument made for the rpcbind case.

--b.

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3044 bytes --]

On Sun, 5 Jan 2014 17:54:21 -0500 "J. Bruce Fields" <bfields@fieldses.org>
wrote:

> On Mon, Jan 06, 2014 at 09:37:44AM +1100, NeilBrown wrote:
> > On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> > wrote:
> > 
> > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > far as I can tell, only gssproxy itself will open this file and it
> > > > never reads from it. Change it to just give the present setting of
> > > > sn->use_gss_proxy without waiting for anything.
> > > 
> > > I think my *only* reason for doing this was to give a simple way to wait
> > > for gss-proxy to start (just wait for a read to return).
> > > 
> > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > long as that comes after writing to use-gss-proxy, we're fine.
> > > 
> > 
> > 
> > Only tangentially related to the above email .....
> > 
> > I had a look at this new-fangled gssproxy thing and while it mostly seems
> > like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
> > kernel source .... disturbing.
> > You never know when some user-space might want to change that - maybe to
> > "/run/gssproxy.sock" (unlikely I know - but possible).
> > 
> > Probably the easiest would be to hand the path to the kernel.
> > 
> > e.g. instead  of writing '1' to "use-gss-proxy", we could 
> >   echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy
> > 
> > Then you could even use an 'abstract' socket name if you wanted. i.e. one
> > starting with a nul and which doesn't exist anywhere in the filesystem.
> > I would feel a lot more comfortable with that than with the current
> > hard-coding.
> 
> See also RPCBIND_SOCK_PATHNAME.  (I *think* that's completely hardcoded,
> not just a default.)

Arrgghhh!! (Hi Chuck.  Yes, I agree it is "undesirable in the long term").

> 
> I get the general principle.  I have a hard time seeing how it would be
> a problem in practice.

One day some distro will decide that it is time to get rid of the legacy
symlink (or bind mount) of /var/run -> /run.  And they will test it out and
everything will work fine.  So they will commit to it.
And then when they release it, their customers will discover that NFS doesn't
work properly with gss (because the distro forgot to test that).

> 
> If we wanted to do as you suggest, I suppose we could even special-case
> the string "1" (at least for a while) to make the change
> backwards-compatible.

I like the idea.

> 
> I'd like to see the same argument made for the rpcbind case.

I hereby make said argument.  Hardcoding path names in the kernel is
Just-Wrong(TM).  Even /sbin/init is configurable.

NeilBrown


> 
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Chuck Lever]

On Jan 5, 2014, at 6:30 PM, NeilBrown <neilb@suse.de> wrote:

> On Sun, 5 Jan 2014 17:54:21 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> wrote:
> 
>> On Mon, Jan 06, 2014 at 09:37:44AM +1100, NeilBrown wrote:
>>> On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
>>> wrote:
>>> 
>>>> On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
>>>>> It doesn't make much sense to make reads from this procfile hang. As
>>>>> far as I can tell, only gssproxy itself will open this file and it
>>>>> never reads from it. Change it to just give the present setting of
>>>>> sn->use_gss_proxy without waiting for anything.
>>>> 
>>>> I think my *only* reason for doing this was to give a simple way to wait
>>>> for gss-proxy to start (just wait for a read to return).
>>>> 
>>>> As long as gss-proxy has some way to say "I'm up and running", and as
>>>> long as that comes after writing to use-gss-proxy, we're fine.
>>>> 
>>> 
>>> 
>>> Only tangentially related to the above email .....
>>> 
>>> I had a look at this new-fangled gssproxy thing and while it mostly seems
>>> like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
>>> kernel source .... disturbing.
>>> You never know when some user-space might want to change that - maybe to
>>> "/run/gssproxy.sock" (unlikely I know - but possible).
>>> 
>>> Probably the easiest would be to hand the path to the kernel.
>>> 
>>> e.g. instead  of writing '1' to "use-gss-proxy", we could 
>>>  echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy
>>> 
>>> Then you could even use an 'abstract' socket name if you wanted. i.e. one
>>> starting with a nul and which doesn't exist anywhere in the filesystem.
>>> I would feel a lot more comfortable with that than with the current
>>> hard-coding.
>> 
>> See also RPCBIND_SOCK_PATHNAME.  (I *think* that's completely hardcoded,
>> not just a default.)
> 
> Arrgghhh!! (Hi Chuck.  Yes, I agree it is "undesirable in the long term").

I think that, when I added rpcbind AF_LOCAL support, we just didn't have a way for the kernel to find out what socket pathname had been chosen by user space.  But for gssproxy, we should think hard about the security ramifications of allowing user space to tell the kernel how to access its security helpers.

> 
>> 
>> I get the general principle.  I have a hard time seeing how it would be
>> a problem in practice.
> 
> One day some distro will decide that it is time to get rid of the legacy
> symlink (or bind mount) of /var/run -> /run.  And they will test it out and
> everything will work fine.  So they will commit to it.
> And then when they release it, their customers will discover that NFS doesn't
> work properly with gss (because the distro forgot to test that).

Right, but we're not at that point yet (or are we?).  Maybe we should wait until we get there, to avoid over-design.  Why would it be infeasible for such a distribution to find the hard-coded pathname in the kernel and simply change it and rebuild?

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Jeff Layton]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

On Mon, 6 Jan 2014 09:37:44 +1100
NeilBrown <neilb@suse.de> wrote:

> On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> wrote:
> 
> > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > It doesn't make much sense to make reads from this procfile hang. As
> > > far as I can tell, only gssproxy itself will open this file and it
> > > never reads from it. Change it to just give the present setting of
> > > sn->use_gss_proxy without waiting for anything.
> > 
> > I think my *only* reason for doing this was to give a simple way to wait
> > for gss-proxy to start (just wait for a read to return).
> > 
> > As long as gss-proxy has some way to say "I'm up and running", and as
> > long as that comes after writing to use-gss-proxy, we're fine.
> > 
> 
> 
> Only tangentially related to the above email .....
> 
> I had a look at this new-fangled gssproxy thing and while it mostly seems
> like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
> kernel source .... disturbing.
> You never know when some user-space might want to change that - maybe to
> "/run/gssproxy.sock" (unlikely I know - but possible).
> 
> Probably the easiest would be to hand the path to the kernel.
> 
> e.g. instead  of writing '1' to "use-gss-proxy", we could 
>   echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy
> 
> Then you could even use an 'abstract' socket name if you wanted. i.e. one
> starting with a nul and which doesn't exist anywhere in the filesystem.
> I would feel a lot more comfortable with that than with the current
> hard-coding.
> 

I like that idea -- particularly if you keep the legacy behavior that
writing a '1' to the file makes it default to /var/run/gssproxy.sock so
we don't break compatability with older gssproxy releases.

-- 
Jeff Layton <jlayton@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Simo Sorce]

On Sun, 2014-01-05 at 20:45 -0500, Jeff Layton wrote:
> On Mon, 6 Jan 2014 09:37:44 +1100
> NeilBrown <neilb@suse.de> wrote:
> 
> > On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> > wrote:
> > 
> > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > far as I can tell, only gssproxy itself will open this file and it
> > > > never reads from it. Change it to just give the present setting of
> > > > sn->use_gss_proxy without waiting for anything.
> > > 
> > > I think my *only* reason for doing this was to give a simple way to wait
> > > for gss-proxy to start (just wait for a read to return).
> > > 
> > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > long as that comes after writing to use-gss-proxy, we're fine.
> > > 
> > 
> > 
> > Only tangentially related to the above email .....
> > 
> > I had a look at this new-fangled gssproxy thing and while it mostly seems
> > like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
> > kernel source .... disturbing.
> > You never know when some user-space might want to change that - maybe to
> > "/run/gssproxy.sock" (unlikely I know - but possible).
> > 
> > Probably the easiest would be to hand the path to the kernel.
> > 
> > e.g. instead  of writing '1' to "use-gss-proxy", we could 
> >   echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy
> > 
> > Then you could even use an 'abstract' socket name if you wanted. i.e. one
> > starting with a nul and which doesn't exist anywhere in the filesystem.
> > I would feel a lot more comfortable with that than with the current
> > hard-coding.
> > 
> 
> I like that idea -- particularly if you keep the legacy behavior that
> writing a '1' to the file makes it default to /var/run/gssproxy.sock so
> we don't break compatability with older gssproxy releases.

I have no problem adding this to gss-proxy but I wonder if it is really
that important.

In what case gss-proxy will not be able to create a file
named /var/run/gssproxy.sock ? The only case would be for the distro to
outlaw creating a path named /var/run, note that /var/run does not need
to be the same as /run for gssproxy to be able to create a socket.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[J. Bruce Fields]

On Mon, Jan 06, 2014 at 01:36:13AM -0500, Simo Sorce wrote:
> On Sun, 2014-01-05 at 20:45 -0500, Jeff Layton wrote:
> > On Mon, 6 Jan 2014 09:37:44 +1100
> > NeilBrown <neilb@suse.de> wrote:
> > 
> > > On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> > > wrote:
> > > 
> > > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > > far as I can tell, only gssproxy itself will open this file and it
> > > > > never reads from it. Change it to just give the present setting of
> > > > > sn->use_gss_proxy without waiting for anything.
> > > > 
> > > > I think my *only* reason for doing this was to give a simple way to wait
> > > > for gss-proxy to start (just wait for a read to return).
> > > > 
> > > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > > long as that comes after writing to use-gss-proxy, we're fine.
> > > > 
> > > 
> > > 
> > > Only tangentially related to the above email .....
> > > 
> > > I had a look at this new-fangled gssproxy thing and while it mostly seems
> > > like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
> > > kernel source .... disturbing.
> > > You never know when some user-space might want to change that - maybe to
> > > "/run/gssproxy.sock" (unlikely I know - but possible).
> > > 
> > > Probably the easiest would be to hand the path to the kernel.
> > > 
> > > e.g. instead  of writing '1' to "use-gss-proxy", we could 
> > >   echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy
> > > 
> > > Then you could even use an 'abstract' socket name if you wanted. i.e. one
> > > starting with a nul and which doesn't exist anywhere in the filesystem.
> > > I would feel a lot more comfortable with that than with the current
> > > hard-coding.
> > > 
> > 
> > I like that idea -- particularly if you keep the legacy behavior that
> > writing a '1' to the file makes it default to /var/run/gssproxy.sock so
> > we don't break compatability with older gssproxy releases.
> 
> I have no problem adding this to gss-proxy but I wonder if it is really
> that important.
> 
> In what case gss-proxy will not be able to create a file
> named /var/run/gssproxy.sock ? The only case would be for the distro to
> outlaw creating a path named /var/run, note that /var/run does not need
> to be the same as /run for gssproxy to be able to create a socket.

Well, I suppose we could fix the hard-coded kernel paths but still leave
it hard-coded in gss-proxy until someone demonstrated a need for it to
be configurable.

I like the principle but don't see this as a very high priority.

--b.

Re: [RFC PATCH 1/5] sunrpc: don't wait for write before allowing reads from use-gss-proxy file

[Simo Sorce]

On Mon, 2014-01-06 at 10:04 -0500, J. Bruce Fields wrote:
> On Mon, Jan 06, 2014 at 01:36:13AM -0500, Simo Sorce wrote:
> > On Sun, 2014-01-05 at 20:45 -0500, Jeff Layton wrote:
> > > On Mon, 6 Jan 2014 09:37:44 +1100
> > > NeilBrown <neilb@suse.de> wrote:
> > > 
> > > > On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <bfields@fieldses.org>
> > > > wrote:
> > > > 
> > > > > On Wed, Jan 01, 2014 at 07:28:30AM -0500, Jeff Layton wrote:
> > > > > > It doesn't make much sense to make reads from this procfile hang. As
> > > > > > far as I can tell, only gssproxy itself will open this file and it
> > > > > > never reads from it. Change it to just give the present setting of
> > > > > > sn->use_gss_proxy without waiting for anything.
> > > > > 
> > > > > I think my *only* reason for doing this was to give a simple way to wait
> > > > > for gss-proxy to start (just wait for a read to return).
> > > > > 
> > > > > As long as gss-proxy has some way to say "I'm up and running", and as
> > > > > long as that comes after writing to use-gss-proxy, we're fine.
> > > > > 
> > > > 
> > > > 
> > > > Only tangentially related to the above email .....
> > > > 
> > > > I had a look at this new-fangled gssproxy thing and while it mostly seems
> > > > like a good idea, I find the hard-coding of "/var/run/gssproxy.sock" in the
> > > > kernel source .... disturbing.
> > > > You never know when some user-space might want to change that - maybe to
> > > > "/run/gssproxy.sock" (unlikely I know - but possible).
> > > > 
> > > > Probably the easiest would be to hand the path to the kernel.
> > > > 
> > > > e.g. instead  of writing '1' to "use-gss-proxy", we could 
> > > >   echo /my/path/gss-proxy-sock > /proc/net/rpc/use-gss-proxy
> > > > 
> > > > Then you could even use an 'abstract' socket name if you wanted. i.e. one
> > > > starting with a nul and which doesn't exist anywhere in the filesystem.
> > > > I would feel a lot more comfortable with that than with the current
> > > > hard-coding.
> > > > 
> > > 
> > > I like that idea -- particularly if you keep the legacy behavior that
> > > writing a '1' to the file makes it default to /var/run/gssproxy.sock so
> > > we don't break compatability with older gssproxy releases.
> > 
> > I have no problem adding this to gss-proxy but I wonder if it is really
> > that important.
> > 
> > In what case gss-proxy will not be able to create a file
> > named /var/run/gssproxy.sock ? The only case would be for the distro to
> > outlaw creating a path named /var/run, note that /var/run does not need
> > to be the same as /run for gssproxy to be able to create a socket.
> 
> Well, I suppose we could fix the hard-coded kernel paths but still leave
> it hard-coded in gss-proxy until someone demonstrated a need for it to
> be configurable.

Sockets are already configurable in gss-proxy, so I would just need to
change the init function that writes to proc, not a big deal.

> I like the principle but don't see this as a very high priority.

Me either, but if someone contributes the kernel side I will fix
gss-proxy in short order to follow it.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[RFC PATCH 2/5] sunrpc: don't hang indefinitely in wait_for_gss_proxy

[Jeff Layton]

A later patch will make upcalls delay for a bit if gssproxy isn't
up yet. Make wait_for_gss_proxy just delay for a little while before
giving up and disabling it. The 5s here is definitely negotiable.

Also, since there are no callers now, we can get rid of the file
pointer here and eliminate the O_NONBLOCK goop.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 5e9323e..64c025e 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1313,13 +1313,23 @@ static inline bool gssp_ready(struct sunrpc_net *sn)
 	return false;
 }
 
-static int wait_for_gss_proxy(struct net *net, struct file *file)
+/*
+ * The initial upcall to gssproxy will wait 5s for the use_gss_proxy
+ * var to change to something besides -1.
+ */
+#define GSSP_INITIAL_UPCALL_TIMEOUT	(5 * HZ)
+
+static int wait_for_gss_proxy(struct net *net)
 {
+	int ret;
 	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
 
-	if (file->f_flags & O_NONBLOCK && !gssp_ready(sn))
-		return -EAGAIN;
-	return wait_event_interruptible(sn->gssp_wq, gssp_ready(sn));
+	ret = wait_event_interruptible_timeout(sn->gssp_wq, gssp_ready(sn),
+					       GSSP_INITIAL_UPCALL_TIMEOUT);
+	/* If we timed out, disable gssp */
+	if (ret == 0)
+		set_gss_proxy(net, 0);
+	return ret < 0 ? ret : 0;
 }
 
 
-- 
1.8.4.2

[RFC PATCH 3/5] sunrpc: wait for gssproxy to start on initial upcall attempt before falling back to legacy upcall

[Jeff Layton]

Currently, the only thing that waits for gssproxy to start is reads
from the use-gss-proxy procfile. That's a little odd, since nothing
but gssproxy itself will likely ever open that file, and all it does
it write to it.

It seems like what we really want instead is for svcrpc code to give
gssproxy a little time to start up in the event that some RPCs come
in before it's ready.

Change it so that the upcall instead waits a little while for gssproxy
to start before giving up. If the wait times out, we go ahead and
disable gssproxy so that later upcalls don't delay.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 64c025e..921f388 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1265,24 +1265,6 @@ out:
 
 DEFINE_SPINLOCK(use_gssp_lock);
 
-static bool use_gss_proxy(struct net *net)
-{
-	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
-
-	if (sn->use_gss_proxy != -1)
-		return sn->use_gss_proxy;
-	spin_lock(&use_gssp_lock);
-	/*
-	 * If you wanted gss-proxy, you should have said so before
-	 * starting to accept requests:
-	 */
-	sn->use_gss_proxy = 0;
-	spin_unlock(&use_gssp_lock);
-	return 0;
-}
-
-#ifdef CONFIG_PROC_FS
-
 static int set_gss_proxy(struct net *net, int type)
 {
 	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
@@ -1332,6 +1314,16 @@ static int wait_for_gss_proxy(struct net *net)
 	return ret < 0 ? ret : 0;
 }
 
+static bool use_gss_proxy(struct net *net)
+{
+	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
+
+	if (wait_for_gss_proxy(net))
+		return false;
+	return gssp_ready(sn);
+}
+
+#ifdef CONFIG_PROC_FS
 
 static ssize_t write_gssp(struct file *file, const char __user *buf,
 			 size_t count, loff_t *ppos)
-- 
1.8.4.2

Re: [RFC PATCH 3/5] sunrpc: wait for gssproxy to start on initial upcall attempt before falling back to legacy upcall

[J. Bruce Fields]

On Wed, Jan 01, 2014 at 07:28:32AM -0500, Jeff Layton wrote:
> Currently, the only thing that waits for gssproxy to start is reads
> from the use-gss-proxy procfile. That's a little odd, since nothing
> but gssproxy itself will likely ever open that file, and all it does
> it write to it.
> 
> It seems like what we really want instead is for svcrpc code to give
> gssproxy a little time to start up in the event that some RPCs come
> in before it's ready.

Legacy userspace will never write, so this adds an 5-second delay in
that case.

I'd rather avoid any arbitrary timeout.

It seems to me all you need is for gss-proxy to ensure that it does the
write to use-gss-proxy before it indicates to whoever started it that
it's done starting.

Then as long as the init system orders gss-proxy startup before nfsd
startup, we're good.

--b.

> 
> Change it so that the upcall instead waits a little while for gssproxy
> to start before giving up. If the wait times out, we go ahead and
> disable gssproxy so that later upcalls don't delay.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  net/sunrpc/auth_gss/svcauth_gss.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 64c025e..921f388 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1265,24 +1265,6 @@ out:
>  
>  DEFINE_SPINLOCK(use_gssp_lock);
>  
> -static bool use_gss_proxy(struct net *net)
> -{
> -	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> -
> -	if (sn->use_gss_proxy != -1)
> -		return sn->use_gss_proxy;
> -	spin_lock(&use_gssp_lock);
> -	/*
> -	 * If you wanted gss-proxy, you should have said so before
> -	 * starting to accept requests:
> -	 */
> -	sn->use_gss_proxy = 0;
> -	spin_unlock(&use_gssp_lock);
> -	return 0;
> -}
> -
> -#ifdef CONFIG_PROC_FS
> -
>  static int set_gss_proxy(struct net *net, int type)
>  {
>  	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> @@ -1332,6 +1314,16 @@ static int wait_for_gss_proxy(struct net *net)
>  	return ret < 0 ? ret : 0;
>  }
>  
> +static bool use_gss_proxy(struct net *net)
> +{
> +	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> +
> +	if (wait_for_gss_proxy(net))
> +		return false;
> +	return gssp_ready(sn);
> +}
> +
> +#ifdef CONFIG_PROC_FS
>  
>  static ssize_t write_gssp(struct file *file, const char __user *buf,
>  			 size_t count, loff_t *ppos)
> -- 
> 1.8.4.2
> 

Re: [RFC PATCH 3/5] sunrpc: wait for gssproxy to start on initial upcall attempt before falling back to legacy upcall

[Jeff Layton]

On Thu, 2 Jan 2014 16:35:47 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jan 01, 2014 at 07:28:32AM -0500, Jeff Layton wrote:
> > Currently, the only thing that waits for gssproxy to start is reads
> > from the use-gss-proxy procfile. That's a little odd, since nothing
> > but gssproxy itself will likely ever open that file, and all it does
> > it write to it.
> > 
> > It seems like what we really want instead is for svcrpc code to give
> > gssproxy a little time to start up in the event that some RPCs come
> > in before it's ready.
> 
> Legacy userspace will never write, so this adds an 5-second delay in
> that case.
> 
> I'd rather avoid any arbitrary timeout.
> 
> It seems to me all you need is for gss-proxy to ensure that it does the
> write to use-gss-proxy before it indicates to whoever started it that
> it's done starting.
> 
> Then as long as the init system orders gss-proxy startup before nfsd
> startup, we're good.
> 
> --b.
> 

Yep, that 5s delay on the first upcall was the part that was
questionable...

Ok, so basically the behavior you want is:

"If gssproxy is up and running by the time the first RPC comes in then
 use that. If not, then use the legacy code."

I'll respin the set with that behavior in mind. I need to do a bit of
testing with it too, so it may be a week or so before I repost.

> > 
> > Change it so that the upcall instead waits a little while for gssproxy
> > to start before giving up. If the wait times out, we go ahead and
> > disable gssproxy so that later upcalls don't delay.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  net/sunrpc/auth_gss/svcauth_gss.c | 28 ++++++++++------------------
> >  1 file changed, 10 insertions(+), 18 deletions(-)
> > 
> > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > index 64c025e..921f388 100644
> > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > @@ -1265,24 +1265,6 @@ out:
> >  
> >  DEFINE_SPINLOCK(use_gssp_lock);
> >  
> > -static bool use_gss_proxy(struct net *net)
> > -{
> > -	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> > -
> > -	if (sn->use_gss_proxy != -1)
> > -		return sn->use_gss_proxy;
> > -	spin_lock(&use_gssp_lock);
> > -	/*
> > -	 * If you wanted gss-proxy, you should have said so before
> > -	 * starting to accept requests:
> > -	 */
> > -	sn->use_gss_proxy = 0;
> > -	spin_unlock(&use_gssp_lock);
> > -	return 0;
> > -}
> > -
> > -#ifdef CONFIG_PROC_FS
> > -
> >  static int set_gss_proxy(struct net *net, int type)
> >  {
> >  	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> > @@ -1332,6 +1314,16 @@ static int wait_for_gss_proxy(struct net *net)
> >  	return ret < 0 ? ret : 0;
> >  }
> >  
> > +static bool use_gss_proxy(struct net *net)
> > +{
> > +	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> > +
> > +	if (wait_for_gss_proxy(net))
> > +		return false;
> > +	return gssp_ready(sn);
> > +}
> > +
> > +#ifdef CONFIG_PROC_FS
> >  
> >  static ssize_t write_gssp(struct file *file, const char __user *buf,
> >  			 size_t count, loff_t *ppos)
> > -- 
> > 1.8.4.2
> > 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH 3/5] sunrpc: wait for gssproxy to start on initial upcall attempt before falling back to legacy upcall

[J. Bruce Fields]

On Thu, Jan 02, 2014 at 06:10:55PM -0500, Jeff Layton wrote:
> On Thu, 2 Jan 2014 16:35:47 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Jan 01, 2014 at 07:28:32AM -0500, Jeff Layton wrote:
> > > Currently, the only thing that waits for gssproxy to start is reads
> > > from the use-gss-proxy procfile. That's a little odd, since nothing
> > > but gssproxy itself will likely ever open that file, and all it does
> > > it write to it.
> > > 
> > > It seems like what we really want instead is for svcrpc code to give
> > > gssproxy a little time to start up in the event that some RPCs come
> > > in before it's ready.
> > 
> > Legacy userspace will never write, so this adds an 5-second delay in
> > that case.
> > 
> > I'd rather avoid any arbitrary timeout.
> > 
> > It seems to me all you need is for gss-proxy to ensure that it does the
> > write to use-gss-proxy before it indicates to whoever started it that
> > it's done starting.
> > 
> > Then as long as the init system orders gss-proxy startup before nfsd
> > startup, we're good.
> > 
> > --b.
> > 
> 
> Yep, that 5s delay on the first upcall was the part that was
> questionable...
> 
> Ok, so basically the behavior you want is:
> 
> "If gssproxy is up and running by the time the first RPC comes in then
>  use that. If not, then use the legacy code."
> 
> I'll respin the set with that behavior in mind. I need to do a bit of
> testing with it too, so it may be a week or so before I repost.

Thanks!

On a quick skim, I thought that rendered the later patches mostly
unnecessary, but I may not have been paying close enough attention....

--b.

> 
> > > 
> > > Change it so that the upcall instead waits a little while for gssproxy
> > > to start before giving up. If the wait times out, we go ahead and
> > > disable gssproxy so that later upcalls don't delay.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  net/sunrpc/auth_gss/svcauth_gss.c | 28 ++++++++++------------------
> > >  1 file changed, 10 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > > index 64c025e..921f388 100644
> > > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > > @@ -1265,24 +1265,6 @@ out:
> > >  
> > >  DEFINE_SPINLOCK(use_gssp_lock);
> > >  
> > > -static bool use_gss_proxy(struct net *net)
> > > -{
> > > -	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> > > -
> > > -	if (sn->use_gss_proxy != -1)
> > > -		return sn->use_gss_proxy;
> > > -	spin_lock(&use_gssp_lock);
> > > -	/*
> > > -	 * If you wanted gss-proxy, you should have said so before
> > > -	 * starting to accept requests:
> > > -	 */
> > > -	sn->use_gss_proxy = 0;
> > > -	spin_unlock(&use_gssp_lock);
> > > -	return 0;
> > > -}
> > > -
> > > -#ifdef CONFIG_PROC_FS
> > > -
> > >  static int set_gss_proxy(struct net *net, int type)
> > >  {
> > >  	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> > > @@ -1332,6 +1314,16 @@ static int wait_for_gss_proxy(struct net *net)
> > >  	return ret < 0 ? ret : 0;
> > >  }
> > >  
> > > +static bool use_gss_proxy(struct net *net)
> > > +{
> > > +	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
> > > +
> > > +	if (wait_for_gss_proxy(net))
> > > +		return false;
> > > +	return gssp_ready(sn);
> > > +}
> > > +
> > > +#ifdef CONFIG_PROC_FS
> > >  
> > >  static ssize_t write_gssp(struct file *file, const char __user *buf,
> > >  			 size_t count, loff_t *ppos)
> > > -- 
> > > 1.8.4.2
> > > 
> 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH 3/5] sunrpc: wait for gssproxy to start on initial upcall attempt before falling back to legacy upcall

[Jeff Layton]

On Fri, 3 Jan 2014 11:33:47 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Jan 02, 2014 at 06:10:55PM -0500, Jeff Layton wrote:
> > On Thu, 2 Jan 2014 16:35:47 -0500
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Wed, Jan 01, 2014 at 07:28:32AM -0500, Jeff Layton wrote:
> > > > Currently, the only thing that waits for gssproxy to start is reads
> > > > from the use-gss-proxy procfile. That's a little odd, since nothing
> > > > but gssproxy itself will likely ever open that file, and all it does
> > > > it write to it.
> > > > 
> > > > It seems like what we really want instead is for svcrpc code to give
> > > > gssproxy a little time to start up in the event that some RPCs come
> > > > in before it's ready.
> > > 
> > > Legacy userspace will never write, so this adds an 5-second delay in
> > > that case.
> > > 
> > > I'd rather avoid any arbitrary timeout.
> > > 
> > > It seems to me all you need is for gss-proxy to ensure that it does the
> > > write to use-gss-proxy before it indicates to whoever started it that
> > > it's done starting.
> > > 
> > > Then as long as the init system orders gss-proxy startup before nfsd
> > > startup, we're good.
> > > 
> > > --b.
> > > 
> > 
> > Yep, that 5s delay on the first upcall was the part that was
> > questionable...
> > 
> > Ok, so basically the behavior you want is:
> > 
> > "If gssproxy is up and running by the time the first RPC comes in then
> >  use that. If not, then use the legacy code."
> > 
> > I'll respin the set with that behavior in mind. I need to do a bit of
> > testing with it too, so it may be a week or so before I repost.
> 
> Thanks!
> 
> On a quick skim, I thought that rendered the later patches mostly
> unnecessary, but I may not have been paying close enough attention....
> 
> --b.
> 

For the most part, yes...

There is some code that can be removed however since we don't need that
wait anymore. Also, the race between setting the variable and setting up the
client still exists, and I think the use_gssp_lock is really
unnecessary. We can use cmpxchg() to get the same result without a spinlock.

-- 
Jeff Layton <jlayton@redhat.com>

[RFC PATCH 4/5] sunrpc: fix potential race between setting use_gss_proxy and the upcall rpc_clnt

[Jeff Layton]

Currently, the write_gssp code will change the variable and wake up any
waiters waiting on that change. It then goes and tries to set the
gssp_clnt. This is racy -- a task waiting on the set_gss_proxy call may
end up waking up and then subsequently finding that the gss_clnt isn't
there yet and end up not using it even though it'll soon be ready.

This patch reverses the order of operations. The gssp_clnt is created
first, and the variable change is done only if that succeeds.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 921f388..17c24bd 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1344,10 +1344,10 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
 		return res;
 	if (i != 1)
 		return -EINVAL;
-	res = set_gss_proxy(net, 1);
+	res = set_gssp_clnt(net);
 	if (res)
 		return res;
-	res = set_gssp_clnt(net);
+	res = set_gss_proxy(net, 1);
 	if (res)
 		return res;
 	return count;
-- 
1.8.4.2

[RFC PATCH 5/5] sunrpc: allow gssproxy to be explicitly disabled from userland

[Jeff Layton]

Writing anything but '1' to the use-gss-proxy file currently results in
an error. This means that you can't explicitly disable gssproxy. Change
it so that it allows a '0' to be written there as well.

With this, we can potentially have rpc.svcgssd write a 0 to this file
to get around the initial 5s hang on RPCs if gssproxy is not in use.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 17c24bd..aa30d49 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1342,12 +1342,20 @@ static ssize_t write_gssp(struct file *file, const char __user *buf,
 	res = kstrtoul(tbuf, 0, &i);
 	if (res)
 		return res;
-	if (i != 1)
+
+	switch (i) {
+	case 1:
+		res = set_gssp_clnt(net);
+		if (res)
+			return res;
+		break;
+	case 0:
+		break;
+	default:
 		return -EINVAL;
-	res = set_gssp_clnt(net);
-	if (res)
-		return res;
-	res = set_gss_proxy(net, 1);
+	}
+
+	res = set_gss_proxy(net, i);
 	if (res)
 		return res;
 	return count;
-- 
1.8.4.2

Re: [RFC PATCH 0/5] sunrpc: change handling of use-gss-proxy file

[Simo Sorce]

On Wed, 2014-01-01 at 07:28 -0500, Jeff Layton wrote:
> Consider this patch a RFC at this point. This changes how the
> use-gss-proxy file works, and alters the kernel upcall to delay a little
> bit before giving up on it if gssproxy isn't up yet.
> 
> It's not heavily tested yet, but it seems to do the right thing...
> 
> I think the first patch isn't too controversial. The big question is
> whether the initial upcall delay is desirable.
> 
> Thoughts?
> 
> Jeff Layton (5):
>   sunrpc: don't wait for write before allowing reads from use-gss-proxy
>     file
>   sunrpc: don't hang indefinitely in wait_for_gss_proxy
>   sunrpc: wait for gssproxy to start on initial upcall attempt before
>     falling back to legacy upcall
>   sunrpc: fix potential race between setting use_gss_proxy and the
>     upcall rpc_clnt
>   sunrpc: allow gssproxy to be explicitly disabled from userland
> 
>  net/sunrpc/auth_gss/svcauth_gss.c | 72 +++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 33 deletions(-)
> 

I like the whole patchset but ahven't tested any of it. so consider a
tentative ack by me.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York