]> git.dujemihanovic.xyz Git - linux.git/commitdiff
inotify: Fix possible deadlock in fsnotify_destroy_mark
authorLizhi Xu <lizhi.xu@windriver.com>
Fri, 27 Sep 2024 14:36:42 +0000 (22:36 +0800)
committerJan Kara <jack@suse.cz>
Wed, 2 Oct 2024 13:14:29 +0000 (15:14 +0200)
[Syzbot reported]
WARNING: possible circular locking dependency detected
6.11.0-rc4-syzkaller-00019-gb311c1b497e5 #0 Not tainted
------------------------------------------------------
kswapd0/78 is trying to acquire lock:
ffff88801b8d8930 (&group->mark_mutex){+.+.}-{3:3}, at: fsnotify_group_lock include/linux/fsnotify_backend.h:270 [inline]
ffff88801b8d8930 (&group->mark_mutex){+.+.}-{3:3}, at: fsnotify_destroy_mark+0x38/0x3c0 fs/notify/mark.c:578

but task is already holding lock:
ffffffff8ea2fd60 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat mm/vmscan.c:6841 [inline]
ffffffff8ea2fd60 (fs_reclaim){+.+.}-{0:0}, at: kswapd+0xbb4/0x35a0 mm/vmscan.c:7223

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (fs_reclaim){+.+.}-{0:0}:
       ...
       kmem_cache_alloc_noprof+0x3d/0x2a0 mm/slub.c:4044
       inotify_new_watch fs/notify/inotify/inotify_user.c:599 [inline]
       inotify_update_watch fs/notify/inotify/inotify_user.c:647 [inline]
       __do_sys_inotify_add_watch fs/notify/inotify/inotify_user.c:786 [inline]
       __se_sys_inotify_add_watch+0x72e/0x1070 fs/notify/inotify/inotify_user.c:729
       do_syscall_x64 arch/x86/entry/common.c:52 [inline]
       do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

-> #0 (&group->mark_mutex){+.+.}-{3:3}:
       ...
       __mutex_lock+0x136/0xd70 kernel/locking/mutex.c:752
       fsnotify_group_lock include/linux/fsnotify_backend.h:270 [inline]
       fsnotify_destroy_mark+0x38/0x3c0 fs/notify/mark.c:578
       fsnotify_destroy_marks+0x14a/0x660 fs/notify/mark.c:934
       fsnotify_inoderemove include/linux/fsnotify.h:264 [inline]
       dentry_unlink_inode+0x2e0/0x430 fs/dcache.c:403
       __dentry_kill+0x20d/0x630 fs/dcache.c:610
       shrink_kill+0xa9/0x2c0 fs/dcache.c:1055
       shrink_dentry_list+0x2c0/0x5b0 fs/dcache.c:1082
       prune_dcache_sb+0x10f/0x180 fs/dcache.c:1163
       super_cache_scan+0x34f/0x4b0 fs/super.c:221
       do_shrink_slab+0x701/0x1160 mm/shrinker.c:435
       shrink_slab+0x1093/0x14d0 mm/shrinker.c:662
       shrink_one+0x43b/0x850 mm/vmscan.c:4815
       shrink_many mm/vmscan.c:4876 [inline]
       lru_gen_shrink_node mm/vmscan.c:4954 [inline]
       shrink_node+0x3799/0x3de0 mm/vmscan.c:5934
       kswapd_shrink_node mm/vmscan.c:6762 [inline]
       balance_pgdat mm/vmscan.c:6954 [inline]
       kswapd+0x1bcd/0x35a0 mm/vmscan.c:7223

[Analysis]
The problem is that inotify_new_watch() is using GFP_KERNEL to allocate
new watches under group->mark_mutex, however if dentry reclaim races
with unlinking of an inode, it can end up dropping the last dentry reference
for an unlinked inode resulting in removal of fsnotify mark from reclaim
context which wants to acquire group->mark_mutex as well.

This scenario shows that all notification groups are in principle prone
to this kind of a deadlock (previously, we considered only fanotify and
dnotify to be problematic for other reasons) so make sure all
allocations under group->mark_mutex happen with GFP_NOFS.

Reported-and-tested-by: syzbot+c679f13773f295d2da53@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c679f13773f295d2da53
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
Link: https://patch.msgid.link/20240927143642.2369508-1-lizhi.xu@windriver.com
fs/nfsd/filecache.c
fs/notify/dnotify/dnotify.c
fs/notify/fanotify/fanotify_user.c
fs/notify/group.c
include/linux/fsnotify_backend.h

index 19bb88c7eebd9b7621398d46ff9a971b55037781..ea1ca374cdab019f28b15bbced915b0b3923e4c6 100644 (file)
@@ -792,7 +792,7 @@ nfsd_file_cache_init(void)
        }
 
        nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,
