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