| Message ID | 173328106602.1145623.16395857710576941601.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/6] xfs: don't move nondir/nonreg temporary repair files to the metadir namespace | expand |
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.
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
On Wed, Dec 04, 2024 at 10:14:50PM -0800, Darrick J. Wong wrote: > 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. Maybe just write this down in a big fat comment?
On Wed, Dec 04, 2024 at 10:46:23PM -0800, Christoph Hellwig wrote: > On Wed, Dec 04, 2024 at 10:14:50PM -0800, Darrick J. Wong wrote: > > 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. > > Maybe just write this down in a big fat comment? Will do. --D
diff --git a/fs/xfs/scrub/tempfile.c b/fs/xfs/scrub/tempfile.c index dc3802c7f678ce..82ecbb654fbb39 100644 --- a/fs/xfs/scrub/tempfile.c +++ b/fs/xfs/scrub/tempfile.c @@ -204,6 +204,9 @@ xrep_tempfile_adjust_directory_tree( if (!sc->ip || !xfs_is_metadir_inode(sc->ip)) return 0; + if (!S_ISDIR(VFS_I(sc->tempip)->i_mode) && + !S_ISREG(VFS_I(sc->tempip)->i_mode)) + return 0; xfs_ilock(sc->tempip, XFS_IOLOCK_EXCL); sc->temp_ilock_flags |= XFS_IOLOCK_EXCL;