Re: [PATCH 02/12] xfs: improve dirent bounds checking in scrub and repair

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

On Sun, Sep 27, 2026 at 11:37:37PM -0700, Christoph Hellwig wrote:
> On Sun, Sep 27, 2026 at 11:16:20PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > LOLLM complains that we don't do enough bounds checking of the directory
> > entries in directory blocks when we're looking for errors or trying to
> > salvage entries.  Let's improve that with more detailed checks.
> > 
> > Cc: <stable@vger.kernel.org> # v4.16
> > Fixes: ce92d29ddf9908 ("xfs: directory scrubber must walk through data block to offset")
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > Assisted-by: LOLLM # finding obvious bugs
> > ---
> >  fs/xfs/scrub/dir.c        |   56 +++++++++++++++++++++++++++++++++++++++++----
> >  fs/xfs/scrub/dir_repair.c |   30 +++++++++++++++++++++++-
> >  2 files changed, 79 insertions(+), 7 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> > index a63eebf39fd496..dd21570378b281 100644
> > --- a/fs/xfs/scrub/dir.c
> > +++ b/fs/xfs/scrub/dir.c
> > @@ -340,6 +340,7 @@ xchk_dir_rec(
> >  	xfs_dahash_t			hash;
> >  	struct xfs_dir3_icleaf_hdr	hdr;
> >  	unsigned int			tag;
> > +	bool				foundit = false;
> >  	int				error;
> >  
> >  	ASSERT(blk->magic == XFS_DIR2_LEAF1_MAGIC ||
> > @@ -390,22 +391,60 @@ xchk_dir_rec(
> >  		xchk_fblock_set_corrupt(ds->sc, XFS_DATA_FORK, rec_bno);
> >  		goto out_relse;
> >  	}
> > -	for (;;) {
> > +	while (iter_off < end) {
> 
> Nit: maybe move the iter_off initialization just above this for
> clarify?

Ok.

> >  		struct xfs_dir2_data_entry	*dep = bp->b_addr + iter_off;
> >  		struct xfs_dir2_data_unused	*dup = bp->b_addr + iter_off;
> > +		unsigned int			advance;
> >  
> > -		if (iter_off >= end) {
> > +		/* must have freetag */
> > +		if (iter_off + offsetof(struct xfs_dir2_data_unused, length) >= end) {
> 
> Overly long line.

Will fix these.

		/* must have freetag */
		advance = offsetof(struct xfs_dir2_data_unused, length);
		if (offset + advance >= end)
			break;

> In general it feels like the inner body would benefit from being
> split into a helper for readability given how big it becomes.

I really wish C had a way to make it so that you could hoist just the
*dirent walking code* whilst retaining the custom bits of functionality
(each error handling, and finding dirents).  I don't know of a good way
to do that in C that doesn't involve cpp.

> That would also ease deduplicating the xchk_fblock_set_corrupt calls.

I prefer to keep those separate because xchk_*_set_corrupt contains a
tracepoint that captures the callsite, so you can use gdb or other tools
to go find the exact line in the source code that set the corruption
flag.

Originally I had a fugly macro that wrapped the exact metadata check so
that we could stringify it and record that directly in the tracepoint
buffer but enough people complained that I morphed it into what's there
now.

--D