Re: [PATCH 1/6] iomap: report file IO errors to fsnotify

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Nov 06, 2025 at 11:13:45AM +0100, Jan Kara wrote:
> On Wed 05-11-25 11:41:38, Darrick J. Wong wrote:
> > On Wed, Nov 05, 2025 at 10:28:08AM -0800, Darrick J. Wong wrote:
> > > On Wed, Nov 05, 2025 at 03:24:41PM +0100, Jan Kara wrote:
> > > > On Wed 05-11-25 12:14:52, Amir Goldstein wrote:
> > > > > 
> > > > > ...
> > > > > We recently discovered that fsnotify_sb_error() calls are exposed to
> > > > > races with generic_shutdown_super():
> > > > > https://lore.kernel.org/linux-fsdevel/scmyycf2trich22v25s6gpe3ib6ejawflwf76znxg7sedqablp@ejfycd34xvpa/
> > > 
> > > Hrmm.  I've noticed that ever since I added this new patchset, I've been
> > > getting more instances of outright crashes in the timer code, or
> > > workqueue lockups.  I wonder if that UAF is what's going on here...
> > > 
> > > > > Will punting all FS_ERROR events to workqueue help to improve this
> > > > > situation or will it make it worse?
> > > > 
> > > > Worse. But you raise a really good point which I've missed during my
> > > > review. Currently there's nothing which synchronizes pending works with
> > > > superblock getting destroyed with obvious UAF issues already in
> > > > handle_sb_error().
> > > 
> > > I wonder, could __sb_error call get_active_super() to obtain an active
> > > reference to the sb, and then deactivate_super() it in the workqueue
> > > callback?  If we can't get an active ref then we presume that the fs is
> > > already shutting down and don't send the event.
> > 
> > ...and now that I've actually tried it, I realize that we can't actually
> > call get_active_super because it can sleep waiting for s_umount and
> > SB_BORN.  Maybe we could directly atomic_inc_not_zero(&sb->s_active)
> > adn trust that the caller has an active ref to the sb?  I think that's
> > true for anyone calling __sb_error with a non-null inode.
> 
> Well, the side-effects of holding active sb reference from some workqueue
> item tend to hit back occasionally (like when userspace assumes the device
> isn't used anymore but it in fact still is because of the active
> reference). Every time we tried something like this (last time it was with
> iouring I believe) some user came back and complained his setup broke. In
> this case it should be really rare but still I think it's better to avoid
> it if we can (plus I'm not sure what you'd like to do for __sb_error()
> callers that don't get the inode and thus active reference isn't really
> guaranteed - they still need the protection against umount so that
> handle_sb_error() can do the notifier callchain thing).
> 
> So I think a better solution might be that generic_shutdown_super() waits
> for pending error notifications after clearing SB_ACTIVE before umount
> proceeds further and __sb_error() just starts discarding new notifications
> as soon as we see SB_ACTIVE is clear.

<nod> Summarizing what we just talked about on the ext4 call--

Instead of grabbing an active reference to the sb, I'll instead fix
__sb_error to (a) ignore !BORN || !ACTIVE || DYING supers, and (b)
increment a counter in the sb whenever we queue an event, and decrement
it when the worker finishes with it.  generic_shutdown_super can then
wait (having just cleared ACTIVE) for the counter to hit zero before it
stops fsnotify and drops the dentry cache.

Or at least that's what I'll try today.

--D

> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>