| Message ID | 20240823181423.20458-5-snitzer@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs/nfsd: add support for localio | expand |
On Fri, Aug 23, 2024 at 02:14:02PM -0400, Mike Snitzer wrote: > From: NeilBrown <neilb@suse.de> > > __fh_verify() offers an interface like fh_verify() but doesn't require > a struct svc_rqst *, instead it also takes the specific parts as > explicit required arguments. So it is safe to call __fh_verify() with > a NULL rqstp, but the net, cred, and client args must not be NULL. > > __fh_verify() does not use SVC_NET(), nor does the functions it calls. > > Rather then depending on rqstp->rq_vers to determine nfs version, pass > it in explicitly. This removes another dependency on rqstp and ensures > the correct version is checked. The rqstp can be for an NLM request and > while some code tests that, other code does not. > > Rather than using rqstp->rq_client pass the client and gssclient > explicitly to __fh_verify and then to nfsd_set_fh_dentry(). > > The final places where __fh_verify unconditionally dereferences rqstp > involve checking if the connection is suitably secure. They look at > rqstp->rq_xprt which is not meaningful in the target use case of > "localio" NFS in which the client talks directly to the local server. > So have these always succeed when rqstp is NULL. > > Lastly, 4 associated tracepoints are only used if rqstp is not NULL > (this is a stop-gap that should be properly fixed so localio also > benefits from the utility these tracepoints provide when debugging > fh_verify issues). > > Signed-off-by: NeilBrown <neilb@suse.de> > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > Signed-off-by: Mike Snitzer <snitzer@kernel.org> IMO this patch needs to be split up. There are several changes that need separate explanation and rationale, and the changes here need to be individually bisectable. If you prefer, I can provide a patch series that replaces this one patch, or Neil could provide it if he wants. A few more specific comments below. > --- > fs/nfsd/export.c | 8 ++- > fs/nfsd/nfsfh.c | 124 ++++++++++++++++++++++++++++------------------- > 2 files changed, 82 insertions(+), 50 deletions(-) > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > index 7bb4f2075ac5..fe36f441d1d9 100644 > --- a/fs/nfsd/export.c > +++ b/fs/nfsd/export.c > @@ -1077,7 +1077,13 @@ static struct svc_export *exp_find(struct cache_detail *cd, > __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) > { > struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors; > - struct svc_xprt *xprt = rqstp->rq_xprt; > + struct svc_xprt *xprt; > + > + if (!rqstp) > + /* Always allow LOCALIO */ > + return 0; > + > + xprt = rqstp->rq_xprt; check_nfsd_access() is a public API, so it needs a kdoc comment. These changes should be split into a separate patch with a clear rationale of why “Always allow LOCALIO” is secure and appropriate to do. > if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) { > if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags)) > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index 50d23d56f403..19e173187ab9 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -87,23 +87,24 @@ nfsd_mode_check(struct dentry *dentry, umode_t requested) > return nfserr_wrong_type; > } > > -static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, int flags) > +static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, > + struct svc_cred *cred, > + struct svc_export *exp) > { > - if (flags & NFSEXP_INSECURE_PORT) > + if (nfsexp_flags(cred, exp) & NFSEXP_INSECURE_PORT) > return true; > /* We don't require gss requests to use low ports: */ > - if (rqstp->rq_cred.cr_flavor >= RPC_AUTH_GSS) > + if (cred->cr_flavor >= RPC_AUTH_GSS) > return true; > return test_bit(RQ_SECURE, &rqstp->rq_flags); > } > > static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp, > + struct svc_cred *cred, > struct svc_export *exp) > { > - int flags = nfsexp_flags(&rqstp->rq_cred, exp); > - > /* Check if the request originated from a secure port. */ > - if (!nfsd_originating_port_ok(rqstp, flags)) { > + if (rqstp && !nfsd_originating_port_ok(rqstp, cred, exp)) { > RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]); > dprintk("nfsd: request from insecure port %s!\n", > svc_print_addr(rqstp, buf, sizeof(buf))); > @@ -111,7 +112,7 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp, > } > > /* Set user creds for this exportpoint */ > - return nfserrno(nfsd_setuser(&rqstp->rq_cred, exp)); > + return nfserrno(nfsd_setuser(cred, exp)); > } Just for cleanliness, the above hunks could be in their own patch, labeled as a refactoring change. > > static inline __be32 check_pseudo_root(struct dentry *dentry, > @@ -141,7 +142,11 @@ static inline __be32 check_pseudo_root(struct dentry *dentry, > * dentry. On success, the results are used to set fh_export and > * fh_dentry. > */ > -static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > +static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, > + struct svc_cred *cred, int nfs_vers, > + struct auth_domain *client, > + struct auth_domain *gssclient, > + struct svc_fh *fhp) > { > struct knfsd_fh *fh = &fhp->fh_handle; > struct fid *fid = NULL; > @@ -183,14 +188,15 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > data_left -= len; > if (data_left < 0) > return error; > - exp = rqst_exp_find(&rqstp->rq_chandle, SVC_NET(rqstp), > - rqstp->rq_client, rqstp->rq_gssclient, > + exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL, > + net, client, gssclient, > fh->fh_fsid_type, fh->fh_fsid); Question: Would rqst_exp_find() be the function that would prevent a LOCALIO open to a file handle where the client's IP address is not listed on the export? I don't really see how IP address-related export access control is being enforced, but it's possible I'm missing something. > fid = (struct fid *)(fh->fh_fsid + len); > > error = nfserr_stale; > if (IS_ERR(exp)) { > - trace_nfsd_set_fh_dentry_badexport(rqstp, fhp, PTR_ERR(exp)); > + if (rqstp) > + trace_nfsd_set_fh_dentry_badexport(rqstp, fhp, PTR_ERR(exp)); > > if (PTR_ERR(exp) == -ENOENT) > return error; > @@ -219,7 +225,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > put_cred(override_creds(new)); > put_cred(new); > } else { > - error = nfsd_setuser_and_check_port(rqstp, exp); > + error = nfsd_setuser_and_check_port(rqstp, cred, exp); > if (error) > goto out; > } > @@ -238,7 +244,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > data_left, fileid_type, 0, > nfsd_acceptable, exp); > if (IS_ERR_OR_NULL(dentry)) { > - trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp, > + if (rqstp) > + trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp, > dentry ? PTR_ERR(dentry) : -ESTALE); > switch (PTR_ERR(dentry)) { > case -ENOMEM: > @@ -266,7 +273,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > fhp->fh_dentry = dentry; > fhp->fh_export = exp; > > - switch (rqstp->rq_vers) { > + switch (nfs_vers) { > case 4: > if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR) > fhp->fh_no_atomic_attr = true; @nfs_vers is actually the new FH’s NFS version; that needs to be better documented because this is NFS version-specific code and I guess it needs to stay in here since it parametrizes the form of the new file handle. Perhaps these cases need to be wrapped with ifdefs. IMO all callers should supply this value, rather than manufacturing it in fh_verify(). Ie, "I want an NFSv3 file handle, please." That should be a separate patch, it will be somewhat less than a surgical change. > @@ -293,50 +300,29 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > return error; > } > > -/** > - * fh_verify - filehandle lookup and access checking > - * @rqstp: pointer to current rpc request > - * @fhp: filehandle to be verified > - * @type: expected type of object pointed to by filehandle > - * @access: type of access needed to object > - * > - * Look up a dentry from the on-the-wire filehandle, check the client's > - * access to the export, and set the current task's credentials. > - * > - * Regardless of success or failure of fh_verify(), fh_put() should be > - * called on @fhp when the caller is finished with the filehandle. > - * > - * fh_verify() may be called multiple times on a given filehandle, for > - * example, when processing an NFSv4 compound. The first call will look > - * up a dentry using the on-the-wire filehandle. Subsequent calls will > - * skip the lookup and just perform the other checks and possibly change > - * the current task's credentials. > - * > - * @type specifies the type of object expected using one of the S_IF* > - * constants defined in include/linux/stat.h. The caller may use zero > - * to indicate that it doesn't care, or a negative integer to indicate > - * that it expects something not of the given type. > - * > - * @access is formed from the NFSD_MAY_* constants defined in > - * fs/nfsd/vfs.h. > - */ See comment on 5/N: since that patch makes this a public API again, consider not removing this kdoc comment but rather updating it. > -__be32 > -fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > +static __be32 > +__fh_verify(struct svc_rqst *rqstp, > + struct net *net, struct svc_cred *cred, > + int nfs_vers, struct auth_domain *client, > + struct auth_domain *gssclient, > + struct svc_fh *fhp, umode_t type, int access) > { > - struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); > + struct nfsd_net *nn = net_generic(net, nfsd_net_id); > struct svc_export *exp = NULL; > struct dentry *dentry; > __be32 error; > > if (!fhp->fh_dentry) { > - error = nfsd_set_fh_dentry(rqstp, fhp); > + error = nfsd_set_fh_dentry(rqstp, net, cred, nfs_vers, > + client, gssclient, fhp); > if (error) > goto out; > } > dentry = fhp->fh_dentry; > exp = fhp->fh_export; > > - trace_nfsd_fh_verify(rqstp, fhp, type, access); > + if (rqstp) > + trace_nfsd_fh_verify(rqstp, fhp, type, access); > > /* > * We still have to do all these permission checks, even when > @@ -358,7 +344,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > if (error) > goto out; > > - error = nfsd_setuser_and_check_port(rqstp, exp); > + error = nfsd_setuser_and_check_port(rqstp, cred, exp); > if (error) > goto out; > > @@ -388,14 +374,54 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > > skip_pseudoflavor_check: > /* Finally, check access permissions. */ > - error = nfsd_permission(&rqstp->rq_cred, exp, dentry, access); > + error = nfsd_permission(cred, exp, dentry, access); > out: > - trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error); > + if (rqstp) > + trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error); > if (error == nfserr_stale) > nfsd_stats_fh_stale_inc(nn, exp); > return error; > } > > +/** > + * fh_verify - filehandle lookup and access checking > + * @rqstp: pointer to current rpc request > + * @fhp: filehandle to be verified > + * @type: expected type of object pointed to by filehandle > + * @access: type of access needed to object > + * > + * Look up a dentry from the on-the-wire filehandle, check the client's > + * access to the export, and set the current task's credentials. > + * > + * Regardless of success or failure of fh_verify(), fh_put() should be > + * called on @fhp when the caller is finished with the filehandle. > + * > + * fh_verify() may be called multiple times on a given filehandle, for > + * example, when processing an NFSv4 compound. The first call will look > + * up a dentry using the on-the-wire filehandle. Subsequent calls will > + * skip the lookup and just perform the other checks and possibly change > + * the current task's credentials. > + * > + * @type specifies the type of object expected using one of the S_IF* > + * constants defined in include/linux/stat.h. The caller may use zero > + * to indicate that it doesn't care, or a negative integer to indicate > + * that it expects something not of the given type. > + * > + * @access is formed from the NFSD_MAY_* constants defined in > + * fs/nfsd/vfs.h. > + */ > +__be32 > +fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > +{ > + int nfs_vers; > + if (rqstp->rq_prog == NFS_PROGRAM) > + nfs_vers = rqstp->rq_vers; > + else /* must be NLM */ > + nfs_vers = rqstp->rq_vers == 4 ? 3 : 2; > + return __fh_verify(rqstp, SVC_NET(rqstp), &rqstp->rq_cred, nfs_vers, > + rqstp->rq_client, rqstp->rq_gssclient, > + fhp, type, access); > +} > > /* > * Compose a file handle for an NFS reply. > -- > 2.44.0 >
On Mon, 26 Aug 2024, Chuck Lever wrote: > On Fri, Aug 23, 2024 at 02:14:02PM -0400, Mike Snitzer wrote: > > + exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL, > > + net, client, gssclient, > > fh->fh_fsid_type, fh->fh_fsid); > > Question: Would rqst_exp_find() be the function that would prevent > a LOCALIO open to a file handle where the client's IP address is not > listed on the export? Yes. > > I don't really see how IP address-related export access control is > being enforced, but it's possible I'm missing something. The "client" is key. The LOCALIO RPC protocol allows the server to determine a "client" which matches the network connection. It passes this to the client code which uses it for future authentication. > See comment on 5/N: since that patch makes this a public API again, > consider not removing this kdoc comment but rather updating it. What exactly do you consider to be a "public API"?? Anything without "static"? That seems somewhat arbitrary. I think of __fh_verify() as a private API used by fh_verify() and nfsd_file_acquire_local() and nothing else. It seems pointless duplication the documentation for __fh_verify() and fh_verify(). Maybe one could refer to the other "fh_verify is like fh_verify except ....." ?? > > > > -__be32 > > -fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > > +static __be32 > > +__fh_verify(struct svc_rqst *rqstp, Thanks, NeilBrown
On Mon, Aug 26, 2024 at 09:44:48AM +1000, NeilBrown wrote: > On Mon, 26 Aug 2024, Chuck Lever wrote: > > See comment on 5/N: since that patch makes this a public API again, > > consider not removing this kdoc comment but rather updating it. > > What exactly do you consider to be a "public API"?? Anything without > "static"? That seems somewhat arbitrary. Anything that is invoked from another, distinct, area of code presents an API boundary. > I think of __fh_verify() as a private API used by fh_verify() and > nfsd_file_acquire_local() and nothing else. That is indeed the current situation. But practically speaking, there's nothing that C can do to prevent other call sites from appearing over time. > It seems pointless duplication the documentation for __fh_verify() and > fh_verify(). We have that duplication already for the family of nfsd_file_acquire APIs, for example. These two, however, take somewhat different parameters, and somewhere (in the code) you have to document things like "this one expects a non-NULL @rqstp". Some API documentation, even for "private" APIs, is valuable. > Maybe one could refer to the other "fh_verify is like > fh_verify except ....." Highlighting the differences is certainly helpful for human readers. Perhaps it doesn't have to be a kdoc-style comment, but that's generally the kind of comment a developer would expect to explain how and when to invoke a function.
On Sun, Aug 25, 2024 at 11:32:50AM -0400, Chuck Lever wrote: > On Fri, Aug 23, 2024 at 02:14:02PM -0400, Mike Snitzer wrote: > > From: NeilBrown <neilb@suse.de> > > > > __fh_verify() offers an interface like fh_verify() but doesn't require > > a struct svc_rqst *, instead it also takes the specific parts as > > explicit required arguments. So it is safe to call __fh_verify() with > > a NULL rqstp, but the net, cred, and client args must not be NULL. > > > > __fh_verify() does not use SVC_NET(), nor does the functions it calls. > > > > Rather then depending on rqstp->rq_vers to determine nfs version, pass > > it in explicitly. This removes another dependency on rqstp and ensures > > the correct version is checked. The rqstp can be for an NLM request and > > while some code tests that, other code does not. > > > > Rather than using rqstp->rq_client pass the client and gssclient > > explicitly to __fh_verify and then to nfsd_set_fh_dentry(). > > > > The final places where __fh_verify unconditionally dereferences rqstp > > involve checking if the connection is suitably secure. They look at > > rqstp->rq_xprt which is not meaningful in the target use case of > > "localio" NFS in which the client talks directly to the local server. > > So have these always succeed when rqstp is NULL. > > > > Lastly, 4 associated tracepoints are only used if rqstp is not NULL > > (this is a stop-gap that should be properly fixed so localio also > > benefits from the utility these tracepoints provide when debugging > > fh_verify issues). > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > > IMO this patch needs to be split up. There are several changes that > need separate explanation and rationale, and the changes here need > to be individually bisectable. > > If you prefer, I can provide a patch series that replaces this one > patch, or Neil could provide it if he wants. I'm probably not the best person to take a crack at splitting this patch up. Neil initially had factored out N patches, but they weren't fully baked so when I had to fix the code up it became a challenge to sprinkle fixes across the N patches. Because they were all pretty interdependent. In the end, all these changes are in service to allowing for the possibility that the rqstp not available (NULL). So it made sense to fold them together. I really don't see how factoring these changes out to N patches makes them useful for bisection (you need all of them to test the case when rqstp is NULL, and a partial application of changes to track down a rqstp-full regression really isn't such a concern given fh_verify() fully passes all args to __fh_verify so status-quo preserved). All said, if your intuition and experience makes you feel splitting this patch up is needed then I'm fine with it and I welcome your or Neil's contribtion. It is fiddley work though, so having had my own challenges with the code when these changes were split out makes me hesitant to jump on splitting them out again. Hope I've explained myself clearly... not being confrontational, dismissive or anything else. ;) > A few more specific comments below. > > > > --- > > fs/nfsd/export.c | 8 ++- > > fs/nfsd/nfsfh.c | 124 ++++++++++++++++++++++++++++------------------- > > 2 files changed, 82 insertions(+), 50 deletions(-) > > > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > > index 7bb4f2075ac5..fe36f441d1d9 100644 > > --- a/fs/nfsd/export.c > > +++ b/fs/nfsd/export.c > > @@ -1077,7 +1077,13 @@ static struct svc_export *exp_find(struct cache_detail *cd, > > __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) > > { > > struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors; > > - struct svc_xprt *xprt = rqstp->rq_xprt; > > + struct svc_xprt *xprt; > > + > > + if (!rqstp) > > + /* Always allow LOCALIO */ > > + return 0; > > + > > + xprt = rqstp->rq_xprt; > > check_nfsd_access() is a public API, so it needs a kdoc comment. > > These changes should be split into a separate patch with a clear > rationale of why "Always allow LOCALIO" is secure and appropriate > to do. Separate patch aside, I'll try to improve that comment. Thanks, Mike
On Tue, Aug 27, 2024 at 02:58:21PM -0400, Mike Snitzer wrote: > On Sun, Aug 25, 2024 at 11:32:50AM -0400, Chuck Lever wrote: > > On Fri, Aug 23, 2024 at 02:14:02PM -0400, Mike Snitzer wrote: > > > From: NeilBrown <neilb@suse.de> > > > > > > __fh_verify() offers an interface like fh_verify() but doesn't require > > > a struct svc_rqst *, instead it also takes the specific parts as > > > explicit required arguments. So it is safe to call __fh_verify() with > > > a NULL rqstp, but the net, cred, and client args must not be NULL. > > > > > > __fh_verify() does not use SVC_NET(), nor does the functions it calls. > > > > > > Rather then depending on rqstp->rq_vers to determine nfs version, pass > > > it in explicitly. This removes another dependency on rqstp and ensures > > > the correct version is checked. The rqstp can be for an NLM request and > > > while some code tests that, other code does not. > > > > > > Rather than using rqstp->rq_client pass the client and gssclient > > > explicitly to __fh_verify and then to nfsd_set_fh_dentry(). > > > > > > The final places where __fh_verify unconditionally dereferences rqstp > > > involve checking if the connection is suitably secure. They look at > > > rqstp->rq_xprt which is not meaningful in the target use case of > > > "localio" NFS in which the client talks directly to the local server. > > > So have these always succeed when rqstp is NULL. > > > > > > Lastly, 4 associated tracepoints are only used if rqstp is not NULL > > > (this is a stop-gap that should be properly fixed so localio also > > > benefits from the utility these tracepoints provide when debugging > > > fh_verify issues). > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > > > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > > > > IMO this patch needs to be split up. There are several changes that > > need separate explanation and rationale, and the changes here need > > to be individually bisectable. > > > > If you prefer, I can provide a patch series that replaces this one > > patch, or Neil could provide it if he wants. > > I'm probably not the best person to take a crack at splitting this > patch up. > > Neil initially had factored out N patches, but they weren't fully > baked so when I had to fix the code up it became a challenge to > sprinkle fixes across the N patches. Because they were all > pretty interdependent. > > In the end, all these changes are in service to allowing for the > possibility that the rqstp not available (NULL). So it made sense to > fold them together. > > I really don't see how factoring these changes out to N patches makes > them useful for bisection (you need all of them to test the case when > rqstp is NULL, and a partial application of changes to track down a > rqstp-full regression really isn't such a concern given fh_verify() > fully passes all args to __fh_verify so status-quo preserved). > > All said, if your intuition and experience makes you feel splitting > this patch up is needed then I'm fine with it and I welcome your or > Neil's contribtion. It is fiddley work though, so having had my own > challenges with the code when these changes were split out makes me > hesitant to jump on splitting them out again. > > Hope I've explained myself clearly... not being confrontational, > dismissive or anything else. ;) True, this isn't an enormous single patch, but you gotta draw that line somewhere. The goal is to make these changes as small and atomic as possible so it becomes easy to spot a mistake or bisect to find unintended behavior introduced in the non-LOCALIO case. This is a code path that is heavily utilized by NFSD so it pays to take some defensive precautions. Generally I find that a typo or hidden assumption magically appears when I've engaged in this kind of refactoring exercise. I've got a good toolchain that should make this quick work. > > > --- a/fs/nfsd/export.c > > > +++ b/fs/nfsd/export.c > > > @@ -1077,7 +1077,13 @@ static struct svc_export *exp_find(struct cache_detail *cd, > > > __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) > > > { > > > struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors; > > > - struct svc_xprt *xprt = rqstp->rq_xprt; > > > + struct svc_xprt *xprt; > > > + > > > + if (!rqstp) > > > + /* Always allow LOCALIO */ > > > + return 0; > > > + > > > + xprt = rqstp->rq_xprt; > > > > check_nfsd_access() is a public API, so it needs a kdoc comment. > > > > These changes should be split into a separate patch with a clear > > rationale of why "Always allow LOCALIO" is secure and appropriate > > to do. > > Separate patch aside, I'll try to improve that comment. Post the comment update here and I can work that in.
On Tue, Aug 27, 2024 at 03:26:36PM -0400, Chuck Lever wrote: > On Tue, Aug 27, 2024 at 02:58:21PM -0400, Mike Snitzer wrote: > > On Sun, Aug 25, 2024 at 11:32:50AM -0400, Chuck Lever wrote: > > > On Fri, Aug 23, 2024 at 02:14:02PM -0400, Mike Snitzer wrote: > > > > From: NeilBrown <neilb@suse.de> > > > > > > > > __fh_verify() offers an interface like fh_verify() but doesn't require > > > > a struct svc_rqst *, instead it also takes the specific parts as > > > > explicit required arguments. So it is safe to call __fh_verify() with > > > > a NULL rqstp, but the net, cred, and client args must not be NULL. > > > > > > > > __fh_verify() does not use SVC_NET(), nor does the functions it calls. > > > > > > > > Rather then depending on rqstp->rq_vers to determine nfs version, pass > > > > it in explicitly. This removes another dependency on rqstp and ensures > > > > the correct version is checked. The rqstp can be for an NLM request and > > > > while some code tests that, other code does not. > > > > > > > > Rather than using rqstp->rq_client pass the client and gssclient > > > > explicitly to __fh_verify and then to nfsd_set_fh_dentry(). > > > > > > > > The final places where __fh_verify unconditionally dereferences rqstp > > > > involve checking if the connection is suitably secure. They look at > > > > rqstp->rq_xprt which is not meaningful in the target use case of > > > > "localio" NFS in which the client talks directly to the local server. > > > > So have these always succeed when rqstp is NULL. > > > > > > > > Lastly, 4 associated tracepoints are only used if rqstp is not NULL > > > > (this is a stop-gap that should be properly fixed so localio also > > > > benefits from the utility these tracepoints provide when debugging > > > > fh_verify issues). > > > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > > > > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > > > > > > IMO this patch needs to be split up. There are several changes that > > > need separate explanation and rationale, and the changes here need > > > to be individually bisectable. > > > > > > If you prefer, I can provide a patch series that replaces this one > > > patch, or Neil could provide it if he wants. > > > > I'm probably not the best person to take a crack at splitting this > > patch up. > > > > Neil initially had factored out N patches, but they weren't fully > > baked so when I had to fix the code up it became a challenge to > > sprinkle fixes across the N patches. Because they were all > > pretty interdependent. > > > > In the end, all these changes are in service to allowing for the > > possibility that the rqstp not available (NULL). So it made sense to > > fold them together. > > > > I really don't see how factoring these changes out to N patches makes > > them useful for bisection (you need all of them to test the case when > > rqstp is NULL, and a partial application of changes to track down a > > rqstp-full regression really isn't such a concern given fh_verify() > > fully passes all args to __fh_verify so status-quo preserved). > > > > All said, if your intuition and experience makes you feel splitting > > this patch up is needed then I'm fine with it and I welcome your or > > Neil's contribtion. It is fiddley work though, so having had my own > > challenges with the code when these changes were split out makes me > > hesitant to jump on splitting them out again. > > > > Hope I've explained myself clearly... not being confrontational, > > dismissive or anything else. ;) > > True, this isn't an enormous single patch, but you gotta draw that > line somewhere. > > The goal is to make these changes as small and atomic as possible so > it becomes easy to spot a mistake or bisect to find unintended > behavior introduced in the non-LOCALIO case. This is a code path > that is heavily utilized by NFSD so it pays to take some defensive > precautions. > > Generally I find that a typo or hidden assumption magically appears > when I've engaged in this kind of refactoring exercise. I've got a > good toolchain that should make this quick work. Awesome. > > > > --- a/fs/nfsd/export.c > > > > +++ b/fs/nfsd/export.c > > > > @@ -1077,7 +1077,13 @@ static struct svc_export *exp_find(struct cache_detail *cd, > > > > __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) > > > > { > > > > struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors; > > > > - struct svc_xprt *xprt = rqstp->rq_xprt; > > > > + struct svc_xprt *xprt; > > > > + > > > > + if (!rqstp) > > > > + /* Always allow LOCALIO */ > > > > + return 0; > > > > + > > > > + xprt = rqstp->rq_xprt; > > > > > > check_nfsd_access() is a public API, so it needs a kdoc comment. > > > > > > These changes should be split into a separate patch with a clear > > > rationale of why "Always allow LOCALIO" is secure and appropriate > > > to do. > > > > Separate patch aside, I'll try to improve that comment. > > Post the comment update here and I can work that in. Here is the incremental patch: diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c index fe36f441d1d9..8b54f66f0e0f 100644 --- a/fs/nfsd/export.c +++ b/fs/nfsd/export.c @@ -1074,14 +1074,26 @@ static struct svc_export *exp_find(struct cache_detail *cd, return exp; } +/** + * check_nfsd_access - check if access to export is allowed. + * @exp: svc_export that is being accessed. + * @rqstp: svc_rqst attempting to access @exp (will be NULL for LOCALIO). + * + * Returns 0 if access is granted or nfserr_wrongsec if access is denied. + */ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) { struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors; struct svc_xprt *xprt; - if (!rqstp) - /* Always allow LOCALIO */ + if (!rqstp) { + /* + * The target use case for rqstp being NULL is LOCALIO, + * which only supports AUTH_UNIX, so always allow LOCALIO + * because the other checks below aren't applicable. + */ return 0; + } xprt = rqstp->rq_xprt;
On Mon, Aug 26, 2024 at 09:44:48AM +1000, NeilBrown wrote: > On Mon, 26 Aug 2024, Chuck Lever wrote: > > See comment on 5/N: since that patch makes this a public API again, > > consider not removing this kdoc comment but rather updating it. > > What exactly do you consider to be a "public API"?? Anything without > "static"? That seems somewhat arbitrary. > > I think of __fh_verify() as a private API used by fh_verify() and > nfsd_file_acquire_local() and nothing else. > > It seems pointless duplication the documentation for __fh_verify() and > fh_verify(). Maybe one could refer to the other "fh_verify is like > fh_verify except ....." > > ?? > > > > > > > > -__be32 > > > -fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > > > +static __be32 > > > +__fh_verify(struct svc_rqst *rqstp, An alternative would be to leave __fh_verify() as a static, and then add an fh_verify_local() API, echoing nfsd_file_acquire_local(), and then give that API a kdoc comment. That would make it clear who the intended consumer is.
On Wed, Aug 28, 2024 at 01:01:01PM -0400, Chuck Lever wrote: > On Mon, Aug 26, 2024 at 09:44:48AM +1000, NeilBrown wrote: > > On Mon, 26 Aug 2024, Chuck Lever wrote: > > > See comment on 5/N: since that patch makes this a public API again, > > > consider not removing this kdoc comment but rather updating it. > > > > What exactly do you consider to be a "public API"?? Anything without > > "static"? That seems somewhat arbitrary. > > > > I think of __fh_verify() as a private API used by fh_verify() and > > nfsd_file_acquire_local() and nothing else. > > > > It seems pointless duplication the documentation for __fh_verify() and > > fh_verify(). Maybe one could refer to the other "fh_verify is like > > fh_verify except ....." > > > > ?? > > > > > > > > > > > > -__be32 > > > > -fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > > > > +static __be32 > > > > +__fh_verify(struct svc_rqst *rqstp, > > An alternative would be to leave __fh_verify() as a static, and then > add an fh_verify_local() API, echoing nfsd_file_acquire_local(), and > then give that API a kdoc comment. > > That would make it clear who the intended consumer is. I ran with this idea, definite improvement, you'll find the changes folded in to the relevant patch (with the same subject) in v14. Thanks, Mike
On Wed, Aug 28, 2024 at 08:30:11PM -0400, Mike Snitzer wrote: > On Wed, Aug 28, 2024 at 01:01:01PM -0400, Chuck Lever wrote: > > On Mon, Aug 26, 2024 at 09:44:48AM +1000, NeilBrown wrote: > > > On Mon, 26 Aug 2024, Chuck Lever wrote: > > > > See comment on 5/N: since that patch makes this a public API again, > > > > consider not removing this kdoc comment but rather updating it. > > > > > > What exactly do you consider to be a "public API"?? Anything without > > > "static"? That seems somewhat arbitrary. > > > > > > I think of __fh_verify() as a private API used by fh_verify() and > > > nfsd_file_acquire_local() and nothing else. > > > > > > It seems pointless duplication the documentation for __fh_verify() and > > > fh_verify(). Maybe one could refer to the other "fh_verify is like > > > fh_verify except ....." > > > > > > ?? > > > > > > > > > > > > > > > > -__be32 > > > > > -fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) > > > > > +static __be32 > > > > > +__fh_verify(struct svc_rqst *rqstp, > > > > An alternative would be to leave __fh_verify() as a static, and then > > add an fh_verify_local() API, echoing nfsd_file_acquire_local(), and > > then give that API a kdoc comment. > > > > That would make it clear who the intended consumer is. > > I ran with this idea, definite improvement, you'll find the changes > folded in to the relevant patch (with the same subject) in v14. Err, sorry, in v14 it'll be patch 9 with subject: "nfsd: add nfsd_file_acquire_local()"
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c index 7bb4f2075ac5..fe36f441d1d9 100644 --- a/fs/nfsd/export.c +++ b/fs/nfsd/export.c @@ -1077,7 +1077,13 @@ static struct svc_export *exp_find(struct cache_detail *cd, __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) { struct exp_flavor_info *f, *end = exp->ex_flavors + exp->ex_nflavors; - struct svc_xprt *xprt = rqstp->rq_xprt; + struct svc_xprt *xprt; + + if (!rqstp) + /* Always allow LOCALIO */ + return 0; + + xprt = rqstp->rq_xprt; if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) { if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags)) diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index 50d23d56f403..19e173187ab9 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -87,23 +87,24 @@ nfsd_mode_check(struct dentry *dentry, umode_t requested) return nfserr_wrong_type; } -static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, int flags) +static bool nfsd_originating_port_ok(struct svc_rqst *rqstp, + struct svc_cred *cred, + struct svc_export *exp) { - if (flags & NFSEXP_INSECURE_PORT) + if (nfsexp_flags(cred, exp) & NFSEXP_INSECURE_PORT) return true; /* We don't require gss requests to use low ports: */ - if (rqstp->rq_cred.cr_flavor >= RPC_AUTH_GSS) + if (cred->cr_flavor >= RPC_AUTH_GSS) return true; return test_bit(RQ_SECURE, &rqstp->rq_flags); } static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp, + struct svc_cred *cred, struct svc_export *exp) { - int flags = nfsexp_flags(&rqstp->rq_cred, exp); - /* Check if the request originated from a secure port. */ - if (!nfsd_originating_port_ok(rqstp, flags)) { + if (rqstp && !nfsd_originating_port_ok(rqstp, cred, exp)) { RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]); dprintk("nfsd: request from insecure port %s!\n", svc_print_addr(rqstp, buf, sizeof(buf))); @@ -111,7 +112,7 @@ static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp, } /* Set user creds for this exportpoint */ - return nfserrno(nfsd_setuser(&rqstp->rq_cred, exp)); + return nfserrno(nfsd_setuser(cred, exp)); } static inline __be32 check_pseudo_root(struct dentry *dentry, @@ -141,7 +142,11 @@ static inline __be32 check_pseudo_root(struct dentry *dentry, * dentry. On success, the results are used to set fh_export and * fh_dentry. */ -static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) +static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, + struct svc_cred *cred, int nfs_vers, + struct auth_domain *client, + struct auth_domain *gssclient, + struct svc_fh *fhp) { struct knfsd_fh *fh = &fhp->fh_handle; struct fid *fid = NULL; @@ -183,14 +188,15 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) data_left -= len; if (data_left < 0) return error; - exp = rqst_exp_find(&rqstp->rq_chandle, SVC_NET(rqstp), - rqstp->rq_client, rqstp->rq_gssclient, + exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL, + net, client, gssclient, fh->fh_fsid_type, fh->fh_fsid); fid = (struct fid *)(fh->fh_fsid + len); error = nfserr_stale; if (IS_ERR(exp)) { - trace_nfsd_set_fh_dentry_badexport(rqstp, fhp, PTR_ERR(exp)); + if (rqstp) + trace_nfsd_set_fh_dentry_badexport(rqstp, fhp, PTR_ERR(exp)); if (PTR_ERR(exp) == -ENOENT) return error; @@ -219,7 +225,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) put_cred(override_creds(new)); put_cred(new); } else { - error = nfsd_setuser_and_check_port(rqstp, exp); + error = nfsd_setuser_and_check_port(rqstp, cred, exp); if (error) goto out; } @@ -238,7 +244,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) data_left, fileid_type, 0, nfsd_acceptable, exp); if (IS_ERR_OR_NULL(dentry)) { - trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp, + if (rqstp) + trace_nfsd_set_fh_dentry_badhandle(rqstp, fhp, dentry ? PTR_ERR(dentry) : -ESTALE); switch (PTR_ERR(dentry)) { case -ENOMEM: @@ -266,7 +273,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) fhp->fh_dentry = dentry; fhp->fh_export = exp; - switch (rqstp->rq_vers) { + switch (nfs_vers) { case 4: if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR) fhp->fh_no_atomic_attr = true; @@ -293,50 +300,29 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) return error; } -/** - * fh_verify - filehandle lookup and access checking - * @rqstp: pointer to current rpc request - * @fhp: filehandle to be verified - * @type: expected type of object pointed to by filehandle - * @access: type of access needed to object - * - * Look up a dentry from the on-the-wire filehandle, check the client's - * access to the export, and set the current task's credentials. - * - * Regardless of success or failure of fh_verify(), fh_put() should be - * called on @fhp when the caller is finished with the filehandle. - * - * fh_verify() may be called multiple times on a given filehandle, for - * example, when processing an NFSv4 compound. The first call will look - * up a dentry using the on-the-wire filehandle. Subsequent calls will - * skip the lookup and just perform the other checks and possibly change - * the current task's credentials. - * - * @type specifies the type of object expected using one of the S_IF* - * constants defined in include/linux/stat.h. The caller may use zero - * to indicate that it doesn't care, or a negative integer to indicate - * that it expects something not of the given type. - * - * @access is formed from the NFSD_MAY_* constants defined in - * fs/nfsd/vfs.h. - */ -__be32 -fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) +static __be32 +__fh_verify(struct svc_rqst *rqstp, + struct net *net, struct svc_cred *cred, + int nfs_vers, struct auth_domain *client, + struct auth_domain *gssclient, + struct svc_fh *fhp, umode_t type, int access) { - struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); + struct nfsd_net *nn = net_generic(net, nfsd_net_id); struct svc_export *exp = NULL; struct dentry *dentry; __be32 error; if (!fhp->fh_dentry) { - error = nfsd_set_fh_dentry(rqstp, fhp); + error = nfsd_set_fh_dentry(rqstp, net, cred, nfs_vers, + client, gssclient, fhp); if (error) goto out; } dentry = fhp->fh_dentry; exp = fhp->fh_export; - trace_nfsd_fh_verify(rqstp, fhp, type, access); + if (rqstp) + trace_nfsd_fh_verify(rqstp, fhp, type, access); /* * We still have to do all these permission checks, even when @@ -358,7 +344,7 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) if (error) goto out; - error = nfsd_setuser_and_check_port(rqstp, exp); + error = nfsd_setuser_and_check_port(rqstp, cred, exp); if (error) goto out; @@ -388,14 +374,54 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) skip_pseudoflavor_check: /* Finally, check access permissions. */ - error = nfsd_permission(&rqstp->rq_cred, exp, dentry, access); + error = nfsd_permission(cred, exp, dentry, access); out: - trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error); + if (rqstp) + trace_nfsd_fh_verify_err(rqstp, fhp, type, access, error); if (error == nfserr_stale) nfsd_stats_fh_stale_inc(nn, exp); return error; } +/** + * fh_verify - filehandle lookup and access checking + * @rqstp: pointer to current rpc request + * @fhp: filehandle to be verified + * @type: expected type of object pointed to by filehandle + * @access: type of access needed to object + * + * Look up a dentry from the on-the-wire filehandle, check the client's + * access to the export, and set the current task's credentials. + * + * Regardless of success or failure of fh_verify(), fh_put() should be + * called on @fhp when the caller is finished with the filehandle. + * + * fh_verify() may be called multiple times on a given filehandle, for + * example, when processing an NFSv4 compound. The first call will look + * up a dentry using the on-the-wire filehandle. Subsequent calls will + * skip the lookup and just perform the other checks and possibly change + * the current task's credentials. + * + * @type specifies the type of object expected using one of the S_IF* + * constants defined in include/linux/stat.h. The caller may use zero + * to indicate that it doesn't care, or a negative integer to indicate + * that it expects something not of the given type. + * + * @access is formed from the NFSD_MAY_* constants defined in + * fs/nfsd/vfs.h. + */ +__be32 +fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) +{ + int nfs_vers; + if (rqstp->rq_prog == NFS_PROGRAM) + nfs_vers = rqstp->rq_vers; + else /* must be NLM */ + nfs_vers = rqstp->rq_vers == 4 ? 3 : 2; + return __fh_verify(rqstp, SVC_NET(rqstp), &rqstp->rq_cred, nfs_vers, + rqstp->rq_client, rqstp->rq_gssclient, + fhp, type, access); +} /* * Compose a file handle for an NFS reply.