On Tue, Jan 02, 2024 at 02:29:42AM -0800, Christoph Hellwig wrote:
> On Sun, Dec 31, 2023 at 12:07:18PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > An earlier version of this patch ("xfs: repair obviously broken inode
> > modes") tried to reset the di_mode of a file by guessing it from the
> > data fork format and/or data block 0 contents. Christoph didn't like
> > this approach because it opens the possibility that users could craft a
> > file to look like a directory and trick online repair into turning the
> > mode into S_IFDIR.
>
> I find the commit message here really weird. What I want doesn't
> matter. If what I say makes sense (I hope it does, if it doesn't please
> push back) then we should document thing based on the cross-checked
> facts and assumptions I provided. If not we should not be doing this
> at at all.
What you said back at [1] makes sense -- user controlled data blocks
should not be used to guess the inode mode. I yanked that patch and
replaced it with this one, which scans the inodes looking for a dirent
pointing down to the busted inode, and uses that to decide if the busted
file is S_IFDIR.
How about I rephrase the whole commit message like this:
"xfs: repair file modes by scanning for a dirent pointing to us
"Repair might encounter an inode with a totally garbage i_mode. To fix
this problem, we have to figure out if the file was a regular file, a
directory, or a special file. One way to figure this out is to check if
there are any directories with entries pointing down to the busted file.
"This patch recovers the file mode by scanning every directory entry on
the filesystem to see if there are any that point to the busted file.
If the ftype of all such dirents are consistent, the mode is recovered
from the ftype. If no dirents are found, the file becomes a regular
file. In all cases, ACLs are canceled and the file is made accessible
only by root.
"A previous patch attempted to guess the mode by reading the beginning
of the file data. This was rejected by Christoph on the grounds that we
cannot trust user-controlled data blocks. Users do not have direct
control over the ondisk contents of directory links, so this method
should be much safer."
--D
[1] https://lore.kernel.org/linux-xfs/ZXFhuNaLx1C8yYV+@xxxxxxxxxxxxx/
