diff mbox series

fs/smb: server: fix recursive locking in vfs helpers

Message ID 20231012174349.462290-1-mmakassikis@freebox.fr (mailing list archive)
State New, archived
Headers show
Series fs/smb: server: fix recursive locking in vfs helpers | expand

Commit Message

Marios Makassikis Oct. 12, 2023, 5:43 p.m. UTC
Running smb2.rename test from Samba smbtorture suite against a kernel built
with lockdep triggers a "possible recursive locking detected" warning.

This is because mnt_want_write() is called twice with no mnt_drop_write()
in between:
  -> ksmbd_vfs_mkdir()
    -> ksmbd_vfs_kern_path_create()
       -> kern_path_create()
          -> filename_create()
            -> mnt_want_write()
       -> mnt_want_write()

Fix this by removing the mnt_want_write/mnt_drop_write calls from vfs
helpers that call kern_path_create().

Full lockdep trace below:

============================================
WARNING: possible recursive locking detected
6.6.0-rc5 #775 Not tainted
--------------------------------------------
kworker/1:1/32 is trying to acquire lock:
ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at: ksmbd_vfs_mkdir+0xe1/0x410

but task is already holding lock:
ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at: filename_create+0xb6/0x260

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(sb_writers#5);
  lock(sb_writers#5);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

4 locks held by kworker/1:1/32:
 #0: ffff8880064e4138 ((wq_completion)ksmbd-io){+.+.}-{0:0}, at: process_one_work+0x40e/0x980
 #1: ffff888005b0fdd0 ((work_completion)(&work->work)){+.+.}-{0:0}, at: process_one_work+0x40e/0x980
 #2: ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at: filename_create+0xb6/0x260
 #3: ffff8880057ce760 (&type->i_mutex_dir_key#3/1){+.+.}-{3:3}, at: filename_create+0x123/0x260

stack backtrace:
CPU: 1 PID: 32 Comm: kworker/1:1 Not tainted 6.6.0-rc5 #775
Workqueue: ksmbd-io handle_ksmbd_work
Call Trace:
 <TASK>
 dump_stack_lvl+0x4f/0x90
 dump_stack+0x14/0x20
 print_deadlock_bug+0x2f0/0x410
 check_deadlock+0x26b/0x3b0
 __lock_acquire+0xce2/0x1060
 ? mark_lock.part.0+0xff/0x720
 ? __pfx___lock_acquire+0x10/0x10
 ? mark_held_locks+0x6b/0x90
 lock_acquire.part.0+0x125/0x2d0
 ? ksmbd_vfs_mkdir+0xe1/0x410
 ? __pfx_lock_acquire.part.0+0x10/0x10
 ? __kmem_cache_free+0x179/0x280
 ? ____kasan_slab_free+0x15b/0x1d0
 ? __kasan_slab_free+0x16/0x20
 ? slab_free_freelist_hook+0xbc/0x180
 lock_acquire+0x93/0x160
 ? ksmbd_vfs_mkdir+0xe1/0x410
 mnt_want_write+0x49/0x220
 ? ksmbd_vfs_mkdir+0xe1/0x410
 ksmbd_vfs_mkdir+0xe1/0x410
 ? __pfx__printk+0x10/0x10
 ? __pfx_ksmbd_vfs_mkdir+0x10/0x10
 smb2_open+0x1064/0x38b0
 ? __pfx___lock_acquire+0x10/0x10
 ? __pfx_smb2_open+0x10/0x10
 ? __lock_release+0x13f/0x290
 ? smb2_validate_credit_charge+0x25d/0x360
 ? __pfx___lock_release+0x10/0x10
 ? do_raw_spin_lock+0x127/0x1c0
 ? __pfx_do_raw_spin_lock+0x10/0x10
 ? do_raw_spin_unlock+0xac/0x110
 ? _raw_spin_unlock+0x22/0x50
 ? smb2_validate_credit_charge+0x25d/0x360
 ? ksmbd_smb2_check_message+0x3be/0x400
 ? __pfx_ksmbd_smb2_check_message+0x10/0x10
 __process_request+0x151/0x310
 __handle_ksmbd_work+0x33c/0x520
 ? __pfx___handle_ksmbd_work+0x10/0x10
 handle_ksmbd_work+0x4a/0xd0
 process_one_work+0x4a7/0x980
 ? __pfx_process_one_work+0x10/0x10
 ? assign_work+0xe1/0x120
 worker_thread+0x365/0x570
 ? __pfx_worker_thread+0x10/0x10
 kthread+0x18d/0x1d0
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x38/0x70
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1b/0x30
 </TASK>

Fixes: 40b268d384a2 ("ksmbd: add mnt_want_write to ksmbd vfs functions")
Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
---
 fs/smb/server/vfs.c | 17 -----------------
 1 file changed, 17 deletions(-)

Comments

Namjae Jeon Oct. 13, 2023, 11:59 p.m. UTC | #1
2023-10-13 2:43 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>:
> Running smb2.rename test from Samba smbtorture suite against a kernel built
> with lockdep triggers a "possible recursive locking detected" warning.
>
> This is because mnt_want_write() is called twice with no mnt_drop_write()
> in between:
>   -> ksmbd_vfs_mkdir()
>     -> ksmbd_vfs_kern_path_create()
>        -> kern_path_create()
>           -> filename_create()
>             -> mnt_want_write()
>        -> mnt_want_write()
>
> Fix this by removing the mnt_want_write/mnt_drop_write calls from vfs
> helpers that call kern_path_create().
>
> Full lockdep trace below:
>
> ============================================
> WARNING: possible recursive locking detected
> 6.6.0-rc5 #775 Not tainted
> --------------------------------------------
> kworker/1:1/32 is trying to acquire lock:
> ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at: ksmbd_vfs_mkdir+0xe1/0x410
>
> but task is already holding lock:
> ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at: filename_create+0xb6/0x260
>
> other info that might help us debug this:
>  Possible unsafe locking scenario:
>
>        CPU0
>        ----
>   lock(sb_writers#5);
>   lock(sb_writers#5);
>
>  *** DEADLOCK ***
>
>  May be due to missing lock nesting notation
>
> 4 locks held by kworker/1:1/32:
>  #0: ffff8880064e4138 ((wq_completion)ksmbd-io){+.+.}-{0:0}, at:
> process_one_work+0x40e/0x980
>  #1: ffff888005b0fdd0 ((work_completion)(&work->work)){+.+.}-{0:0}, at:
> process_one_work+0x40e/0x980
>  #2: ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at:
> filename_create+0xb6/0x260
>  #3: ffff8880057ce760 (&type->i_mutex_dir_key#3/1){+.+.}-{3:3}, at:
> filename_create+0x123/0x260
>
> stack backtrace:
> CPU: 1 PID: 32 Comm: kworker/1:1 Not tainted 6.6.0-rc5 #775
> Workqueue: ksmbd-io handle_ksmbd_work
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x4f/0x90
>  dump_stack+0x14/0x20
>  print_deadlock_bug+0x2f0/0x410
>  check_deadlock+0x26b/0x3b0
>  __lock_acquire+0xce2/0x1060
>  ? mark_lock.part.0+0xff/0x720
>  ? __pfx___lock_acquire+0x10/0x10
>  ? mark_held_locks+0x6b/0x90
>  lock_acquire.part.0+0x125/0x2d0
>  ? ksmbd_vfs_mkdir+0xe1/0x410
>  ? __pfx_lock_acquire.part.0+0x10/0x10
>  ? __kmem_cache_free+0x179/0x280
>  ? ____kasan_slab_free+0x15b/0x1d0
>  ? __kasan_slab_free+0x16/0x20
>  ? slab_free_freelist_hook+0xbc/0x180
>  lock_acquire+0x93/0x160
>  ? ksmbd_vfs_mkdir+0xe1/0x410
>  mnt_want_write+0x49/0x220
>  ? ksmbd_vfs_mkdir+0xe1/0x410
>  ksmbd_vfs_mkdir+0xe1/0x410
>  ? __pfx__printk+0x10/0x10
>  ? __pfx_ksmbd_vfs_mkdir+0x10/0x10
>  smb2_open+0x1064/0x38b0
>  ? __pfx___lock_acquire+0x10/0x10
>  ? __pfx_smb2_open+0x10/0x10
>  ? __lock_release+0x13f/0x290
>  ? smb2_validate_credit_charge+0x25d/0x360
>  ? __pfx___lock_release+0x10/0x10
>  ? do_raw_spin_lock+0x127/0x1c0
>  ? __pfx_do_raw_spin_lock+0x10/0x10
>  ? do_raw_spin_unlock+0xac/0x110
>  ? _raw_spin_unlock+0x22/0x50
>  ? smb2_validate_credit_charge+0x25d/0x360
>  ? ksmbd_smb2_check_message+0x3be/0x400
>  ? __pfx_ksmbd_smb2_check_message+0x10/0x10
>  __process_request+0x151/0x310
>  __handle_ksmbd_work+0x33c/0x520
>  ? __pfx___handle_ksmbd_work+0x10/0x10
>  handle_ksmbd_work+0x4a/0xd0
>  process_one_work+0x4a7/0x980
>  ? __pfx_process_one_work+0x10/0x10
>  ? assign_work+0xe1/0x120
>  worker_thread+0x365/0x570
>  ? __pfx_worker_thread+0x10/0x10
>  kthread+0x18d/0x1d0
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork+0x38/0x70
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork_asm+0x1b/0x30
>  </TASK>
>
> Fixes: 40b268d384a2 ("ksmbd: add mnt_want_write to ksmbd vfs functions")
> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> ---
>  fs/smb/server/vfs.c | 17 -----------------
>  1 file changed, 17 deletions(-)
>
> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> index b5a5e50fc9ca..ac49e295d4c3 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -173,10 +173,6 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const
> char *name, umode_t mode)
>  		return err;
>  	}
>
> -	err = mnt_want_write(path.mnt);
> -	if (err)
> -		goto out_err;
> -
>  	mode |= S_IFREG;
>  	err = vfs_create(mnt_idmap(path.mnt), d_inode(path.dentry),
>  			 dentry, mode, true);
> @@ -186,9 +182,7 @@ int ksmbd_vfs_create(struct ksmbd_work *work, const char
> *name, umode_t mode)
>  	} else {
>  		pr_err("File(%s): creation failed (err:%d)\n", name, err);
>  	}
> -	mnt_drop_write(path.mnt);
>
> -out_err:
>  	done_path_create(&path, dentry);
>  	return err;
>  }
> @@ -219,10 +213,6 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char
> *name, umode_t mode)
>  		return err;
>  	}
>
> -	err = mnt_want_write(path.mnt);
> -	if (err)
> -		goto out_err2;
> -
>  	idmap = mnt_idmap(path.mnt);
>  	mode |= S_IFDIR;
>  	err = vfs_mkdir(idmap, d_inode(path.dentry), dentry, mode);
> @@ -246,8 +236,6 @@ int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char
> *name, umode_t mode)
>  	}
>
>  out_err1:
minor nit: we can change it with our_err if removing out_err2.
> -	mnt_drop_write(path.mnt);
> -out_err2:
>  	done_path_create(&path, dentry);
>  	if (err)
>  		pr_err("mkdir(%s): creation failed (err:%d)\n", name, err);
> @@ -665,16 +653,11 @@ int ksmbd_vfs_link(struct ksmbd_work *work, const char
> *oldname,
>  		goto out3;
>  	}
>
> -	err = mnt_want_write(newpath.mnt);
> -	if (err)
> -		goto out3;
> -
>  	err = vfs_link(oldpath.dentry, mnt_idmap(newpath.mnt),
>  		       d_inode(newpath.dentry),
>  		       dentry, NULL);
>  	if (err)
>  		ksmbd_debug(VFS, "vfs_link failed err %d\n", err);
> -	mnt_drop_write(newpath.mnt);
>
>  out3:
>  	done_path_create(&newpath, dentry);
> --
> 2.34.1
>
>
Namjae Jeon Oct. 14, 2023, 3:31 a.m. UTC | #2
2023-10-13 2:43 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>:
> Running smb2.rename test from Samba smbtorture suite against a kernel built
> with lockdep triggers a "possible recursive locking detected" warning.
>
> This is because mnt_want_write() is called twice with no mnt_drop_write()
> in between:
>   -> ksmbd_vfs_mkdir()
>     -> ksmbd_vfs_kern_path_create()
>        -> kern_path_create()
>           -> filename_create()
>             -> mnt_want_write()
>        -> mnt_want_write()
>
> Fix this by removing the mnt_want_write/mnt_drop_write calls from vfs
> helpers that call kern_path_create().
>
> Full lockdep trace below:
>
> ============================================
> WARNING: possible recursive locking detected
> 6.6.0-rc5 #775 Not tainted
> --------------------------------------------
> kworker/1:1/32 is trying to acquire lock:
> ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at: ksmbd_vfs_mkdir+0xe1/0x410
>
> but task is already holding lock:
> ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at: filename_create+0xb6/0x260
>
> other info that might help us debug this:
>  Possible unsafe locking scenario:
>
>        CPU0
>        ----
>   lock(sb_writers#5);
>   lock(sb_writers#5);
>
>  *** DEADLOCK ***
>
>  May be due to missing lock nesting notation
>
> 4 locks held by kworker/1:1/32:
>  #0: ffff8880064e4138 ((wq_completion)ksmbd-io){+.+.}-{0:0}, at:
> process_one_work+0x40e/0x980
>  #1: ffff888005b0fdd0 ((work_completion)(&work->work)){+.+.}-{0:0}, at:
> process_one_work+0x40e/0x980
>  #2: ffff888005ac83f8 (sb_writers#5){.+.+}-{0:0}, at:
> filename_create+0xb6/0x260
>  #3: ffff8880057ce760 (&type->i_mutex_dir_key#3/1){+.+.}-{3:3}, at:
> filename_create+0x123/0x260
>
> stack backtrace:
> CPU: 1 PID: 32 Comm: kworker/1:1 Not tainted 6.6.0-rc5 #775
> Workqueue: ksmbd-io handle_ksmbd_work
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x4f/0x90
>  dump_stack+0x14/0x20
>  print_deadlock_bug+0x2f0/0x410
>  check_deadlock+0x26b/0x3b0
>  __lock_acquire+0xce2/0x1060
>  ? mark_lock.part.0+0xff/0x720
>  ? __pfx___lock_acquire+0x10/0x10
>  ? mark_held_locks+0x6b/0x90
>  lock_acquire.part.0+0x125/0x2d0
>  ? ksmbd_vfs_mkdir+0xe1/0x410
>  ? __pfx_lock_acquire.part.0+0x10/0x10
>  ? __kmem_cache_free+0x179/0x280
>  ? ____kasan_slab_free+0x15b/0x1d0
>  ? __kasan_slab_free+0x16/0x20
>  ? slab_free_freelist_hook+0xbc/0x180
>  lock_acquire+0x93/0x160
>  ? ksmbd_vfs_mkdir+0xe1/0x410
>  mnt_want_write+0x49/0x220
>  ? ksmbd_vfs_mkdir+0xe1/0x410
>  ksmbd_vfs_mkdir+0xe1/0x410
>  ? __pfx__printk+0x10/0x10
>  ? __pfx_ksmbd_vfs_mkdir+0x10/0x10
>  smb2_open+0x1064/0x38b0
>  ? __pfx___lock_acquire+0x10/0x10
>  ? __pfx_smb2_open+0x10/0x10
>  ? __lock_release+0x13f/0x290
>  ? smb2_validate_credit_charge+0x25d/0x360
>  ? __pfx___lock_release+0x10/0x10
>  ? do_raw_spin_lock+0x127/0x1c0
>  ? __pfx_do_raw_spin_lock+0x10/0x10
>  ? do_raw_spin_unlock+0xac/0x110
>  ? _raw_spin_unlock+0x22/0x50
>  ? smb2_validate_credit_charge+0x25d/0x360
>  ? ksmbd_smb2_check_message+0x3be/0x400
>  ? __pfx_ksmbd_smb2_check_message+0x10/0x10
>  __process_request+0x151/0x310
>  __handle_ksmbd_work+0x33c/0x520
>  ? __pfx___handle_ksmbd_work+0x10/0x10
>  handle_ksmbd_work+0x4a/0xd0
>  process_one_work+0x4a7/0x980
>  ? __pfx_process_one_work+0x10/0x10
>  ? assign_work+0xe1/0x120
>  worker_thread+0x365/0x570
>  ? __pfx_worker_thread+0x10/0x10
>  kthread+0x18d/0x1d0
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork+0x38/0x70
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork_asm+0x1b/0x30
>  </TASK>
>
> Fixes: 40b268d384a2 ("ksmbd: add mnt_want_write to ksmbd vfs functions")
> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
Applied it to #ksmbd-for-next-next after updating the patch.

Thanks for your patch!
diff mbox series

Patch

diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index b5a5e50fc9ca..ac49e295d4c3 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -173,10 +173,6 @@  int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
 		return err;
 	}
 
-	err = mnt_want_write(path.mnt);
-	if (err)
-		goto out_err;
-
 	mode |= S_IFREG;
 	err = vfs_create(mnt_idmap(path.mnt), d_inode(path.dentry),
 			 dentry, mode, true);
@@ -186,9 +182,7 @@  int ksmbd_vfs_create(struct ksmbd_work *work, const char *name, umode_t mode)
 	} else {
 		pr_err("File(%s): creation failed (err:%d)\n", name, err);
 	}
