diff mbox series

[06/13] afs: fix __afs_break_callback() / afs_drop_open_mmap() race

Message ID 20240204021739.1157830-6-viro@zeniv.linux.org.uk
State New
Headers show
Series [01/13] fs/super.c: don't drop ->s_user_ns until we free struct super_block itself | expand

Commit Message

Al Viro Feb. 4, 2024, 2:17 a.m. UTC
In __afs_break_callback() we might check ->cb_nr_mmap and if it's non-zero
do queue_work(&vnode->cb_work).  In afs_drop_open_mmap() we decrement
->cb_nr_mmap and do flush_work(&vnode->cb_work) if it reaches zero.

The trouble is, there's nothing to prevent __afs_break_callback() from
seeing ->cb_nr_mmap before the decrement and do queue_work() after both
the decrement and flush_work().  If that happens, we might be in trouble -
vnode might get freed before the queued work runs.

__afs_break_callback() is always done under ->cb_lock, so let's make
sure that ->cb_nr_mmap can change from non-zero to zero while holding
->cb_lock (the spinlock component of it - it's a seqlock and we don't
need to mess with the counter).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/afs/file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Christian Brauner Feb. 5, 2024, 12:28 p.m. UTC | #1
On Sun, Feb 04, 2024 at 02:17:32AM +0000, Al Viro wrote:
> In __afs_break_callback() we might check ->cb_nr_mmap and if it's non-zero
> do queue_work(&vnode->cb_work).  In afs_drop_open_mmap() we decrement
> ->cb_nr_mmap and do flush_work(&vnode->cb_work) if it reaches zero.
> 
> The trouble is, there's nothing to prevent __afs_break_callback() from
> seeing ->cb_nr_mmap before the decrement and do queue_work() after both
> the decrement and flush_work().  If that happens, we might be in trouble -
> vnode might get freed before the queued work runs.
> 
> __afs_break_callback() is always done under ->cb_lock, so let's make
> sure that ->cb_nr_mmap can change from non-zero to zero while holding
> ->cb_lock (the spinlock component of it - it's a seqlock and we don't
> need to mess with the counter).
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Acked-by: Christian Brauner <brauner@kernel.org>
diff mbox series

Patch

diff --git a/fs/afs/file.c b/fs/afs/file.c
index 3d33b221d9ca..ef2cc8f565d2 100644
--- a/fs/afs/file.c
+++ b/fs/afs/file.c
@@ -417,13 +417,17 @@  static void afs_add_open_mmap(struct afs_vnode *vnode)
 
 static void afs_drop_open_mmap(struct afs_vnode *vnode)
 {
-	if (!atomic_dec_and_test(&vnode->cb_nr_mmap))
+	if (atomic_add_unless(&vnode->cb_nr_mmap, -1, 1))
 		return;
 
 	down_write(&vnode->volume->open_mmaps_lock);
 
-	if (atomic_read(&vnode->cb_nr_mmap) == 0)
+	read_seqlock_excl(&vnode->cb_lock);
+	// the only place where ->cb_nr_mmap may hit 0
+	// see __afs_break_callback() for the other side...
+	if (atomic_dec_and_test(&vnode->cb_nr_mmap))
 		list_del_init(&vnode->cb_mmap_link);
+	read_sequnlock_excl(&vnode->cb_lock);
 
 	up_write(&vnode->volume->open_mmaps_lock);
 	flush_work(&vnode->cb_work);