Re: [RFC PATCH] NFSD: Replace use of NFSD_MAY_LOCK in nfsd4_lock()

[Chuck Lever <chuck.lever@oracle.com>]

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
> > 
> 
> 

-- 
Chuck Lever