by Simo Sorce

[permalink] [raw]

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

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

2014-01-05 23:30:48

On Mon, 6 Jan 2014 09:37:44 +1100
NeilBrown <[email protected]> wrote:

> On Thu, 2 Jan 2014 16:21:50 -0500 "J. Bruce Fields" <[email protected]>
> 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 <[email protected]>

Attachments:

signature.asc (836.00 B)

2014-01-02 21:35:49

by J. Bruce Fields

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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
>

2014-01-02 21:21:51

by J. Bruce Fields

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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
>

2014-01-03 08:14:10

by Simo Sorce

[permalink] [raw]

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

On Thu, 2014-01-02 at 18:27 -0500, Jeff Layton wrote:
> On Thu, 2 Jan 2014 17:40:10 -0500
> "J. Bruce Fields" <[email protected]> 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" <[email protected]> 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

2014-01-05 22:37:56

On Thu, 2 Jan 2014 17:40:10 -0500
"J. Bruce Fields" <[email protected]> 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" <[email protected]> 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 <[email protected]>