linux-ext4 - Re: [PATCH 2/6] fs: report filesystem and file I/O errors to fsnotify

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251218184429.GX7725@frogsfrogsfrogs>
Date: Thu, 18 Dec 2025 10:44:29 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Christoph Hellwig <hch@...radead.org>
Cc: brauner@...nel.org, linux-ext4@...r.kernel.org, jack@...e.cz,
	linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	gabriel@...sman.be, amir73il@...il.com
Subject: Re: [PATCH 2/6] fs: report filesystem and file I/O errors to fsnotify

On Wed, Dec 17, 2025 at 09:21:52PM -0800, Christoph Hellwig wrote:
> >  	long					s_min_writeback_pages;
> > +
> > +	/* number of fserrors that are being sent to fsnotify/filesystems */
> > +	refcount_t		s_pending_errors;
> 
> Use the same tab-alignment as the fields above?  Also is this really

Will fix.

> a refcount?  It's a counter, but not really a reference?  I guess
> that doesn't matter too much.

<shrug> It does count pending events, but you could also look upon it as
a count of weak "references" to the super_block since the fserror_event
object does point to the super_block.

(Not sure what happens if we get more than 2^31 errors...)

> > +static inline void fserror_unmount(struct super_block *sb)
> > +{
> > +	/*
> > +	 * If we don't drop the pending error count to zero, then wait for it
> > +	 * to drop below 1, which means that the pending errors cleared or
> > +	 * that we saturated the system with 1 billion+ concurrent events.
> > +	 */
> > +	if (!refcount_dec_and_test(&sb->s_pending_errors))
> > +		wait_var_event(&sb->s_pending_errors,
> > +			       refcount_read(&sb->s_pending_errors) < 1);
> > +}
> 
> Should this be out of line?

Yes, it's better to hide these details (particularly since there's a fat
comment about the refcount bias) in fserror.c.

> > +/**
> > + * fserror_report - report a filesystem error of some kind
> > + *
> > + * Report details of a filesystem error to the super_operations::report_error
> > + * callback if present; and to fsnotify for distribution to userspace.  @sb,
> > + * @gfp, @type, and @error must all be specified.  For file I/O errors, the
> > + * @inode, @pos, and @len fields must also be specified.  For file metadata
> > + * errors, @inode must be specified.  If @inode is not NULL, then @inode->i_sb
> > + * must point to @sb.
> > + *
> > + * Reporting work is deferred to a workqueue to ensure that ->report_error is
> > + * called from process context without any locks held.  An active reference to
> > + * the inode is maintained until event handling is complete, and unmount will
> > + * wait for queued events to drain.
> > + *
> > + * @sb:		superblock of the filesystem
> 
> The normal convention is to have the arguments documented above the
> long description.  Any reason to deviate from that here?

Nope.  Fixed.

--D