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@xxxxxxxxxx wrote:
> > > > From: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxx>
> > >
> > > 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