-	mnt_drop_write(path.mnt);
 
-out_err:
 	done_path_create(&path, dentry);
 	return err;
 }
@@ -219,10 +213,6 @@  int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
 		return err;
 	}
 
-	err = mnt_want_write(path.mnt);
-	if (err)
-		goto out_err2;
-
 	idmap = mnt_idmap(path.mnt);
 	mode |= S_IFDIR;
 	err = vfs_mkdir(idmap, d_inode(path.dentry), dentry, mode);
@@ -246,8 +236,6 @@  int ksmbd_vfs_mkdir(struct ksmbd_work *work, const char *name, umode_t mode)
 	}
 
 out_err1:
-	mnt_drop_write(path.mnt);
-out_err2:
 	done_path_create(&path, dentry);
 	if (err)
 		pr_err("mkdir(%s): creation failed (err:%d)\n", name, err);
@@ -665,16 +653,11 @@  int ksmbd_vfs_link(struct ksmbd_work *work, const char *oldname,
 		goto out3;
 	}
 
-	err = mnt_want_write(newpath.mnt);
-	if (err)
-		goto out3;
-
 	err = vfs_link(oldpath.dentry, mnt_idmap(newpath.mnt),
 		       d_inode(newpath.dentry),
 		       dentry, NULL);
 	if (err)
 		ksmbd_debug(VFS, "vfs_link failed err %d\n", err);
-	mnt_drop_write(newpath.mnt);
 
 out3:
 	done_path_create(&newpath, dentry);