Re: [PATCH v2 1/2] lockd: fix vfs_test_lock() calls

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2025-11-23 at 07:48 +1100, NeilBrown wrote:
> On Sun, 23 Nov 2025, Chuck Lever wrote:
> > On Fri, Nov 21, 2025, at 8:00 PM, NeilBrown wrote:
> > > From: NeilBrown <neil@brown.name>
> > > 
> > > Usage of vfs_test_lock() is somewhat confused.  Documentation suggests
> > > it is given a "lock" but this is not the case.  It is given a struct
> > > file_lock which contains some details of the sort of lock it should be
> > > looking for.
> > > 
> > > In particular passing a "file_lock" containing fl_lmops or fl_ops is
> > > meaningless and possibly confusing.
> > > 
> > > This is particularly problematic in lockd.  nlmsvc_testlock() receives
> > > an initialised "file_lock" from xdr-decode, including manager ops and an
> > > owner.  It then mistakenly passes this to vfs_test_lock() which might
> > > replace the owner and the ops.  This can lead to confusion when freeing
> > > the lock.
> > > 
> > > The primary role of the 'struct file_lock' passed to vfs_test_lock() is
> > > to report a conflicting lock that was found, so it makes more sense for
> > > nlmsvc_testlock() to pass "conflock", which it uses for returning the
> > > conflicting lock.
> > > 
> > > With this change, freeing of the lock is not confused and code in
> > > __nlm4svc_proc_test() and __nlmsvc_proc_test() can be simplified.
> > > 
> > > Documentation for vfs_test_lock() is improved to reflect its real
> > > purpose, and a WARN_ON_ONCE() is added to avoid a similar problem in the
> > > future.
> > > 
> > > Reported-by: Olga Kornievskaia <okorniev@redhat.com>
> > > Closes: https://lore.kernel.org/all/20251021130506.45065-1-okorniev@redhat.com
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > 
> > At a guess:
> > 
> > Fixes: 5ea0d75037b9 ("lockd: handle test_lock deferrals")  ??
> 
> I think the problem is, in practice, more recent.
> I think there is only a problem if the filesystem that lockd is
> accessing sets fl_lmops on locks.
> 

...and specifically if the filesystem sets lm_get_owner/lm_put_owner.
That's only NFS/NLM, currently.

> So maybe
> 
> Fixes: 20fa19027286 ("nfs: add export operations")
> 

That's likely the place where it became a real problem, yes.

> > 
> > I suspect this also needs a Cc: stable.
> 
> I think that would be appropriate - yes.
> 
> NeilBrown

Ditto.
-- 
Jeff Layton <jlayton@kernel.org>