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