| Message ID | 20240828004445.22634-2-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Split up refactoring of fh_verify() | expand |
On Wed, 28 Aug 2024, cel@kernel.org wrote: > From: NeilBrown <neilb@suse.de> > > LOCALIO-initiated open operations are not running in an nfsd thread > and thus do not have an associated svc_rqst context. > > Signed-off-by: NeilBrown <neilb@suse.de> > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/export.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > index 7bb4f2075ac5..46a4d989c850 100644 > --- a/fs/nfsd/export.c > +++ b/fs/nfsd/export.c > @@ -1074,10 +1074,29 @@ 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). > + * > + * Return values: > + * %nfs_ok 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 = rqstp->rq_xprt; > + struct svc_xprt *xprt; > + > + /* > + * The target use case for rqstp being NULL is LOCALIO, which > + * currently only supports AUTH_UNIX. The behavior for LOCALIO > + * is therefore the same as the AUTH_UNIX check below. The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0. To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need to confirm that exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE and either exp->ex_nflavours == 0 or one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX I'm not sure that is all really necessary, but if not then we probably need a better comment... NeilBrown > + */ > + if (!rqstp) > + return nfs_ok; > + > + xprt = rqstp->rq_xprt; > > if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) { > if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags)) > @@ -1098,17 +1117,17 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) > ok: > /* legacy gss-only clients are always OK: */ > if (exp->ex_client == rqstp->rq_gssclient) > - return 0; > + return nfs_ok; > /* ip-address based client; check sec= export option: */ > for (f = exp->ex_flavors; f < end; f++) { > if (f->pseudoflavor == rqstp->rq_cred.cr_flavor) > - return 0; > + return nfs_ok; > } > /* defaults in absence of sec= options: */ > if (exp->ex_nflavors == 0) { > if (rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL || > rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX) > - return 0; > + return nfs_ok; > } > > /* If the compound op contains a spo_must_allowed op, > @@ -1118,7 +1137,7 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) > */ > > if (nfsd4_spo_must_allow(rqstp)) > - return 0; > + return nfs_ok; > > denied: > return nfserr_wrongsec; > -- > 2.45.2 > >
On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote: > On Wed, 28 Aug 2024, cel@kernel.org wrote: > > From: NeilBrown <neilb@suse.de> > > > > LOCALIO-initiated open operations are not running in an nfsd thread > > and thus do not have an associated svc_rqst context. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > --- > > fs/nfsd/export.c | 29 ++++++++++++++++++++++++----- > > 1 file changed, 24 insertions(+), 5 deletions(-) > > > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > > index 7bb4f2075ac5..46a4d989c850 100644 > > --- a/fs/nfsd/export.c > > +++ b/fs/nfsd/export.c > > @@ -1074,10 +1074,29 @@ 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). > > + * > > + * Return values: > > + * %nfs_ok 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 = rqstp->rq_xprt; > > + struct svc_xprt *xprt; > > + > > + /* > > + * The target use case for rqstp being NULL is LOCALIO, which > > + * currently only supports AUTH_UNIX. The behavior for LOCALIO > > + * is therefore the same as the AUTH_UNIX check below. > > The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0. > To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need > to confirm that > exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE > and either > exp->ex_nflavours == 0 > or > one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX > > I'm not sure that is all really necessary, but if not then we probably > need a better comment... Think extra checks aren't needed (unless you think a NULL rqstp _without_ the use of LOCALIO possible? which could trigger a false positive granting of access? seems unlikely but...) Mike
On Wed, 28 Aug 2024, Mike Snitzer wrote: > On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote: > > On Wed, 28 Aug 2024, cel@kernel.org wrote: > > > From: NeilBrown <neilb@suse.de> > > > > > > LOCALIO-initiated open operations are not running in an nfsd thread > > > and thus do not have an associated svc_rqst context. > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > > > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > > --- > > > fs/nfsd/export.c | 29 ++++++++++++++++++++++++----- > > > 1 file changed, 24 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > > > index 7bb4f2075ac5..46a4d989c850 100644 > > > --- a/fs/nfsd/export.c > > > +++ b/fs/nfsd/export.c > > > @@ -1074,10 +1074,29 @@ 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). > > > + * > > > + * Return values: > > > + * %nfs_ok 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 = rqstp->rq_xprt; > > > + struct svc_xprt *xprt; > > > + > > > + /* > > > + * The target use case for rqstp being NULL is LOCALIO, which > > > + * currently only supports AUTH_UNIX. The behavior for LOCALIO > > > + * is therefore the same as the AUTH_UNIX check below. > > > > The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0. > > To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need > > to confirm that > > exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE > > and either > > exp->ex_nflavours == 0 > > or > > one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX > > > > I'm not sure that is all really necessary, but if not then we probably > > need a better comment... > > Think extra checks aren't needed (unless you think a NULL rqstp > _without_ the use of LOCALIO possible? which could trigger a false > positive granting of access? seems unlikely but...) > I agree they aren't needed. I think we need to have a clear understanding of why that aren't needed, and to write that understanding down. So that if some day someone wants to change this code, they can understand the consequences. Maybe the answer is that LOCALIO would never ask for access that isn't allowed, so there is no need to check. Maybe the client can determine the relevant xpt_flags from the client end of the session, so it can pass them reliably to check_nfsd_access(). I don't know what is best, but I think we should have a comment justifying the short-circuit, and I don't think the current proposed comment does that correctly. Thanks, NeilBrown
> On Aug 28, 2024, at 2:30 AM, NeilBrown <neilb@suse.de> wrote: > > On Wed, 28 Aug 2024, Mike Snitzer wrote: >> On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote: >>> >>> The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0. >>> To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need >>> to confirm that >>> exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE >>> and either >>> exp->ex_nflavours == 0 >>> or >>> one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX >>> >>> I'm not sure that is all really necessary, but if not then we probably >>> need a better comment... >> >> Think extra checks aren't needed (unless you think a NULL rqstp >> _without_ the use of LOCALIO possible? which could trigger a false >> positive granting of access? seems unlikely but...) >> > > I agree they aren't needed. I think we need to have a clear > understanding of why that aren't needed, and to write that understanding > down. So that if some day someone wants to change this code, they can > understand the consequences. > I don't know what is best, but I think we should have a comment > justifying the short-circuit, and I don't think the current proposed > comment does that correctly. My goal is to document that understanding here, as you stated. I will leave it to you and Mike to adjust the wording to your liking. -- Chuck Lever
On Wed, Aug 28, 2024 at 04:30:05PM +1000, NeilBrown wrote: > On Wed, 28 Aug 2024, Mike Snitzer wrote: > > On Wed, Aug 28, 2024 at 11:12:00AM +1000, NeilBrown wrote: > > > On Wed, 28 Aug 2024, cel@kernel.org wrote: > > > > From: NeilBrown <neilb@suse.de> > > > > > > > > LOCALIO-initiated open operations are not running in an nfsd thread > > > > and thus do not have an associated svc_rqst context. > > > > > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > Co-developed-by: Mike Snitzer <snitzer@kernel.org> > > > > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > > > --- > > > > fs/nfsd/export.c | 29 ++++++++++++++++++++++++----- > > > > 1 file changed, 24 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > > > > index 7bb4f2075ac5..46a4d989c850 100644 > > > > --- a/fs/nfsd/export.c > > > > +++ b/fs/nfsd/export.c > > > > @@ -1074,10 +1074,29 @@ 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). > > > > + * > > > > + * Return values: > > > > + * %nfs_ok 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 = rqstp->rq_xprt; > > > > + struct svc_xprt *xprt; > > > > + > > > > + /* > > > > + * The target use case for rqstp being NULL is LOCALIO, which > > > > + * currently only supports AUTH_UNIX. The behavior for LOCALIO > > > > + * is therefore the same as the AUTH_UNIX check below. > > > > > > The "AUTH_UNIX check below" only applies if exp->ex_flavours == 0. > > > To make "rqstp == NULL" mean "treat like AUTH_UNIX" I think we need > > > to confirm that > > > exp->ex_xprtsec_mods & NFSEXP_XPRTSEC_NONE > > > and either > > > exp->ex_nflavours == 0 > > > or > > > one for the exp->ex_flavors->pseudoflavor values is RPC_AUTH_UNIX > > > > > > I'm not sure that is all really necessary, but if not then we probably > > > need a better comment... > > > > Think extra checks aren't needed (unless you think a NULL rqstp > > _without_ the use of LOCALIO possible? which could trigger a false > > positive granting of access? seems unlikely but...) > > > > I agree they aren't needed. I think we need to have a clear > understanding of why that aren't needed, and to write that understanding > down. So that if some day someone wants to change this code, they can > understand the consequences. > > Maybe the answer is that LOCALIO would never ask for access that isn't > allowed, so there is no need to check. > > Maybe the client can determine the relevant xpt_flags from the client > end of the session, so it can pass them reliably to check_nfsd_access(). > > I don't know what is best, but I think we should have a comment > justifying the short-circuit, and I don't think the current proposed > comment does that correctly. Just to recap, this is what you had originally, which Chuck correctly said needed improvement: __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 */ return 0; I offered my suggestion and Chuck then tweaked it: __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) { - /* - * 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; - } + /* + * The target use case for rqstp being NULL is LOCALIO, which + * currently only supports AUTH_UNIX. The behavior for LOCALIO + * is therefore the same as the AUTH_UNIX check below. + */ + if (!rqstp) + return nfs_ok; Now you're saying that comment needs to be more precise... ;) localio only supports AUTH_UNIX, and the client verifies that is the method being used: void nfs_local_probe(struct nfs_client *clp) { nfs_uuid_t nfs_uuid; /* Disallow localio if disabled via sysfs or AUTH_SYS isn't used */ if (!localio_enabled || clp->cl_rpcclient->cl_auth->au_flavor != RPC_AUTH_UNIX) { nfs_local_disable(clp); return; } ... So I honestly feel like Chuck's latest revision is perfectly fine. I disagree that "The behavior for LOCALIO is therefore the same as the AUTH_UNIX check below." is inaccurate. The precondition from the client (used to establish localio and cause rqstp to be NULL in check_nfsd_access) is effectively comparable no?
On Wed, 28 Aug 2024, Mike Snitzer wrote: > > So I honestly feel like Chuck's latest revision is perfectly fine. > I disagree that "The behavior for LOCALIO is therefore the same as > the AUTH_UNIX check below." is inaccurate. The precondition from the > client (used to establish localio and cause rqstp to be NULL in > check_nfsd_access) is effectively comparable no? > I don't think the correctness of the code is at all related to AUTH_UNIX. Suppose we did add support for krb5 somehow - would we really need a different test? I think not. I think that the reason the code is correct and safe is that we trust the client. We *know* the client will only present an filehandle which it has received over the wire and which it verified - with the accompanying credential - it was allowed to access. Maybe that is what you meant by "The precondition from the client". I agree that does give us sufficient safety. I don't think AUTH_UNIX is relevant. /* * If rqstp is NULL, this is a LOCALIO request which will only ever use * filehandle/credential pair for which access has been affirmed (by * ACCESS or OPEN NFS requests) over the wire. So there is no need for * further checks here. */ (It wasn't quite this clear to me when I wrote previously - but I always seems to think more clearly in the mornings!) Thanks, NeilBrown
On Thu, Aug 29, 2024 at 07:05:45AM +1000, NeilBrown wrote: > On Wed, 28 Aug 2024, Mike Snitzer wrote: > > > > So I honestly feel like Chuck's latest revision is perfectly fine. > > I disagree that "The behavior for LOCALIO is therefore the same as > > the AUTH_UNIX check below." is inaccurate. The precondition from the > > client (used to establish localio and cause rqstp to be NULL in > > check_nfsd_access) is effectively comparable no? > > > > I don't think the correctness of the code is at all related to > AUTH_UNIX. > Suppose we did add support for krb5 somehow - would we really need a > different test? I think not. > > I think that the reason the code is correct and safe is that we trust > the client. We *know* the client will only present an filehandle which > it has received over the wire and which it verified - with the > accompanying credential - it was allowed to access. > > Maybe that is what you meant by "The precondition from the client". I > agree that does give us sufficient safety. I don't think AUTH_UNIX is > relevant. > > /* > * If rqstp is NULL, this is a LOCALIO request which will only ever use > * filehandle/credential pair for which access has been affirmed (by > * ACCESS or OPEN NFS requests) over the wire. So there is no need for > * further checks here. > */ Makes sense, and thanks! > (It wasn't quite this clear to me when I wrote previously - but I always > seems to think more clearly in the mornings!) I haven't been sleeping enough.. tonight, tonight I will!!! ;) Mike
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c index 7bb4f2075ac5..46a4d989c850 100644 --- a/fs/nfsd/export.c +++ b/fs/nfsd/export.c @@ -1074,10 +1074,29 @@ 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). + * + * Return values: + * %nfs_ok 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 = rqstp->rq_xprt; + struct svc_xprt *xprt; + + /* + * The target use case for rqstp being NULL is LOCALIO, which + * currently only supports AUTH_UNIX. The behavior for LOCALIO + * is therefore the same as the AUTH_UNIX check below. + */ + if (!rqstp) + return nfs_ok; + + xprt = rqstp->rq_xprt; if (exp->ex_xprtsec_modes & NFSEXP_XPRTSEC_NONE) { if (!test_bit(XPT_TLS_SESSION, &xprt->xpt_flags)) @@ -1098,17 +1117,17 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) ok: /* legacy gss-only clients are always OK: */ if (exp->ex_client == rqstp->rq_gssclient) - return 0; + return nfs_ok; /* ip-address based client; check sec= export option: */ for (f = exp->ex_flavors; f < end; f++) { if (f->pseudoflavor == rqstp->rq_cred.cr_flavor) - return 0; + return nfs_ok; } /* defaults in absence of sec= options: */ if (exp->ex_nflavors == 0) { if (rqstp->rq_cred.cr_flavor == RPC_AUTH_NULL || rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX) - return 0; + return nfs_ok; } /* If the compound op contains a spo_must_allowed op, @@ -1118,7 +1137,7 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) */ if (nfsd4_spo_must_allow(rqstp)) - return 0; + return nfs_ok; denied: return nfserr_wrongsec;