diff options
author | Vincent Whitchurch <vincent.whitchurch@axis.com> | 2022-06-01 08:03:18 +0300 |
---|---|---|
committer | Steve French <stfrench@microsoft.com> | 2022-06-01 08:03:18 +0300 |
commit | cc391b694ff085f62f133e6b8f864d43a8e69dfd (patch) | |
tree | f0e4f32bac269c482937fa067ec661157f18bf73 /fs/cifs/cifsglob.h | |
parent | f66f8b94e7f2f4ac9fffe710be231ca8f25c5057 (diff) | |
download | linux-cc391b694ff085f62f133e6b8f864d43a8e69dfd.tar.xz |
cifs: fix potential deadlock in direct reclaim
The srv_mutex is used during writeback so cifs should ensure that
allocations done when that mutex is held are done with GFP_NOFS, to
avoid having direct reclaim ending up waiting for the same mutex and
causing a deadlock. This is detected by lockdep with the splat below:
======================================================
WARNING: possible circular locking dependency detected
5.18.0 #70 Not tainted
------------------------------------------------------
kswapd0/49 is trying to acquire lock:
ffff8880195782e0 (&tcp_ses->srv_mutex){+.+.}-{3:3}, at: compound_send_recv
but task is already holding lock:
ffffffffa98e66c0 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (fs_reclaim){+.+.}-{0:0}:
fs_reclaim_acquire
kmem_cache_alloc_trace
__request_module
crypto_alg_mod_lookup
crypto_alloc_tfm_node
crypto_alloc_shash
cifs_alloc_hash
smb311_crypto_shash_allocate
smb311_update_preauth_hash
compound_send_recv
cifs_send_recv
SMB2_negotiate
smb2_negotiate
cifs_negotiate_protocol
cifs_get_smb_ses
cifs_mount
cifs_smb3_do_mount
smb3_get_tree
vfs_get_tree
path_mount
__x64_sys_mount
do_syscall_64
entry_SYSCALL_64_after_hwframe
-> #0 (&tcp_ses->srv_mutex){+.+.}-{3:3}:
__lock_acquire
lock_acquire
__mutex_lock
mutex_lock_nested
compound_send_recv
cifs_send_recv
SMB2_write
smb2_sync_write
cifs_write
cifs_writepage_locked
cifs_writepage
shrink_page_list
shrink_lruvec
shrink_node
balance_pgdat
kswapd
kthread
ret_from_fork
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(fs_reclaim);
lock(&tcp_ses->srv_mutex);
lock(fs_reclaim);
lock(&tcp_ses->srv_mutex);
*** DEADLOCK ***
1 lock held by kswapd0/49:
#0: ffffffffa98e66c0 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat
stack backtrace:
CPU: 2 PID: 49 Comm: kswapd0 Not tainted 5.18.0 #70
Call Trace:
<TASK>
dump_stack_lvl
dump_stack
print_circular_bug.cold
check_noncircular
__lock_acquire
lock_acquire
__mutex_lock
mutex_lock_nested
compound_send_recv
cifs_send_recv
SMB2_write
smb2_sync_write
cifs_write
cifs_writepage_locked
cifs_writepage
shrink_page_list
shrink_lruvec
shrink_node
balance_pgdat
kswapd
kthread
ret_from_fork
</TASK>
Fix this by using the memalloc_nofs_save/restore APIs around the places
where the srv_mutex is held. Do this in a wrapper function for the
lock/unlock of the srv_mutex, and rename the srv_mutex to avoid missing
call sites in the conversion.
Note that there is another lockdep warning involving internal crypto
locks, which was masked by this problem and is visible after this fix,
see the discussion in this thread:
https://lore.kernel.org/all/20220523123755.GA13668@axis.com/
Link: https://lore.kernel.org/r/CANT5p=rqcYfYMVHirqvdnnca4Mo+JQSw5Qu12v=kPfpk5yhhmg@mail.gmail.com/
Reported-by: Shyam Prasad N <nspmangalore@gmail.com>
Suggested-by: Lars Persson <larper@axis.com>
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
Diffstat (limited to 'fs/cifs/cifsglob.h')
-rw-r--r-- | fs/cifs/cifsglob.h | 20 |
1 files changed, 19 insertions, 1 deletions
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 68da230c7f11..d589e687611d 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -16,6 +16,7 @@ #include <linux/mempool.h> #include <linux/workqueue.h> #include <linux/utsname.h> +#include <linux/sched/mm.h> #include <linux/netfs.h> #include "cifs_fs_sb.h" #include "cifsacl.h" @@ -628,7 +629,8 @@ struct TCP_Server_Info { unsigned int in_flight; /* number of requests on the wire to server */ unsigned int max_in_flight; /* max number of requests that were on wire */ spinlock_t req_lock; /* protect the two values above */ - struct mutex srv_mutex; + struct mutex _srv_mutex; + unsigned int nofs_flag; struct task_struct *tsk; char server_GUID[16]; __u16 sec_mode; @@ -743,6 +745,22 @@ struct TCP_Server_Info { #endif }; +static inline void cifs_server_lock(struct TCP_Server_Info *server) +{ + unsigned int nofs_flag = memalloc_nofs_save(); + + mutex_lock(&server->_srv_mutex); + server->nofs_flag = nofs_flag; +} + +static inline void cifs_server_unlock(struct TCP_Server_Info *server) +{ + unsigned int nofs_flag = server->nofs_flag; + + mutex_unlock(&server->_srv_mutex); + memalloc_nofs_restore(nofs_flag); +} + struct cifs_credits { unsigned int value; unsigned int instance; |