| 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
| ||
|
Message-ID: <82320f68300e0cbcbc0545944191a832b946cf80.camel@kernel.org>
Date: Wed, 03 Dec 2025 14:44:15 -0500
From: Jeff Layton <jlayton@...nel.org>
To: Chuck Lever <cel@...nel.org>, 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>,
"Matthew Wilcox (Oracle)" <willy@...radead.org>, Jonathan Corbet
<corbet@....net>, NeilBrown <neil@...wn.name>, Olga Kornievskaia
<okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey
<tom@...pey.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-nfs@...r.kernel.org
Subject: Re: [PATCH 2/2] filelock: allow lease_managers to dictate what
qualifies as a conflict
On Wed, 2025-12-03 at 14:00 -0500, Chuck Lever wrote:
>
> On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> > Requesting a delegation on a file from the userland fcntl() interface
> > currently succeeds when there are conflicting opens present.
> >
> > This is because the lease handling code ignores conflicting opens for
> > FL_LAYOUT and FL_DELEG leases. This was a hack put in place long ago,
> > because nfsd already checks for conflicts in its own way. The kernel
> > needs to perform this check for userland delegations the same way it is
> > done for leases, however.
> >
> > Make this dependent on the lease_manager by adding a new
> > ->lm_open_conflict() lease_manager operation and have
> > generic_add_lease() call that instead of check_conflicting_open().
> > Morph check_conflicting_open() into a ->lm_open_conflict() op that is
> > only called for userland leases/delegations. Set the
> > ->lm_open_conflict() operations for nfsd to trivial functions that
> > always return 0.
> >
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> > Documentation/filesystems/locking.rst | 1 +
> > fs/locks.c | 90 ++++++++++++++++-------------------
> > fs/nfsd/nfs4layouts.c | 11 ++++-
> > fs/nfsd/nfs4state.c | 7 +++
> > include/linux/filelock.h | 1 +
> > 5 files changed, 60 insertions(+), 50 deletions(-)
> >
> > diff --git a/Documentation/filesystems/locking.rst
> > b/Documentation/filesystems/locking.rst
> > index
> > 77704fde98457423beae7ff00525a7383e37132b..29d453a2201bcafa03b26b706e4c68eaf5683829
> > 100644
> > --- a/Documentation/filesystems/locking.rst
> > +++ b/Documentation/filesystems/locking.rst
> > @@ -416,6 +416,7 @@ lm_change yes no no
> > lm_breaker_owns_lease: yes no no
> > lm_lock_expirable yes no no
> > lm_expire_lock no no yes
> > +lm_open_conflict yes no no
> > ====================== ============= ================= =========
> >
> > buffer_head
> > diff --git a/fs/locks.c b/fs/locks.c
> > index
> > e974f8e180fe48682a271af4f143e6bc8e9c4d3b..a58c51c2cdd0cc4496538ed54d063cd523264128
> > 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -585,10 +585,50 @@ lease_setup(struct file_lease *fl, void **priv)
> > __f_setown(filp, task_pid(current), PIDTYPE_TGID, 0);
> > }
> >
> > +/**
> > + * lease_open_conflict - see if the given file points to an inode that has
> > + * an existing open that would conflict with the
> > + * desired lease.
> > + * @filp: file to check
> > + * @arg: type of lease that we're trying to acquire
> > + *
> > + * Check to see if there's an existing open fd on this file that would
> > + * conflict with the lease we're trying to set.
> > + */
> > +static int
> > +lease_open_conflict(struct file *filp, const int arg)
> > +{
> > + struct inode *inode = file_inode(filp);
> > + int self_wcount = 0, self_rcount = 0;
> > +
> > + if (arg == F_RDLCK)
> > + return inode_is_open_for_write(inode) ? -EAGAIN : 0;
> > + else if (arg != F_WRLCK)
> > + return 0;
> > +
> > + /*
> > + * Make sure that only read/write count is from lease requestor.
> > + * Note that this will result in denying write leases when i_writecount
> > + * is negative, which is what we want. (We shouldn't grant write leases
> > + * on files open for execution.)
> > + */
> > + if (filp->f_mode & FMODE_WRITE)
> > + self_wcount = 1;
> > + else if (filp->f_mode & FMODE_READ)
> > + self_rcount = 1;
> > +
> > + if (atomic_read(&inode->i_writecount) != self_wcount ||
> > + atomic_read(&inode->i_readcount) != self_rcount)
> > + return -EAGAIN;
> > +
> > + return 0;
> > +}
> > +
> > static const struct lease_manager_operations lease_manager_ops = {
> > .lm_break = lease_break_callback,
> > .lm_change = lease_modify,
> > .lm_setup = lease_setup,
> > + .lm_open_conflict = lease_open_conflict,
> > };
> >
> > /*
> > @@ -1753,52 +1793,6 @@ int fcntl_getdeleg(struct file *filp, struct
> > delegation *deleg)
> > return 0;
> > }
> >
> > -/**
> > - * check_conflicting_open - see if the given file points to an inode
> > that has
> > - * an existing open that would conflict with the
> > - * desired lease.
> > - * @filp: file to check
> > - * @arg: type of lease that we're trying to acquire
> > - * @flags: current lock flags
> > - *
> > - * Check to see if there's an existing open fd on this file that would
> > - * conflict with the lease we're trying to set.
> > - */
> > -static int
> > -check_conflicting_open(struct file *filp, const int arg, int flags)
> > -{
> > - struct inode *inode = file_inode(filp);
> > - int self_wcount = 0, self_rcount = 0;
> > -
> > - if (flags & FL_LAYOUT)
> > - return 0;
> > - if (flags & FL_DELEG)
> > - /* We leave these checks to the caller */
> > - return 0;
> > -
> > - if (arg == F_RDLCK)
> > - return inode_is_open_for_write(inode) ? -EAGAIN : 0;
> > - else if (arg != F_WRLCK)
> > - return 0;
> > -
> > - /*
> > - * Make sure that only read/write count is from lease requestor.
> > - * Note that this will result in denying write leases when
> > i_writecount
> > - * is negative, which is what we want. (We shouldn't grant write
> > leases
> > - * on files open for execution.)
> > - */
> > - if (filp->f_mode & FMODE_WRITE)
> > - self_wcount = 1;
> > - else if (filp->f_mode & FMODE_READ)
> > - self_rcount = 1;
> > -
> > - if (atomic_read(&inode->i_writecount) != self_wcount ||
> > - atomic_read(&inode->i_readcount) != self_rcount)
> > - return -EAGAIN;
> > -
> > - return 0;
> > -}
> > -
> > static int
> > generic_add_lease(struct file *filp, int arg, struct file_lease **flp,
> > void **priv)
> > {
> > @@ -1835,7 +1829,7 @@ generic_add_lease(struct file *filp, int arg,
> > struct file_lease **flp, void **pr
> > percpu_down_read(&file_rwsem);
> > spin_lock(&ctx->flc_lock);
> > time_out_leases(inode, &dispose);
> > - error = check_conflicting_open(filp, arg, lease->c.flc_flags);
> > + error = lease->fl_lmops->lm_open_conflict(filp, arg);
> > if (error)
> > goto out;
> >
> > @@ -1892,7 +1886,7 @@ generic_add_lease(struct file *filp, int arg,
> > struct file_lease **flp, void **pr
> > * precedes these checks.
> > */
> > smp_mb();
> > - error = check_conflicting_open(filp, arg, lease->c.flc_flags);
> > + error = lease->fl_lmops->lm_open_conflict(filp, arg);
> > if (error) {
> > locks_unlink_lock_ctx(&lease->c);
> > goto out;
> > diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> > index
> > 683bd1130afe298f9df774684192c89f68102b72..ca7ec7a022bd5c12fad60ff9e51145d9cca55527
> > 100644
> > --- a/fs/nfsd/nfs4layouts.c
> > +++ b/fs/nfsd/nfs4layouts.c
> > @@ -764,9 +764,16 @@ nfsd4_layout_lm_change(struct file_lease *onlist,
> > int arg,
> > return lease_modify(onlist, arg, dispose);
> > }
> >
> > +static int
> > +nfsd4_layout_lm_open_conflict(struct file *filp, int arg)
> > +{
> > + return 0;
> > +}
> > +
>
> The usual idiom for no-op callbacks is to make them optional.
> Then generic_add_lease would check if the ->lm_open_conflict
> callback is defined first and skip the call if it's not.
>
That is what we usually do, but there are only a few lease managers and
they all need to define this op, so it saves us a trivial pointer check
to not do that. I can switch to doing it that way if you have a
preference.
> If that doesn't make sense to do, and these NFSD-specific
> functions need to remain, then our usual practice is to add
> a kdoc comment for both of the new functions that looks like
> the one you added above for lease_open_conflict().
>
Even for one that just returns 0? Ok
>
> Otherwise, I'm comfortable that this change fits in with the
> deadlock prevention patches we are considering for NFSD.
>
> Reviewed-by: Chuck Lever <chuck.lever@...cle.com>
>
> For both 1/2 and 2/2.
>
Thanks!
>
> > static const struct lease_manager_operations nfsd4_layouts_lm_ops = {
> > - .lm_break = nfsd4_layout_lm_break,
> > - .lm_change = nfsd4_layout_lm_change,
> > + .lm_break = nfsd4_layout_lm_break,
> > + .lm_change = nfsd4_layout_lm_change,
> > + .lm_open_conflict = nfsd4_layout_lm_open_conflict,
> > };
> >
> > int
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index
> > 8f8c9385101e15b64883eabec71775f26b14f890..669fabb095407e61525e5b71268cf1f06fc09877
> > 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5543,10 +5543,17 @@ nfsd_change_deleg_cb(struct file_lease *onlist,
> > int arg,
> > return -EAGAIN;
> > }
> >
> > +static int
> > +nfsd4_deleg_lm_open_conflict(struct file *filp, int arg)
> > +{
> > + return 0;
> > +}
> > +
> > static const struct lease_manager_operations nfsd_lease_mng_ops = {
> > .lm_breaker_owns_lease = nfsd_breaker_owns_lease,
> > .lm_break = nfsd_break_deleg_cb,
> > .lm_change = nfsd_change_deleg_cb,
> > + .lm_open_conflict = nfsd4_deleg_lm_open_conflict,
> > };
> >
> > static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate,
> > struct nfs4_stateowner *so, u32 seqid)
> > diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> > index
> > 54b824c05299261e6bd6acc4175cb277ea35b35d..2f5e5588ee0733c200103801d0d2ba19bebbf9af
> > 100644
> > --- a/include/linux/filelock.h
> > +++ b/include/linux/filelock.h
> > @@ -49,6 +49,7 @@ struct lease_manager_operations {
> > int (*lm_change)(struct file_lease *, int, struct list_head *);
> > void (*lm_setup)(struct file_lease *, void **);
> > bool (*lm_breaker_owns_lease)(struct file_lease *);
> > + int (*lm_open_conflict)(struct file *, int);
> > };
> >
> > struct lock_manager {
> >
> > --
> > 2.52.0
--
Jeff Layton <jlayton@...nel.org>
Powered by blists - more mailing lists