On Thu, Mar 27, 2025 at 7:54 PM NeilBrown <neilb@xxxxxxx> wrote:
>
> On Sat, 22 Mar 2025, Olga Kornievskaia wrote:
> > NLM locking calls need to pass thru file permission checking
> > and for that prior to calling inode_permission() we need
> > to set appropriate access mask.
> >
> > Fixes: 4cc9b9f2bf4d ("nfsd: refine and rename NFSD_MAY_LOCK")
> > Signed-off-by: Olga Kornievskaia <okorniev@xxxxxxxxxx>
> > ---
> > fs/nfsd/vfs.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 4021b047eb18..7928ae21509f 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -2582,6 +2582,13 @@ nfsd_permission(struct svc_cred *cred, struct svc_export *exp,
> > if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode))
> > return nfserr_perm;
> >
> > + /*
> > + * For the purpose of permission checking of NLM requests,
> > + * the locker must have READ access or own the file
> > + */
> > + if (acc & NFSD_MAY_NLM)
> > + acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE;
> > +
>
> I don't agree with this change.
> The only time that NFSD_MAY_NLM is set, NFSD_MAY_OWNER_OVERRIDE is also
> set. So that part of the change adds no value.
>
> This change only affects the case where a write lock is being requested.
> In that case acc will contains NFSD_MAY_WRITE but not NFSD_MAY_READ.
> This change will set NFSD_MAY_READ. Is that really needed?
>
> Can you please describe the particular problem you saw that is fixed by
> this patch? If there is a problem and we do need to add NFSD_MAY_READ,
> then I would rather it were done in nlm_fopen().
set export policy with (sec=krb5:...) then mount with sec=krb5,vers=3,
then ask for an exclusive flock(), it would fail.
The reason it fails is because nlm_fopen() translates lock to open
with WRITE. Prior to patch 4cc9b9f2bf4d, the access would be set to
acc = NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE; before calling into
inode_permission(). The patch changed it and lead to lock no longer
being given out with sec=krb5 policy.
>
> Thanks,
> NeilBrown
>
>
> > /*
> > * The file owner always gets access permission for accesses that
> > * would normally be checked at open time. This is to make
> > --
> > 2.47.1
> >
> >
>
