by Jeff Layton

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] sunrpc/nfs: more reliable detection of running gssd

On Thu, 14 Nov 2013 14:26:01 -0500
"J. Bruce Fields" <[email protected]> wrote:

> On Wed, Nov 13, 2013 at 09:30:50AM -0500, Jeff Layton wrote:
> > v2:
> > - change name of toplevel pipefs dir from "dummy" to "gssd" (per
> > Trond's suggestion)
> >
> > - when gssd isn't running, don't bother to upcall (per Neil B.'s
> > suggestion)
> >
> > - fix lifecycle of rpc_pipe data. Previously it would have leaked
> > after umount. With this set, it's created and destroyed along with
> > the netns, and just attached to the pipe inode on mount/unmount
> > of rpc_pipefs.
> >
> > - patch has been added to skip attempting setclientid with krb5i
> > if gssd isn't running. This avoids the "AUTH_GSS upcall timed out"
> > message when gssd isn't running and you mount with sec=sys. It also
> > shortens the delay when gssd isn't up.
> >
> > The original cover letter from the v1 posting follows. Note that this
> > set does address the warnings about the AUTH_GSS upcall timing out.
> >
> > -------------------------[snip]-----------------------------
> >
> > We've gotten a lot of complaints recently about the 15s delay when
> > doing a sec=sys mount without gssd running.
> >
> > A large part of the problem is that the kernel isn't able to reliably
> > detect when rpc.gssd is running. What we currently have is a
> > gssd_running flag that is initially set to 1. When an upcall times out,
> > that gets set to 0, and subsequent upcalls get a much shorter timeout
> > (1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.
> >
> > The approach of using a flag like this is pretty inadequate. First, it
> > doesn't eliminate the long delay on the initial upcall attempt. Also,
> > if gssd spontaneously dies, then the flag will still be set to 1 until
> > the next upcall attempt times out. Finally, it currently requires that
> > the pipe be reopened in order to reset the flag back to true.
> >
> > This patchset replaces that flag with a more reliable mechanism for
> > detecting when gssd is running. When rpc_pipefs is mounted, it creates a
> > new "dummy" pipe that gssd will naturally find and hold open. We'll
> > never send an upcall down this pipe, and writing to it always fails.
> > But, since we can detect when something is holding it open, we can use
> > that to determine whether gssd is running.
>
> I think this might have been addressed before, I don't remember: does
> the init system currently have a way to wait till gssd has gotten as far
> as scanning pipefs before allowing mounts?
>
> (To avoid the race where a krb5 mount fails because gssd is still in the
> process of being started.)
>
> --b.
>

I don't think it does.

gssd forks (using daemon()) before it does any real work, and then the
parent exits. I imagine it's possible to hit such a race.

Personally, I've never seen it happen in practice, and I have tested
sec=krb5 mounts in /etc/fstab on recent Fedora. That may just be a
matter of timing or of enough stuff running between rpc.gssd and the
mounts being done.

It might not hurt to patch gssd so that the parent delays exiting until
the child starts up and does its initial run in the !fg case...

--
Jeff Layton <[email protected]>

2013-11-13 15:49:16

by Myklebust, Trond

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] nfs: check if gssd is running before attempting to use krb5i auth in SETCLIENTID call

On Nov 13, 2013, at 10:35, Jeff Layton <[email protected]> wrote:

> On Wed, 13 Nov 2013 10:14:30 -0500
> Chuck Lever <[email protected]> wrote:
>
>>
>> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <[email protected]> wrote:
>>
>>>
>>> On Nov 13, 2013, at 9:38, Chuck Lever <[email protected]> wrote:
>>>
>>>>
>>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <[email protected]> wrote:
>>>>
>>>>> Currently, the client will attempt to use krb5i in the SETCLIENTID call
>>>>> even if rpc.gssd is running. If that fails, it'll then fall back to
>>>>> RPC_AUTH_UNIX. This introduced a delay when mounting if rpc.gssd isn't
>>>>> running, and causes warning messages to pop up in the ring buffer.
>>>>>
>>>>> Check to see if rpc.gssd is running before even attempting to use krb5i
>>>>> auth, and just silently skip trying to do so if it isn't.
>>>>>
>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>> ---
>>>>>
>>>>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>>>>> index b4a160a..988aebf 100644
>>>>> --- a/fs/nfs/nfs4client.c
>>>>> +++ b/fs/nfs/nfs4client.c
>>>>> @@ -8,6 +8,7 @@
>>>>> #include <linux/nfs_mount.h>
>>>>> #include <linux/sunrpc/addr.h>
>>>>> #include <linux/sunrpc/auth.h>
>>>>> +#include <linux/sunrpc/auth_gss.h>
>>>>> #include <linux/sunrpc/xprt.h>
>>>>> #include <linux/sunrpc/bc_xprt.h>
>>>>> #include "internal.h"
>>>>> @@ -351,7 +352,7 @@ static int nfs4_init_client_minor_version(struct nfs_client *clp)
>>>>> */
>>>>> struct nfs_client *nfs4_init_client(struct nfs_client *clp,
>>>>> const struct rpc_timeout *timeparms,
>>>>> - const char *ip_addr)
>>>>> + const char *ip_addr, struct net *net)
>>>
>>> Why the ?struct net? argument? clp->cl_net should already be initialized here.
>>>
>>>>> {
>>>>> char buf[INET6_ADDRSTRLEN + 1];
>>>>> struct nfs_client *old;
>>>>> @@ -370,7 +371,10 @@ struct nfs_client *nfs4_init_client(struct nfs_client *clp,
>>>>> __set_bit(NFS_CS_INFINITE_SLOTS, &clp->cl_flags);
>>>>> __set_bit(NFS_CS_DISCRTRY, &clp->cl_flags);
>>>>> __set_bit(NFS_CS_NO_RETRANS_TIMEOUT, &clp->cl_flags);
>>>>> - error = nfs_create_rpc_client(clp, timeparms, RPC_AUTH_GSS_KRB5I);
>>>>> +
>>>>> + error = -EINVAL;
>>>>> + if (gssd_running(net))
>>>>> + error = nfs_create_rpc_client(clp, timeparms,RPC_AUTH_GSS_KRB5I);
>>>>> if (error == -EINVAL)
>>>>> error = nfs_create_rpc_client(clp, timeparms, RPC_AUTH_UNIX);
>>>>
>>>> This feels like a layering violation. Why wouldn't you put a gssd_running check in gss_create(), for example, and have rpcauth_create() return -EINVAL?
>>>>
>>>
>>> It would be better to put it in the upcall.
>>
>> Waiting until the upcall has its benefits. At that point, GSSD (if it is running) can report other errors, such as that there is no material to form a machine credential.
>>
>> My point to Jeff is that, aside from architectural aesthetics, direct calls from RPC consumers to the GSS layer causes a module dependency problem. The right way to plumb this is to create an RPC client API that invokes gssd_running() but only if auth_rpcgss.ko is loaded.

Chuck, I?ve already told you that the auth_rpcgss dependency is a non-starter. Turning off automatic loading of rpcsec_gss modules is a _worse_ regression than the ones we already have and (as I already said) can be trivially defeated by just compiling them into the kernel.

We _want_ the kernel to be able to autoload modules so that we can add new security flavours etc without having to recompile.

>> However, I don't see why the existing RPC client APIs shouldn't just fail where appropriate if GSSD isn't available. Is there a strong need to expose gssd_running() as a separate API?
>>
>
> One of the complaints about this whole "use krb5i by default" change is
> that we now get the warnings in the ring buffer when gssd isn't
> running. That's a good thing if krb5 was explicitly requested, but is
> useless and confusing if the user just wants to use AUTH_SYS.
>
> If we wait until gss_create, it's too late to know what the "intent"
> was. We'll either fire the warning inappropriately like we do now, or
> miss it altogether when we actually should have printed it.

What if the user intended to use krb5i, but the daemon failed to start? This whole ?kernel second guessing what the admin _actually_ wanted to do? thing is a red herring. Let?s just fix the real problem and then leave it at that.

--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com