Received-SPF: pass (google.com: domain of linux-kernel+bounces-143726-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Hillf Danton <hdanton@sina.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	syzbot <syzbot+5e3f9b2a67b45f16d4e6@syzkaller.appspotmail.com>,
	syzkaller-bugs@googlegroups.com,
	linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [syzbot] [ext4?] KASAN: slab-use-after-free Read in fsnotify
Date: Sat, 13 Apr 2024 20:04:26 +0800
Message-Id: <20240413120426.1889-1-hdanton@sina.com>
In-Reply-To: <CAOQ4uxhh4Tm6j+Hh+F2aQFuHfpCh_kJ10FYTfXo+AxoP4m01ag@mail.gmail.com>
References: <00000000000095bb400615f4b0ed@google.com> <20240413084519.1774-1-hdanton@sina.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

On Sat, 13 Apr 2024 12:32:32 +0300 Amir Goldstein
> On Sat, Apr 13, 2024 at 11:45=E2=80=AFAM Hillf Danton <hdanton@sina.com> wrote:
> >
> > If you composed fix based on SB_ACTIVE that is cleared in
> > generic_shutdown_super() with &sb->s_umount held for write,
> > I wonder what simpler serialization than srcu you could
> > find/create in fsnotify.
> 
> As far as I can tell there is no need for serialisation.
> 
> The problem is that fsnotify_sb_error() can be called from the
> context of ->put_super() call from generic_shutdown_super().
> 
> It's true that in the repro the thread calling fsnotify_sb_error()
> in the worker thread running quota deferred work from put_super()
> but I think there are sufficient barriers for this worker thread to
> observer the cleared SB_ACTIVE flag.
> 
	do_exit				quota_release_workfn
	---				---
	cleanup_mnt()			ext4_release_dquot()
	__super_lock_excl(s);		__ext4_error()
	deactivate_locked_super(s);	fsnotify_sb_error()
	ext4_kill_sb()
	kill_block_super()
	generic_shutdown_super()
					if (!(sb->s_flags & SB_ACTIVE))
						return;
	sb->s_flags &= ~SB_ACTIVE;
	fsnotify_sb_delete()
					fsnotify()

Because of no sync like taking &sb->s_umount in the worker context,
checking SB_ACTIVE added in your fix is unable to close the race.

> Anyway, according to syzbot, repro does not trigger the UAF
> with my last fix.
> 
Note: testing is done by a robot and is best-effort only.