On Wed, 13 Nov 2013 15:49:13 +0000
"Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote:
>
> On Nov 13, 2013, at 10:35, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> > On Wed, 13 Nov 2013 10:14:30 -0500
> > Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >
> >>
> >> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote:
> >>
> >>>
> >>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >>>
> >>>>
> >>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@xxxxxxxxxx> 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 <jlayton@xxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>> 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.
>
In that case, they will get a failure and warning when they go to the
next stage of the mount (I forget which RPC it is). With this change,
krb5/krb5i mounts will still fail just like they do today when gssd
isn't running. You just get a single warning in the ring buffer about
it instead of two.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
