On Sat, Apr 13, 2024 at 4:41 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
> On Thu, 11 Apr 2024 01:11:20 -0700
> > syzbot found the following issue on:
> >
> > HEAD commit: 6ebf211bb11d Add linux-next specific files for 20240410
> > git tree: linux-next
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1621af9d180000
>
> #syz test https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git 6ebf211bb11d
>
> --- x/fs/notify/fsnotify.c
> +++ y/fs/notify/fsnotify.c
> @@ -101,8 +101,8 @@ void fsnotify_sb_delete(struct super_blo
> wait_var_event(fsnotify_sb_watched_objects(sb),
> !atomic_long_read(fsnotify_sb_watched_objects(sb)));
> WARN_ON(fsnotify_sb_has_priority_watchers(sb, FSNOTIFY_PRIO_CONTENT));
> - WARN_ON(fsnotify_sb_has_priority_watchers(sb,
> - FSNOTIFY_PRIO_PRE_CONTENT));
> + WARN_ON(fsnotify_sb_has_priority_watchers(sb, FSNOTIFY_PRIO_PRE_CONTENT));
> + synchronize_srcu(&fsnotify_mark_srcu);
> kfree(sbinfo);
> }
>
> @@ -499,7 +499,7 @@ int fsnotify(__u32 mask, const void *dat
> {
> const struct path *path = fsnotify_data_path(data, data_type);
> struct super_block *sb = fsnotify_data_sb(data, data_type);
> - struct fsnotify_sb_info *sbinfo = fsnotify_sb_info(sb);
> + struct fsnotify_sb_info *sbinfo;
> struct fsnotify_iter_info iter_info = {};
> struct mount *mnt = NULL;
> struct inode *inode2 = NULL;
> @@ -529,6 +529,8 @@ int fsnotify(__u32 mask, const void *dat
> inode2_type = FSNOTIFY_ITER_TYPE_PARENT;
> }
>
> + iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
> + sbinfo = fsnotify_sb_info(sb);
> /*
> * Optimization: srcu_read_lock() has a memory barrier which can
> * be expensive. It protects walking the *_fsnotify_marks lists.
See comment above. This kills the optimization.
It is not worth letting all the fsnotify hooks suffer the consequence
for the edge case of calling fsnotify hook during fs shutdown.
Also, fsnotify_sb_info(sb) in fsnotify_sb_has_priority_watchers()
is also not protected and using srcu_read_lock() there completely
nullifies the purpose of fsnotify_sb_info.
Here is a simplified fix for fsnotify_sb_error() rebased on the
pending mm fixes for this syzbot boot failure:
#syz test: https://github.com/amir73il/linux fsnotify-fixes
Jan,
I think that all the functions called from fs shutdown context
should observe that SB_ACTIVE is cleared but wasn't sure?
Thanks,
Amir.
From 9e5897865c4ba8296a81f451d2463dbd6b49fb3c Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Thu, 11 Apr 2024 18:59:08 +0300
Subject: [PATCH] fsnotify: do not handle events on a shutting down filesystem
Protect against use after free when filesystem calls fsnotify_sb_error()
during fs shutdown.
Reported-by: syzbot+5e3f9b2a67b45f16d4e6@xxxxxxxxxxxxxxxxxxxxxxxxx
Fixes: 07a3b8d0bf72 ("fsnotify: lazy attach fsnotify_sb_info state to sb")
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
include/linux/fsnotify.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 4da80e92f804..66512a965824 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -453,6 +453,10 @@ static inline int fsnotify_sb_error(struct super_block *sb, struct inode *inode,
.sb = sb,
};
+ /* fs may be calling this hook from without shutdown */
+ if (unlikely(!(sb->s_flags & SB_ACTIVE)))
+ return 0;
+
return fsnotify(FS_ERROR, &report, FSNOTIFY_EVENT_ERROR,
NULL, NULL, NULL, 0);
}
--
2.34.1
