Re: [PATCH 6/6] xfs: don't livelock in scrub on a circular unlinked list

On Thu, Jul 16, 2026 at 10:24:21AM +0200, Christoph Hellwig wrote:
> On Wed, Jul 15, 2026 at 11:07:40PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <[email protected]>
> > 
> > LOLLM points out that online fsck can livelock if an unlinked inode list
> > contains a loop.  Use a bitmap to detect cycles.
> 
> Didn't we recently have some AI generated patch for the same thing?

Yes.

> Either way, this does actually look reasonable.  But also complex
> enough that I really want a test case that creates an image with
> such a corruption to test this case before we merge the kernel code
> as this is something that basically requires a maliciously crafted
> image, and we trade new otherwise untested code for a theoretical
> bug.

I'll try to work on that one today.

> > +static int
> > +xchk_iunlink_bucket(
> > +	struct xfs_scrub	*sc,
> > +	unsigned int		bucket,
> > +	xfs_agino_t		agino)
> 
> Maybe add a comment what this function tests?

/*
 * Walk the incore unlinked list for a particular AGI bucket to construct
 * the unlinked inode bitmap for later reconstruction of the unlinked list.
 * Returns 1 if we should keep checking, 0 to stop checking, or a negative
 * errno.
 */

> 
> > +{
> > +	struct xagino_bitmap	seen;
> > +	struct xfs_inode	*ip;
> > +	int			ret = 0;
> > +
> > +	xagino_bitmap_init(&seen);
> > +
> > +	while (agino != NULLAGINO) {
> > +		unsigned int	len = 1;
> > +
> > +		if (agino % XFS_AGI_UNLINKED_BUCKETS != bucket) {
> > +			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
> > +			goto bad;
> > +		}
> 
> Handle entries that should not be here, makes sense.
> 
> > +
> > +		if (xagino_bitmap_test(&seen, agino, &len)) {
> > +			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
> > +			goto bad;
> > +		}
> 
> Check that we don't have duplicates, makes sense.
> 
> > +		ip = xfs_iunlink_lookup(sc->sa.pag, agino);
> > +		if (!ip) {
> > +			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
> > +			goto bad;
> > +		}
> > +
> > +		if (!xfs_inode_on_unlinked_list(ip)) {
> > +			xchk_block_set_corrupt(sc, sc->sa.agi_bp);
> > +			goto bad;
> > +		}
> 
> The that that the inode actually is on the unlinked list, makes
> sense.
> 
> >  	for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++) {
> > -		xfs_agino_t	agino = be32_to_cpu(agi->agi_unlinked[i]);
> > +		int		ret;
> >  
> > -		while (agino != NULLAGINO) {
> > -			if (agino % XFS_AGI_UNLINKED_BUCKETS != i) {
> > -				xchk_block_set_corrupt(sc, sc->sa.agi_bp);
> > -				return;
> > -			}
> 
> Ahh, and this is mostly existing code...
> 
> Maybe split the reactoring into a separate helper into a prep patch?

Ok will do.

--D