-                                                       FSNOTIFY_GROUP_NOFS);
+                                                       0);
        if (IS_ERR(nfsd_file_fsnotify_group)) {
                pr_err("nfsd: unable to create fsnotify group: %ld\n",
                        PTR_ERR(nfsd_file_fsnotify_group));
index 46440fbb8662d69fd5bdc49a88aaaeaf5c4fe27d..d5dbef7f5c95bb522c69b42982a0e728bc75b5f9 100644 (file)
@@ -406,8 +406,7 @@ static int __init dnotify_init(void)
                                          SLAB_PANIC|SLAB_ACCOUNT);
        dnotify_mark_cache = KMEM_CACHE(dnotify_mark, SLAB_PANIC|SLAB_ACCOUNT);
 
-       dnotify_group = fsnotify_alloc_group(&dnotify_fsnotify_ops,
-                                            FSNOTIFY_GROUP_NOFS);
+       dnotify_group = fsnotify_alloc_group(&dnotify_fsnotify_ops, 0);
        if (IS_ERR(dnotify_group))
                panic("unable to allocate fsnotify group for dnotify\n");
        dnotify_sysctl_init();
index 13454e5fd3fb424366342673de395040aa51d918..9644bc72e4573b18c523f212ec68d97a9386eb44 100644 (file)
@@ -1480,7 +1480,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 
        /* fsnotify_alloc_group takes a ref.  Dropped in fanotify_release */
        group = fsnotify_alloc_group(&fanotify_fsnotify_ops,
-                                    FSNOTIFY_GROUP_USER | FSNOTIFY_GROUP_NOFS);
+                                    FSNOTIFY_GROUP_USER);
        if (IS_ERR(group)) {
                return PTR_ERR(group);
        }
index 1de6631a3925ee99266fe5516de5c3669c8385b3..18446b7b0d495d64af470e0b598c6f2c492939ce 100644 (file)
@@ -115,7 +115,6 @@ static struct fsnotify_group *__fsnotify_alloc_group(
                                const struct fsnotify_ops *ops,
                                int flags, gfp_t gfp)
 {
-       static struct lock_class_key nofs_marks_lock;
        struct fsnotify_group *group;
 
        group = kzalloc(sizeof(struct fsnotify_group), gfp);
@@ -136,16 +135,6 @@ static struct fsnotify_group *__fsnotify_alloc_group(
 
        group->ops = ops;
        group->flags = flags;
-       /*
-        * For most backends, eviction of inode with a mark is not expected,
-        * because marks hold a refcount on the inode against eviction.
-        *
-        * Use a different lockdep class for groups that support evictable
-        * inode marks, because with evictable marks, mark_mutex is NOT
-        * fs-reclaim safe - the mutex is taken when evicting inodes.
-        */
-       if (flags & FSNOTIFY_GROUP_NOFS)
-               lockdep_set_class(&group->mark_mutex, &nofs_marks_lock);
 
        return group;
 }
index 8be029bc50b1e33f197b8e3be71983589f534778..3ecf7768e577940f9ea79393c38bc2550ce7ddb8 100644 (file)
@@ -217,7 +217,6 @@ struct fsnotify_group {
 
 #define FSNOTIFY_GROUP_USER    0x01 /* user allocated group */
 #define FSNOTIFY_GROUP_DUPS    0x02 /* allow multiple marks per object */
-#define FSNOTIFY_GROUP_NOFS    0x04 /* group lock is not direct reclaim safe */
        int flags;
        unsigned int owner_flags;       /* stored flags of mark_mutex owner */
 
@@ -268,22 +267,19 @@ struct fsnotify_group {
 static inline void fsnotify_group_lock(struct fsnotify_group *group)
 {
        mutex_lock(&group->mark_mutex);
-       if (group->flags & FSNOTIFY_GROUP_NOFS)
-               group->owner_flags = memalloc_nofs_save();
+       group->owner_flags = memalloc_nofs_save();
 }
 
 static inline void fsnotify_group_unlock(struct fsnotify_group *group)
 {
-       if (group->flags & FSNOTIFY_GROUP_NOFS)
-               memalloc_nofs_restore(group->owner_flags);
+       memalloc_nofs_restore(group->owner_flags);
        mutex_unlock(&group->mark_mutex);
 }
 
 static inline void fsnotify_group_assert_locked(struct fsnotify_group *group)
 {
        WARN_ON_ONCE(!mutex_is_locked(&group->mark_mutex));
-       if (group->flags & FSNOTIFY_GROUP_NOFS)
-               WARN_ON_ONCE(!(current->flags & PF_MEMALLOC_NOFS));
+       WARN_ON_ONCE(!(current->flags & PF_MEMALLOC_NOFS));
 }
 
 /* When calling fsnotify tell it if the data is a path or inode */