Re: [PATCH 1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace

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

On Wed, Dec 04, 2024 at 12:24:38AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 03, 2024 at 07:02:29PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Only directories or regular files are allowed in the metadata directory
> > tree.  Don't move the repair tempfile to the metadir namespace if this
> > is not true; this will cause the inode verifiers to trip.
> 
> Shouldn't this be an error instead of silently returning?  Either way
> the function could probably use a lot more comments explaining what is
> doing and why.

The function opportunistically moves sc->tempip from the regular
directory tree to the metadata directory tree if sc->ip is part of the
metadata directory tree.  However, the scrub setup functions grab sc->ip
and create sc->tempip before we actually get around to checking if the
file is the right type for the scrubber.

IOWs, you can invoke the symlink scrubber with the file handle of a
subdirectory in the metadir.  xrep_setup_symlink will create a temporary
symlink file, xrep_tempfile_adjust_directory_tree will foolishly try to
set the METADATA flag on the temp symlink, which trips the inode
verifier in the inode item precommit, which shuts down the filesystem
when expensive checks are turned on.  If they're /not/ turned on, then
xchk_symlink will return ENOENT when it sees that it's been passed a
symlink.

I considered modifying xchk_setup_inode_contents to check the mode if
desired and return ENOENT to abort the scrub without calling
_adjust_directory_tree, but it seemed simpler to leave the tempfile code
inside tempfile.c.

<shrug> I'm ok doing it that way too.

--D