| Message ID | 1384353053-30002-4-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> > --- > fs/nfs/client.c | 5 +++-- > fs/nfs/internal.h | 4 ++-- > fs/nfs/nfs4client.c | 8 ++++++-- > include/linux/nfs_xdr.h | 2 +- > include/linux/sunrpc/auth_gss.h | 10 ++++++++++ > net/sunrpc/auth_gss/auth_gss.c | 3 ++- > 6 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/fs/nfs/client.c b/fs/nfs/client.c > index 1d09289..fbee354 100644 > --- a/fs/nfs/client.c > +++ b/fs/nfs/client.c > @@ -501,7 +501,8 @@ nfs_get_client(const struct nfs_client_initdata *cl_init, > &nn->nfs_client_list); > spin_unlock(&nn->nfs_client_lock); > new->cl_flags = cl_init->init_flags; > - return rpc_ops->init_client(new, timeparms, ip_addr); > + return rpc_ops->init_client(new, timeparms, ip_addr, > + cl_init->net); > } > > spin_unlock(&nn->nfs_client_lock); > @@ -700,7 +701,7 @@ EXPORT_SYMBOL_GPL(nfs_init_server_rpcclient); > */ > struct nfs_client *nfs_init_client(struct nfs_client *clp, > const struct rpc_timeout *timeparms, > - const char *ip_addr) > + const char *ip_addr, struct net *net) > { > int error; > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h > index bca6a3e..69d3d1c 100644 > --- a/fs/nfs/internal.h > +++ b/fs/nfs/internal.h > @@ -273,7 +273,7 @@ extern struct rpc_procinfo nfs4_procedures[]; > void nfs_close_context(struct nfs_open_context *ctx, int is_sync); > extern struct nfs_client *nfs_init_client(struct nfs_client *clp, > const struct rpc_timeout *timeparms, > - const char *ip_addr); > + const char *ip_addr, struct net *net); > > /* dir.c */ > extern unsigned long nfs_access_cache_count(struct shrinker *shrink, > @@ -462,7 +462,7 @@ extern ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq); > extern void __nfs4_read_done_cb(struct nfs_read_data *); > extern 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); > extern int nfs40_walk_client_list(struct nfs_client *clp, > struct nfs_client **result, > struct rpc_cred *cred); > 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) > { > 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? > if (error < 0) > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > index 3ccfcec..e75b2cc 100644 > --- a/include/linux/nfs_xdr.h > +++ b/include/linux/nfs_xdr.h > @@ -1486,7 +1486,7 @@ struct nfs_rpc_ops { > struct nfs_client *(*alloc_client) (const struct nfs_client_initdata *); > struct nfs_client * > (*init_client) (struct nfs_client *, const struct rpc_timeout *, > - const char *); > + const char *, struct net *); > void (*free_client) (struct nfs_client *); > struct nfs_server *(*create_server)(struct nfs_mount_info *, struct nfs_subversion *); > struct nfs_server *(*clone_server)(struct nfs_server *, struct nfs_fh *, > diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h > index f1cfd4c..cecf684 100644 > --- a/include/linux/sunrpc/auth_gss.h > +++ b/include/linux/sunrpc/auth_gss.h > @@ -86,6 +86,16 @@ struct gss_cred { > unsigned long gc_upcall_timestamp; > }; > > +#if IS_ENABLED(CONFIG_SUNRPC_GSS) > +extern bool gssd_running(struct net *net); > +#else /* !CONFIG_SUNRPC_GSS */ > +static inline bool > +gssd_running(struct net *net) > +{ > + return false; > +} > +#endif /* CONFIG_SUNRPC_GSS */ > + > #endif /* __KERNEL__ */ > #endif /* _LINUX_SUNRPC_AUTH_GSS_H */ > > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c > index 399390e..0cc2174 100644 > --- a/net/sunrpc/auth_gss/auth_gss.c > +++ b/net/sunrpc/auth_gss/auth_gss.c > @@ -592,13 +592,14 @@ out: > return err; > } > > -static bool > +bool > gssd_running(struct net *net) > { > struct sunrpc_net *sn = net_generic(net, sunrpc_net_id); > > return rpc_pipe_is_open(sn->gssd_dummy); > } > +EXPORT_SYMBOL_GPL(gssd_running); > > static inline int > gss_create_upcall(struct gss_auth *gss_auth, struct gss_cred *gss_cred) > -- > 1.8.3.1 >
On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: > > On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> >> --- >> >> 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. Cheers Trond-- 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
On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > > On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: > >> >> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> >>> --- >>> >>> 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. 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?
On Wed, 13 Nov 2013 10:14:30 -0500 Chuck Lever <chuck.lever@oracle.com> wrote: > > On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > > > > > On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: > > > >> > >> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> > >>> --- > >>> > >>> 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. > > 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. So, that was the main reason for the layering violation here. I do however get your point on the module dependency, but IIUC...don't we get that anyway? Now that you try using krb5i by default for SETCLIENTID, don't we end up loading auth_gss.ko anyway on every nfsv4 mount regardless?
On Nov 13, 2013, at 10:35, Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 13 Nov 2013 10:14:30 -0500 > Chuck Lever <chuck.lever@oracle.com> wrote: > >> >> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: >> >>> >>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: >>> >>>> >>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> >>>>> --- >>>>> >>>>> 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 Trond.Myklebust@netapp.com www.netapp.com -- 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
On Wed, 13 Nov 2013 15:49:13 +0000 "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > > On Nov 13, 2013, at 10:35, Jeff Layton <jlayton@redhat.com> wrote: > > > On Wed, 13 Nov 2013 10:14:30 -0500 > > Chuck Lever <chuck.lever@oracle.com> wrote: > > > >> > >> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > >> > >>> > >>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: > >>> > >>>> > >>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> > >>>>> --- > >>>>> > >>>>> 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.
On Wed, 13 Nov 2013 10:57:02 -0500 Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 13 Nov 2013 15:49:13 +0000 > "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > > > > > On Nov 13, 2013, at 10:35, Jeff Layton <jlayton@redhat.com> wrote: > > > > > On Wed, 13 Nov 2013 10:14:30 -0500 > > > Chuck Lever <chuck.lever@oracle.com> wrote: > > > > > >> > > >> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > > >> > > >>> > > >>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: > > >>> > > >>>> > > >>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> > > >>>>> --- > > >>>>> > > >>>>> 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. > > Right, and just because auth_gss isn't currently plugged in, doesn't mean that it's not able to be plugged in. If this is the first mount attempt then it's likely that auth_gss isn't loaded yet, even if rpc.gssd is running. > > >> 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. > So to clarify...today we do this when gssd isn't running and we try an AUTH_GSS mount: - attempt SETCLIENTID with krb5i - when that fails, log a warning to ring buffer - attempt SETCLIENTID with AUTH_SYS - attempt rest of mount with krb5i - log another warning to ring buffer when it fails ...with the first two patches, that doesn't really change. With the third patch in place, we just skip the first two steps if gssd isn't running. You'll still get a single warning in the ring buffer (which is as expected). Trond, is that acceptable or do you want me to just drop the 3rd patch?
On Nov 13, 2013, at 11:09 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 13 Nov 2013 10:57:02 -0500 > Jeff Layton <jlayton@redhat.com> wrote: > >> On Wed, 13 Nov 2013 15:49:13 +0000 >> "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: >> >>> >>> On Nov 13, 2013, at 10:35, Jeff Layton <jlayton@redhat.com> wrote: >>> >>>> On Wed, 13 Nov 2013 10:14:30 -0500 >>>> Chuck Lever <chuck.lever@oracle.com> wrote: >>>> >>>>> >>>>> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: >>>>> >>>>>> >>>>>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: >>>>>> >>>>>>> >>>>>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> >>>>>>>> --- >>>>>>>> >>>>>>>> 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. >>> > > Right, and just because auth_gss isn't currently plugged in, doesn't > mean that it's not able to be plugged in. If this is the first mount > attempt then it's likely that auth_gss isn't loaded yet, even if > rpc.gssd is running. > >>>>> 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. >> > > So to clarify...today we do this when gssd isn't running and we try an > AUTH_GSS mount: > > - attempt SETCLIENTID with krb5i > - when that fails, log a warning to ring buffer > - attempt SETCLIENTID with AUTH_SYS > - attempt rest of mount with krb5i Hold it. This step should not be happening. Lease management should try krb5i by default, but why is the rest of the mount attempted with krb5i? > - log another warning to ring buffer when it fails > > ...with the first two patches, that doesn't really change. With the > third patch in place, we just skip the first two steps if gssd isn't > running. You'll still get a single warning in the ring buffer (which is > as expected). > > Trond, is that acceptable or do you want me to just drop the 3rd patch? NAK from me on the third patch as it stands. Find some other way to invoke gssd_running() if you really need to do that.
On Nov 13, 2013, at 10:49 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > > On Nov 13, 2013, at 10:35, Jeff Layton <jlayton@redhat.com> wrote: > >> On Wed, 13 Nov 2013 10:14:30 -0500 >> Chuck Lever <chuck.lever@oracle.com> wrote: >> >>> >>> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: >>> >>>> >>>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: >>>> >>>>> >>>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> >>>>>> --- >>>>>> >>>>>> 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. Maybe you thought that in your brain, but look at the e-mail archive: You actually haven't explicitly objected yet. You simply observed that this mechanism doesn't work for built-in scenarios, and I agree with that observation. It isn't supposed to work for built-in. But I don't think you understand my full proposal or my objection to Jeff's third patch. > 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. Automatic loading of auth_rpcgss never worked until 71afa85e (March 16). It simply isn't possible that we've depended on automatic loading of auth_rpcgss.ko in the past, because it has never worked until recently. > We _want_ the kernel to be able to autoload modules so that we can add new security flavours etc without having to recompile. I'm not proposing to turn off automatic loading of the GSS mechanisms. If auth_rpcgss.ko is loaded, the mechanisms get loaded automatically. > >>> 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. I agree that the kernel shouldn't guess. Autoloading auth_rpcgss is guessing what the admin wanted, IMO.
On Wed, 13 Nov 2013 11:10:52 -0500 Chuck Lever <chuck.lever@oracle.com> wrote: > > On Nov 13, 2013, at 11:09 AM, Jeff Layton <jlayton@redhat.com> wrote: > > > On Wed, 13 Nov 2013 10:57:02 -0500 > > Jeff Layton <jlayton@redhat.com> wrote: > > > >> On Wed, 13 Nov 2013 15:49:13 +0000 > >> "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > >> > >>> > >>> On Nov 13, 2013, at 10:35, Jeff Layton <jlayton@redhat.com> wrote: > >>> > >>>> On Wed, 13 Nov 2013 10:14:30 -0500 > >>>> Chuck Lever <chuck.lever@oracle.com> wrote: > >>>> > >>>>> > >>>>> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > >>>>> > >>>>>> > >>>>>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: > >>>>>> > >>>>>>> > >>>>>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> > >>>>>>>> --- > >>>>>>>> > >>>>>>>> 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. > >>> > > > > Right, and just because auth_gss isn't currently plugged in, doesn't > > mean that it's not able to be plugged in. If this is the first mount > > attempt then it's likely that auth_gss isn't loaded yet, even if > > rpc.gssd is running. > > > >>>>> 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. > >> > > > > So to clarify...today we do this when gssd isn't running and we try an > > AUTH_GSS mount: > > > > - attempt SETCLIENTID with krb5i > > - when that fails, log a warning to ring buffer > > - attempt SETCLIENTID with AUTH_SYS > > - attempt rest of mount with krb5i > > Hold it. This step should not be happening. Lease management should try krb5i by default, but why is the rest of the mount attempted with krb5i? > Sorry, I should have been more clear...the rest of the mount is attempted with krb5i because sec=krb5i was specified on the command line. IOW, this patch just shortcuts attempting to do the lease establishment with krb5i when we know that that will fail. The main benefit being that we don't end up logging a warning about AUTH_GSS not running in that case. The warning will be logged if/when a later call attempts to use GSSAPI. > > - log another warning to ring buffer when it fails > > > > ...with the first two patches, that doesn't really change. With the > > third patch in place, we just skip the first two steps if gssd isn't > > running. You'll still get a single warning in the ring buffer (which is > > as expected). > > > > Trond, is that acceptable or do you want me to just drop the 3rd patch? > > NAK from me on the third patch as it stands. Find some other way to invoke gssd_running() if you really need to do that. > Again, it's not clear to me why this is a concern. You're attempting to call auth_gss functions in order to do the SETCLIENTID call anyway, why is calling gssd_running prior to doing that a problem? In what situations will that break?
On Nov 13, 2013, at 10:35 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 13 Nov 2013 10:14:30 -0500 > Chuck Lever <chuck.lever@oracle.com> wrote: > >> >> On Nov 13, 2013, at 9:48 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: >> >>> >>> On Nov 13, 2013, at 9:38, Chuck Lever <chuck.lever@oracle.com> wrote: >>> >>>> >>>> On Nov 13, 2013, at 9:30 AM, Jeff Layton <jlayton@redhat.com> 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@redhat.com> >>>>> --- >>>>> >>>>> 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. >> >> 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. Right. If the kernel autoloads auth_rpcgss.ko, it has no way to know whether the administrator forgot to load it, or whether the administrator didn't load it because user space isn't configured. Thus we have to make the kernel more complicated to deal with both of those cases. We don't have another security flavor that has both a kernel and user space component. That's why I think auth_rpcgss.ko should be handled differently. That's off topic, though. > So, that was the main reason for the layering violation here. I do > however get your point on the module dependency, but IIUC...don't we get > that anyway? Strictly speaking, you can't depend on gssd_running() to be available until auth_rpcgss.ko is loaded. You are invoking gssd_running() before nfs_create_rpc_client() has had a chance to autoload auth_rpcgss.ko. Thus, the very first time through this path, gssd_running() may not yet be available. If you need this API in the NFS client, you have to wrap it with something that ensures auth_rpcgss.ko is loaded first, then invokes gssd_running(). See rpcauth_get_pseudoflavor() and rpcauth_get_gssinfo() for examples. Yucky, huh? > Now that you try using krb5i by default for SETCLIENTID, > don't we end up loading auth_gss.ko anyway on every nfsv4 mount > regardless? Yes we do, but that's the problem I'm trying to address elsewhere. (Note though, that the very first time through, even that doesn't protect you from calling a function that may not be loaded yet).
On Nov 13, 2013, at 11:20 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Wed, 13 Nov 2013 11:10:52 -0500 > Chuck Lever <chuck.lever@oracle.com> wrote: > >> >> On Nov 13, 2013, at 11:09 AM, Jeff Layton <jlayton@redhat.com> wrote: >>> >>> So to clarify...today we do this when gssd isn't running and we try an >>> AUTH_GSS mount: >>> >>> - attempt SETCLIENTID with krb5i >>> - when that fails, log a warning to ring buffer >>> - attempt SETCLIENTID with AUTH_SYS >>> - attempt rest of mount with krb5i >> >> Hold it. This step should not be happening. Lease management should try krb5i by default, but why is the rest of the mount attempted with krb5i? >> > > Sorry, I should have been more clear...the rest of the mount is > attempted with krb5i because sec=krb5i was specified on the command > line. > > IOW, this patch just shortcuts attempting to do the lease > establishment with krb5i when we know that that will fail. The main > benefit being that we don't end up logging a warning about AUTH_GSS not > running in that case. > > The warning will be logged if/when a later call attempts to use GSSAPI. Just a thought: The usual way we have of dealing with problems like this is WARN_ONCE() (or a user space equivalent).
diff --git a/fs/nfs/client.c b/fs/nfs/client.c index 1d09289..fbee354 100644 --- a/fs/nfs/client.c +++ b/fs/nfs/client.c @@ -501,7 +501,8 @@ nfs_get_client(const struct nfs_client_initdata *cl_init, &nn->nfs_client_list); spin_unlock(&nn->nfs_client_lock); new->cl_flags = cl_init->init_flags; - return rpc_ops->init_client(new, timeparms, ip_addr); + return rpc_ops->init_client(new, timeparms, ip_addr, + cl_init->net); } spin_unlock(&nn->nfs_client_lock); @@ -700,7 +701,7 @@ EXPORT_SYMBOL_GPL(nfs_init_server_rpcclient); */ struct nfs_client *nfs_init_client(struct nfs_client *clp, const struct rpc_timeout *timeparms, - const char *ip_addr) + const char *ip_addr, struct net *net) { int error; diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index bca6a3e..69d3d1c 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -273,7 +273,7 @@ extern struct rpc_procinfo nfs4_procedures[]; void nfs_close_context(struct nfs_open_context *ctx, int is_sync); extern struct nfs_client *nfs_init_client(struct nfs_client *clp, const struct rpc_timeout *timeparms, - const char *ip_addr); + const char *ip_addr, struct net *net); /* dir.c */ extern unsigned long nfs_access_cache_count(struct shrinker *shrink, @@ -462,7 +462,7 @@ extern ssize_t nfs_dreq_bytes_left(struct nfs_direct_req *dreq); extern void __nfs4_read_done_cb(struct nfs_read_data *); extern 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); extern int nfs40_walk_client_list(struct nfs_client *clp, struct nfs_client **result, struct rpc_cred *cred); 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) { 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); if (error < 0) diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 3ccfcec..e75b2cc 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -1486,7 +1486,7 @@ struct nfs_rpc_ops { struct nfs_client *(*alloc_client) (const struct nfs_client_initdata *); struct nfs_client * (*init_client) (struct nfs_client *, const struct rpc_timeout *, - const char *); + const char *, struct net *); void (*free_client) (struct nfs_client *); struct nfs_server *(*create_server)(struct nfs_mount_info *, struct nfs_subversion *); struct nfs_server *(*clone_server)(struct nfs_server *, struct nfs_fh *, diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h index f1cfd4c..cecf684 100644 --- a/include/linux/sunrpc/auth_gss.h +++ b/include/linux/sunrpc/auth_gss.h @@ -86,6 +86,16 @@ struct gss_cred { unsigned long gc_upcall_timestamp; }; +#if IS_ENABLED(CONFIG_SUNRPC_GSS) +extern bool gssd_running(struct net *net); +#else /* !CONFIG_SUNRPC_GSS */ +static inline bool +gssd_running(struct net *net) +{ + return false; +} +#endif /* CONFIG_SUNRPC_GSS */ + #endif /* __KERNEL__ */ #endif /* _LINUX_SUNRPC_AUTH_GSS_H */ diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c index 399390e..0cc2174 100644 --- a/net/sunrpc/auth_gss/auth_gss.c +++ b/net/sunrpc/auth_gss/auth_gss.c @@ -592,13 +592,14 @@ out: return err; } -static bool +bool gssd_running(struct net *net) { struct sunrpc_net *sn = net_generic(net, sunrpc_net_id); return rpc_pipe_is_open(sn->gssd_dummy); } +EXPORT_SYMBOL_GPL(gssd_running); static inline int gss_create_upcall(struct gss_auth *gss_auth, struct gss_cred *gss_cred)
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@redhat.com> --- fs/nfs/client.c | 5 +++-- fs/nfs/internal.h | 4 ++-- fs/nfs/nfs4client.c | 8 ++++++-- include/linux/nfs_xdr.h | 2 +- include/linux/sunrpc/auth_gss.h | 10 ++++++++++ net/sunrpc/auth_gss/auth_gss.c | 3 ++- 6 files changed, 24 insertions(+), 8 deletions(-)