| Message ID | 20241010153331.143845-2-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] NFSD: Replace use of NFSD_MAY_LOCK in nfsd4_lock() | expand |
On Fri, 11 Oct 2024, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > NFSv4 LOCK operations should not avoid the set of authorization > checks that apply to all other NFSv4 operations. Also, the > "no_auth_nlm" export option should apply only to NLM LOCK requests. > It's not necessary or sensible to apply it to NFSv4 LOCK operations. > > The replacement MAY bit mask, > "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE", comes from the access > bits that are set in nfsd_permission() when the caller has set > NFSD_MAY_LOCK. > > Reported-by: NeilBrown <neilb@suse.de> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs4state.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 9c2b1d251ab3..3f2c11414390 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -7967,11 +7967,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > if (check_lock_length(lock->lk_offset, lock->lk_length)) > return nfserr_inval; > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > - S_IFREG, NFSD_MAY_LOCK))) { > - dprintk("NFSD: nfsd4_lock: permission denied!\n"); > + status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, > + NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE); > + if (status != nfs_ok) > return status; > - } Reviewed-by: NeilBrown <neilb@suse.de> though I think we want a follow-on patch which uses NFSD_MAY_WRITE for write locks for consistency with check_fmode_for_setlk(). And I'm wondering about NFSD_MAY_OWNER_OVERRIDE ... that is really an NFSv3 thing. For NFSv4 we should be checking permission at "open" time, recording that in the state (both of which we do) and then performing permission checks against the state rather than against the inode. But that is a whole different can of worms. Thanks, NeilBrown > sb = cstate->current_fh.fh_dentry->d_sb; > > if (lock->lk_is_new) { > -- > 2.46.2 > >
On Thu, 2024-10-10 at 11:33 -0400, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > NFSv4 LOCK operations should not avoid the set of authorization > checks that apply to all other NFSv4 operations. Also, the > "no_auth_nlm" export option should apply only to NLM LOCK requests. > It's not necessary or sensible to apply it to NFSv4 LOCK operations. > > The replacement MAY bit mask, > "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE", comes from the access > bits that are set in nfsd_permission() when the caller has set > NFSD_MAY_LOCK. > > Reported-by: NeilBrown <neilb@suse.de> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs4state.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 9c2b1d251ab3..3f2c11414390 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -7967,11 +7967,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > if (check_lock_length(lock->lk_offset, lock->lk_length)) > return nfserr_inval; > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > - S_IFREG, NFSD_MAY_LOCK))) { > - dprintk("NFSD: nfsd4_lock: permission denied!\n"); > + status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, > + NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE); > + if (status != nfs_ok) > return status; > - } > sb = cstate->current_fh.fh_dentry->d_sb; > > if (lock->lk_is_new) { I would say this warrants a comment as to why you're not using NFSD_MAY_LOCK here, but Neil's renaming patch takes care of that. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, Oct 11, 2024 at 07:54:12AM +1100, NeilBrown wrote: > On Fri, 11 Oct 2024, cel@kernel.org wrote: > > From: Chuck Lever <chuck.lever@oracle.com> > > > > NFSv4 LOCK operations should not avoid the set of authorization > > checks that apply to all other NFSv4 operations. Also, the > > "no_auth_nlm" export option should apply only to NLM LOCK requests. > > It's not necessary or sensible to apply it to NFSv4 LOCK operations. > > > > The replacement MAY bit mask, > > "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE", comes from the access > > bits that are set in nfsd_permission() when the caller has set > > NFSD_MAY_LOCK. > > > > Reported-by: NeilBrown <neilb@suse.de> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > --- > > fs/nfsd/nfs4state.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 9c2b1d251ab3..3f2c11414390 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -7967,11 +7967,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > if (check_lock_length(lock->lk_offset, lock->lk_length)) > > return nfserr_inval; > > > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > > - S_IFREG, NFSD_MAY_LOCK))) { > > - dprintk("NFSD: nfsd4_lock: permission denied!\n"); > > + status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, > > + NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE); > > + if (status != nfs_ok) > > return status; > > - } > > Reviewed-by: NeilBrown <neilb@suse.de> > > though I think we want a follow-on patch which uses NFSD_MAY_WRITE for > write locks for consistency with check_fmode_for_setlk(). I think this patch might introduce a behavior regression, then. Instead of a follow-on, I need a v2 of this patch. > And I'm wondering about NFSD_MAY_OWNER_OVERRIDE ... that is really an > NFSv3 thing. For NFSv4 we should be checking permission at "open" time, > recording that in the state (both of which we do) and then performing > permission checks against the state rather than against the inode. > But that is a whole different can of worms. I see several sites in NFSv4 land that assert OWNER_OVERRIDE. But point taken on taking the permissions from the state ID instead of using a fixed mask. > Thanks, > NeilBrown > > > > sb = cstate->current_fh.fh_dentry->d_sb; > > > > if (lock->lk_is_new) { > > -- > > 2.46.2 > > > > >
On Sat, 12 Oct 2024, Chuck Lever wrote: > On Fri, Oct 11, 2024 at 07:54:12AM +1100, NeilBrown wrote: > > On Fri, 11 Oct 2024, cel@kernel.org wrote: > > > From: Chuck Lever <chuck.lever@oracle.com> > > > > > > NFSv4 LOCK operations should not avoid the set of authorization > > > checks that apply to all other NFSv4 operations. Also, the > > > "no_auth_nlm" export option should apply only to NLM LOCK requests. > > > It's not necessary or sensible to apply it to NFSv4 LOCK operations. > > > > > > The replacement MAY bit mask, > > > "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE", comes from the access > > > bits that are set in nfsd_permission() when the caller has set > > > NFSD_MAY_LOCK. > > > > > > Reported-by: NeilBrown <neilb@suse.de> > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > > --- > > > fs/nfsd/nfs4state.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > index 9c2b1d251ab3..3f2c11414390 100644 > > > --- a/fs/nfsd/nfs4state.c > > > +++ b/fs/nfsd/nfs4state.c > > > @@ -7967,11 +7967,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > > if (check_lock_length(lock->lk_offset, lock->lk_length)) > > > return nfserr_inval; > > > > > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > > > - S_IFREG, NFSD_MAY_LOCK))) { > > > - dprintk("NFSD: nfsd4_lock: permission denied!\n"); > > > + status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, > > > + NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE); > > > + if (status != nfs_ok) > > > return status; > > > - } > > > > Reviewed-by: NeilBrown <neilb@suse.de> > > > > though I think we want a follow-on patch which uses NFSD_MAY_WRITE for > > write locks for consistency with check_fmode_for_setlk(). > > I think this patch might introduce a behavior regression, then. > Instead of a follow-on, I need a v2 of this patch. This is not a regression - it has always been this way (since 2.3.42). And both NLM and v4 suffer - I was wrong about NLM. If MAY_LOCK is set, then any MAY_READ or MAY_WRITE flag is ignored, and the 'acc' passed to inode_permission() is only MAY_READ | MAY_OWNER_OVERRIDE So any locking over nfsd currently requires ownership or READ access to the inode. This is slightly different behaviour to local filesystems and it might be nice to fix but I don't think it is an important difference. Importantly your patch doesn't change this behaviour at all. Thanks, NeilBrown > > > > And I'm wondering about NFSD_MAY_OWNER_OVERRIDE ... that is really an > > NFSv3 thing. For NFSv4 we should be checking permission at "open" time, > > recording that in the state (both of which we do) and then performing > > permission checks against the state rather than against the inode. > > But that is a whole different can of worms. > > I see several sites in NFSv4 land that assert OWNER_OVERRIDE. But > point taken on taking the permissions from the state ID instead of > using a fixed mask. > > > > Thanks, > > NeilBrown > > > > > > > sb = cstate->current_fh.fh_dentry->d_sb; > > > > > > if (lock->lk_is_new) { > > > -- > > > 2.46.2 > > > > > > > > > > -- > Chuck Lever >
On Sat, Oct 12, 2024 at 07:53:34AM +1100, NeilBrown wrote: > On Sat, 12 Oct 2024, Chuck Lever wrote: > > On Fri, Oct 11, 2024 at 07:54:12AM +1100, NeilBrown wrote: > > > On Fri, 11 Oct 2024, cel@kernel.org wrote: > > > > From: Chuck Lever <chuck.lever@oracle.com> > > > > > > > > NFSv4 LOCK operations should not avoid the set of authorization > > > > checks that apply to all other NFSv4 operations. Also, the > > > > "no_auth_nlm" export option should apply only to NLM LOCK requests. > > > > It's not necessary or sensible to apply it to NFSv4 LOCK operations. > > > > > > > > The replacement MAY bit mask, > > > > "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE", comes from the access > > > > bits that are set in nfsd_permission() when the caller has set > > > > NFSD_MAY_LOCK. > > > > > > > > Reported-by: NeilBrown <neilb@suse.de> > > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > > > --- > > > > fs/nfsd/nfs4state.c | 7 +++---- > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > > index 9c2b1d251ab3..3f2c11414390 100644 > > > > --- a/fs/nfsd/nfs4state.c > > > > +++ b/fs/nfsd/nfs4state.c > > > > @@ -7967,11 +7967,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > > > if (check_lock_length(lock->lk_offset, lock->lk_length)) > > > > return nfserr_inval; > > > > > > > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > > > > - S_IFREG, NFSD_MAY_LOCK))) { > > > > - dprintk("NFSD: nfsd4_lock: permission denied!\n"); > > > > + status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, > > > > + NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE); > > > > + if (status != nfs_ok) > > > > return status; > > > > - } > > > > > > Reviewed-by: NeilBrown <neilb@suse.de> > > > > > > though I think we want a follow-on patch which uses NFSD_MAY_WRITE for > > > write locks for consistency with check_fmode_for_setlk(). > > > > I think this patch might introduce a behavior regression, then. > > Instead of a follow-on, I need a v2 of this patch. > > This is not a regression - it has always been this way (since 2.3.42). > And both NLM and v4 suffer - I was wrong about NLM. > > If MAY_LOCK is set, then any MAY_READ or MAY_WRITE flag is ignored, and > the 'acc' passed to inode_permission() is only MAY_READ | > MAY_OWNER_OVERRIDE That's what I thought when I looked at nfsd_permission() again. > So any locking over nfsd currently requires ownership or READ access to > the inode. This is slightly different behaviour to local filesystems > and it might be nice to fix but I don't think it is an important > difference. Importantly your patch doesn't change this behaviour at all. nfsd4_lock(), IIUC, thoroughly examines the stateid just after it does the fh_verify(). Maybe this would be OK: status = fh_verify( ... , 0); This is what the other NFSv4 lock-related operations do. > > > And I'm wondering about NFSD_MAY_OWNER_OVERRIDE ... that is really an > > > NFSv3 thing. For NFSv4 we should be checking permission at "open" time, > > > recording that in the state (both of which we do) and then performing > > > permission checks against the state rather than against the inode. > > > But that is a whole different can of worms. > > > > I see several sites in NFSv4 land that assert OWNER_OVERRIDE. But > > point taken on taking the permissions from the state ID instead of > > using a fixed mask. > > > > > > > Thanks, > > > NeilBrown > > > > > > > > > > sb = cstate->current_fh.fh_dentry->d_sb; > > > > > > > > if (lock->lk_is_new) { > > > > -- > > > > 2.46.2 > > > > > > > > > > > > > > > -- > > Chuck Lever > > > >
On Sat, 12 Oct 2024, Chuck Lever wrote: > On Sat, Oct 12, 2024 at 07:53:34AM +1100, NeilBrown wrote: > > On Sat, 12 Oct 2024, Chuck Lever wrote: > > > On Fri, Oct 11, 2024 at 07:54:12AM +1100, NeilBrown wrote: > > > > On Fri, 11 Oct 2024, cel@kernel.org wrote: > > > > > From: Chuck Lever <chuck.lever@oracle.com> > > > > > > > > > > NFSv4 LOCK operations should not avoid the set of authorization > > > > > checks that apply to all other NFSv4 operations. Also, the > > > > > "no_auth_nlm" export option should apply only to NLM LOCK requests. > > > > > It's not necessary or sensible to apply it to NFSv4 LOCK operations. > > > > > > > > > > The replacement MAY bit mask, > > > > > "NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE", comes from the access > > > > > bits that are set in nfsd_permission() when the caller has set > > > > > NFSD_MAY_LOCK. > > > > > > > > > > Reported-by: NeilBrown <neilb@suse.de> > > > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > > > > --- > > > > > fs/nfsd/nfs4state.c | 7 +++---- > > > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > > > index 9c2b1d251ab3..3f2c11414390 100644 > > > > > --- a/fs/nfsd/nfs4state.c > > > > > +++ b/fs/nfsd/nfs4state.c > > > > > @@ -7967,11 +7967,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > > > > if (check_lock_length(lock->lk_offset, lock->lk_length)) > > > > > return nfserr_inval; > > > > > > > > > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > > > > > - S_IFREG, NFSD_MAY_LOCK))) { > > > > > - dprintk("NFSD: nfsd4_lock: permission denied!\n"); > > > > > + status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, > > > > > + NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE); > > > > > + if (status != nfs_ok) > > > > > return status; > > > > > - } > > > > > > > > Reviewed-by: NeilBrown <neilb@suse.de> > > > > > > > > though I think we want a follow-on patch which uses NFSD_MAY_WRITE for > > > > write locks for consistency with check_fmode_for_setlk(). > > > > > > I think this patch might introduce a behavior regression, then. > > > Instead of a follow-on, I need a v2 of this patch. > > > > This is not a regression - it has always been this way (since 2.3.42). > > And both NLM and v4 suffer - I was wrong about NLM. > > > > If MAY_LOCK is set, then any MAY_READ or MAY_WRITE flag is ignored, and > > the 'acc' passed to inode_permission() is only MAY_READ | > > MAY_OWNER_OVERRIDE > > That's what I thought when I looked at nfsd_permission() again. > > > > So any locking over nfsd currently requires ownership or READ access to > > the inode. This is slightly different behaviour to local filesystems > > and it might be nice to fix but I don't think it is an important > > difference. Importantly your patch doesn't change this behaviour at all. > > nfsd4_lock(), IIUC, thoroughly examines the stateid just after it > does the fh_verify(). Maybe this would be OK: > > status = fh_verify( ... , 0); > > This is what the other NFSv4 lock-related operations do. I like that! I haven't looked at the code yet to comment on correctness, but it does seem like the right sort of approach. Thanks, NeilBrown
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 9c2b1d251ab3..3f2c11414390 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -7967,11 +7967,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, if (check_lock_length(lock->lk_offset, lock->lk_length)) return nfserr_inval; - if ((status = fh_verify(rqstp, &cstate->current_fh, - S_IFREG, NFSD_MAY_LOCK))) { - dprintk("NFSD: nfsd4_lock: permission denied!\n"); + status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, + NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE); + if (status != nfs_ok) return status; - } sb = cstate->current_fh.fh_dentry->d_sb; if (lock->lk_is_new) {