linux-kernel - Re: [PATCH 02/13] filelock: add a lm_may_setlease lease

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-id: <176047982343.1793333.618816248171085890@noble.neil.brown.name>
Date: Wed, 15 Oct 2025 09:10:23 +1100
From: NeilBrown <neilb@...mail.net>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "Miklos Szeredi" <miklos@...redi.hu>,
 "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Chuck Lever" <chuck.lever@...cle.com>,
 "Alexander Aring" <alex.aring@...il.com>,
 "Trond Myklebust" <trondmy@...nel.org>,
 "Anna Schumaker" <anna@...nel.org>, "Steve French" <sfrench@...ba.org>,
 "Paulo Alcantara" <pc@...guebit.org>,
 "Ronnie Sahlberg" <ronniesahlberg@...il.com>,
 "Shyam Prasad N" <sprasad@...rosoft.com>, "Tom Talpey" <tom@...pey.com>,
 "Bharath SM" <bharathsm@...rosoft.com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 "Danilo Krummrich" <dakr@...nel.org>,
 "David Howells" <dhowells@...hat.com>, "Tyler Hicks" <code@...icks.com>,
 "Olga Kornievskaia" <okorniev@...hat.com>,
 "Dai Ngo" <Dai.Ngo@...cle.com>, "Amir Goldstein" <amir73il@...il.com>,
 "Namjae Jeon" <linkinjeon@...nel.org>,
 "Steve French" <smfrench@...il.com>,
 "Sergey Senozhatsky" <senozhatsky@...omium.org>,
 "Carlos Maiolino" <cem@...nel.org>,
 "Kuniyuki Iwashima" <kuniyu@...gle.com>,
 "David S. Miller" <davem@...emloft.net>,
 "Eric Dumazet" <edumazet@...gle.com>, "Jakub Kicinski" <kuba@...nel.org>,
 "Paolo Abeni" <pabeni@...hat.com>, "Simon Horman" <horms@...nel.org>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-nfs@...r.kernel.org, linux-cifs@...r.kernel.org,
 samba-technical@...ts.samba.org, netfs@...ts.linux.dev,
 ecryptfs@...r.kernel.org, linux-unionfs@...r.kernel.org,
 linux-xfs@...r.kernel.org, netdev@...r.kernel.org
Subject:
 Re: [PATCH 02/13] filelock: add a lm_may_setlease lease_manager callback

On Tue, 14 Oct 2025, Jeff Layton wrote:
> On Tue, 2025-10-14 at 16:34 +1100, NeilBrown wrote:
> > On Tue, 14 Oct 2025, Jeff Layton wrote:
> > > The NFSv4.1 protocol adds support for directory delegations, but it
> > > specifies that if you already have a delegation and try to request a new
> > > one on the same filehandle, the server must reply that the delegation is
> > > unavailable.
> > > 
> > > Add a new lease manager callback to allow the lease manager (nfsd in
> > > this case) to impose this extra check when performing a setlease.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > > ---
> > >  fs/locks.c               |  5 +++++
> > >  include/linux/filelock.h | 14 ++++++++++++++
> > >  2 files changed, 19 insertions(+)
> > > 
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 0b16921fb52e602ea2e0c3de39d9d772af98ba7d..9e366b13674538dbf482ffdeee92fc717733ee20 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -1826,6 +1826,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr
> > >  			continue;
> > >  		}
> > >  
> > > +		/* Allow the lease manager to veto the setlease */
> > > +		if (lease->fl_lmops->lm_may_setlease &&
> > > +		    !lease->fl_lmops->lm_may_setlease(lease, fl))
> > > +			goto out;
> > > +
> > 
> > I don't see any locking around this.  What if the condition which
> > triggers a veto happens after this check, and before the lm_change
> > below?
> > Should lm_change implement the veto?  Return -EAGAIN?
> > 
> > 
> 
> The flc_lock is held over this check and any subsequent lease addition.
> Is that not sufficient?

Ah - I didn't see that - sorry.

But I still wonder why ->lm_change cannot do the veto.

I also wonder if the current code can work.  If that loop finds an
existing lease with the same file and the same owner the it invokes
"continue" before the code that you added.
So unless I'm misunderstanding (again) in the case that you are
interested in, the new code doesn't run.

Thanks,
NeilBrown