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