On Thu, Mar 27, 2025 at 9:43 PM NeilBrown <neilb@xxxxxxx> wrote:
>
> 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.
That's correct, the user doing flock() does NOT have write access to
the file. Yet prior to the 4cc9b9f2bf4d, that access was allowed. If
that was a bug then my bad. I assumed it was regression.
It's interesting to me that on an XFS file system, I can create a file
owned by root (on a local filesystem) and then request an exclusive
lock on it (as a user -- no write permissions).
okorniev@linux:~$ ls -l /export/foobar
-rw-r--r--. 1 root root 4 Mar 28 10:46 /export/foobar
okorniev@linux:~$ flock -x /export/foobar sleep 1s
>
> 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
> > > >
> > > >
> > >
> >
> >
>
>
