On Fri, 28 Mar 2025, Olga Kornievskaia wrote:
> 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.
And do you have WRITE access to the file?
check_fmode_for_setlk() in fs/locks.c suggests that for F_WRLCK to be
granted the file must be open for FMODE_WRITE.
So when an exclusive lock request arrives via NLM, nlm_lookup_file()
calls nlm_do_fopen() with a mode of O_WRONLY and that causes
nfsd_permission() to check that the caller has write access to the file.
So if you are trying to get an exclusive lock to a file that you don't
have write access to, then it should fail.
If, however, you do have write access to the file - I cannot see why
asking for NFSD_MAY_READ instead of NFSD_MAY_WRITE would help.
NeilBrown
>
>
> >
> > 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
> > >
> > >
> >
>
>
