Received-SPF: pass (google.com: domain of linux-kernel+bounces-147340-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <00000000000095bb400615f4b0ed@google.com> <20240413084519.1774-1-hdanton@sina.com>
 <CAOQ4uxhh4Tm6j+Hh+F2aQFuHfpCh_kJ10FYTfXo+AxoP4m01ag@mail.gmail.com>
 <20240415140333.y44rk5ggbadv4oej@quack3> <CAOQ4uxiG_7HGESMNkrJ7QmsXbgOneUGpMjx8vob87kntwTzUTQ@mail.gmail.com>
 <20240416132207.idn7rjzq4d4rayaz@quack3> <CAOQ4uxjJK3YT1+s_OwtM+=p_C8RCvXaAm6v5V+atyqvRKuKp+g@mail.gmail.com>
 <20240416173211.4lnmgctyo4jn5fha@quack3>
In-Reply-To: <20240416173211.4lnmgctyo4jn5fha@quack3>
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 16 Apr 2024 20:55:46 +0300
Message-ID: <CAOQ4uxhQbWQ4hzrG2opRmL0pH5NfWBrkk0PG3aWAF3FODVpbfg@mail.gmail.com>
Subject: Re: [syzbot] Re: [syzbot] [ext4?] KASAN: slab-use-after-free Read in fsnotify
To: Jan Kara <jack@suse.cz>
Cc: Hillf Danton <hdanton@sina.com>, 
	syzbot <syzbot+5e3f9b2a67b45f16d4e6@syzkaller.appspotmail.com>, 
	linux-fsdevel@vger.kernel.org, syzkaller-bugs@googlegroups.com, 
	linux-kernel@vger.kernel.org, Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"

> > > > Maybe it is ok to let go of the optimization in fsnotify(), considering
> > > > that we now have stronger optimizations in the inline hooks and
> > > > in __fsnotify_parent()?
> > > >
> > > > I think that Hillf's patch is missing setting s_fsnotify_info to NULL?
> > > >
> > > >  @@ -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));
> > > > +       WRITE_ONCE(sb->s_fsnotify_info, NULL);
> > > > +       synchronize_srcu(&fsnotify_mark_srcu);
> > > >          kfree(sbinfo);
> > > >  }
> > >
> > > So I had a look into this. Yes, something like this should work. We'll see
> > > whether synchronize_srcu() won't slow down umount too much. If someone will
> > > complain, we'll have to find a better solution.
> > >
> >
> > Actually, kfree_rcu(sbinfo) may be enough.
> > We do not actually access sbinfo during mark iteration and
> > event handling, we only access it to get to the sb connector.
> >
> > Something like the attached patch?
>
> Hum, thinking about this some more - what if we just freed sb_info from
> destroy_super_work()? By then we definitely are not getting fsnotify()
> calls for the superblock so all the problems are solved.
>

Considering that this is the solution for security_sb_free()
I don't see why not have fsnotify_sb_free().
I'll prepare a patch.

Thanks!
Amir.