]> git.ipfire.org Git - thirdparty/kernel/stable-queue.git/commitdiff
6.7-stable patches
authorGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Tue, 20 Feb 2024 15:03:29 +0000 (16:03 +0100)
committerGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Tue, 20 Feb 2024 15:03:29 +0000 (16:03 +0100)
added patches:
nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch

queue-6.7/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch [new file with mode: 0644]
queue-6.7/series

diff --git a/queue-6.7/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch b/queue-6.7/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch
new file mode 100644 (file)
index 0000000..2edda0a
--- /dev/null
@@ -0,0 +1,92 @@
+From 5ea9a7c5fe4149f165f0e3b624fe08df02b6c301 Mon Sep 17 00:00:00 2001
+From: NeilBrown <neilb@suse.de>
+Date: Mon, 5 Feb 2024 13:22:39 +1100
+Subject: nfsd: don't take fi_lock in nfsd_break_deleg_cb()
+
+From: NeilBrown <neilb@suse.de>
+
+commit 5ea9a7c5fe4149f165f0e3b624fe08df02b6c301 upstream.
+
+A recent change to check_for_locks() changed it to take ->flc_lock while
+holding ->fi_lock.  This creates a lock inversion (reported by lockdep)
+because there is a case where ->fi_lock is taken while holding
+->flc_lock.
+
+->flc_lock is held across ->fl_lmops callbacks, and
+nfsd_break_deleg_cb() is one of those and does take ->fi_lock.  However
+it doesn't need to.
+
+Prior to v4.17-rc1~110^2~22 ("nfsd: create a separate lease for each
+delegation") nfsd_break_deleg_cb() would walk the ->fi_delegations list
+and so needed the lock.  Since then it doesn't walk the list and doesn't
+need the lock.
+
+Two actions are performed under the lock.  One is to call
+nfsd_break_one_deleg which calls nfsd4_run_cb().  These doesn't act on
+the nfs4_file at all, so don't need the lock.
+
+The other is to set ->fi_had_conflict which is in the nfs4_file.
+This field is only ever set here (except when initialised to false)
+so there is no possible problem will multiple threads racing when
+setting it.
+
+The field is tested twice in nfs4_set_delegation().  The first test does
+not hold a lock and is documented as an opportunistic optimisation, so
+it doesn't impose any need to hold ->fi_lock while setting
+->fi_had_conflict.
+
+The second test in nfs4_set_delegation() *is* make under ->fi_lock, so
+removing the locking when ->fi_had_conflict is set could make a change.
+The change could only be interesting if ->fi_had_conflict tested as
+false even though nfsd_break_one_deleg() ran before ->fi_lock was
+unlocked.  i.e. while hash_delegation_locked() was running.
+As hash_delegation_lock() doesn't interact in any way with nfs4_run_cb()
+there can be no importance to this interaction.
+
+So this patch removes the locking from nfsd_break_one_deleg() and moves
+the final test on ->fi_had_conflict out of the locked region to make it
+clear that locking isn't important to the test.  It is still tested
+*after* vfs_setlease() has succeeded.  This might be significant and as
+vfs_setlease() takes ->flc_lock, and nfsd_break_one_deleg() is called
+under ->flc_lock this "after" is a true ordering provided by a spinlock.
+
+Fixes: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER")
+Signed-off-by: NeilBrown <neilb@suse.de>
+Reviewed-by: Jeff Layton <jlayton@kernel.org>
+Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
+Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+---
+ fs/nfsd/nfs4state.c |   11 +++++------
+ 1 file changed, 5 insertions(+), 6 deletions(-)
+
+--- a/fs/nfsd/nfs4state.c
++++ b/fs/nfsd/nfs4state.c
+@@ -4945,10 +4945,8 @@ nfsd_break_deleg_cb(struct file_lock *fl
+        */
+       fl->fl_break_time = 0;
+-      spin_lock(&fp->fi_lock);
+       fp->fi_had_conflict = true;
+       nfsd_break_one_deleg(dp);
+-      spin_unlock(&fp->fi_lock);
+       return false;
+ }
+@@ -5557,12 +5555,13 @@ nfs4_set_delegation(struct nfsd4_open *o
+       if (status)
+               goto out_unlock;
++      status = -EAGAIN;
++      if (fp->fi_had_conflict)
++              goto out_unlock;
++
+       spin_lock(&state_lock);
+       spin_lock(&fp->fi_lock);
+-      if (fp->fi_had_conflict)
+-              status = -EAGAIN;
+-      else
+-              status = hash_delegation_locked(dp, fp);
++      status = hash_delegation_locked(dp, fp);
+       spin_unlock(&fp->fi_lock);
+       spin_unlock(&state_lock);
index 8710d86851ea7c17eca293ee04c1ecb408c97d00..fe4fbc9ad8c8962eedf62c4cbad0f54d084f3278 100644 (file)
@@ -308,3 +308,4 @@ block-fix-partial-zone-append-completion-handling-in.patch
 usb-typec-tpcm-fix-issues-with-power-being-removed-during-reset.patch
 netfilter-ipset-fix-performance-regression-in-swap-operation.patch
 netfilter-ipset-missing-gc-cancellations-fixed.patch
+nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch