Re: [syzbot] Re: [syzbot] [ext4?] KASAN: slab-use-after-free Read in fsnotify

[syzbot <syzbot+5e3f9b2a67b45f16d4e6@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [syzbot] [ext4?] KASAN: slab-use-after-free Read in fsnotify
Author: amir73il@gmail.com

On Sat, Apr 13, 2024 at 4:41 AM Hillf Danton <hdanton@sina.com> 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.