Re: [PATCH] xfs: honor init_xattrs in xfs_init_new_inode for !attr && attr2 fs

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

On Mon, Jul 01, 2024 at 11:27:09AM +1000, Dave Chinner wrote:
> On Tue, Jun 18, 2024 at 06:06:22PM -0700, Darrick J. Wong wrote:
> > On Tue, Jun 18, 2024 at 04:21:12PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > xfs_init_new_inode ignores the init_xattrs parameter for filesystems
> > > that have ATTR2 enabled but not ATTR.  As a result, the first file to be
> > > created by the kernel will not have an attr fork created to store acls.
> > > Storing that first acl will add ATTR to the superblock flags, so chances
> > > are nobody has noticed this previously.
> > > 
> > > However, this is disastrous on a filesystem with parent pointers because
> > > it requires that a new linkable file /must/ have a pre-existing attr
> > > fork.
> 
> How are we creating a parent pointer filesystem that doesn't have
> XFS_SB_VERSION_ATTRBIT set in it? I thought that mkfs.xfs was going
> to always set this....

<shrug> The first three versions didn't set ATTRBIT; somewhere between
v4 and v5 Allison changed mkfs to set ATTRBIT; and as of v13 it still
does.

That said, there aren't actually any parent pointers on an empty
filesystem because the root dir is empty and the rt/quota inode are
children of the superblock.  So, technically speaking, it's not
*required* for mkfs to set it, at least not until we start adding
metadir inodes.

At no point during the development of parent pointers has xfs_repair
ever validated that ATTRBIT is set if PARENT is enabled -- it only
checks that ATTRBIT is set if any attr forks are found.

> > > The preproduction version of mkfs.xfs have always set this, but
> > > as xfs_sb.c doesn't validate that pptrs filesystems have ATTR set, we
> > > must catch this case.
> 
> Which is sounds like it is supposed to - how is parent pointers
> getting enabled such that XFS_SB_VERSION_ATTRBIT is not actually
> set?

Someone who fuzzes the filesystem could turn off ATTRBIT on an empty fs.
That's a valid configuration since there are also no parent pointers.

Or at least I'm assuming it is since xfs_repair has never complained
about ATTRBIT being set on a filesystem with no attr forks, and nobody's
suggested adding that enforcement in the 6 years the parent pointer
series has been out for review.

Getting back to the story, if someone mounts that empty filesystem and
tries to create a directory structure, the fs blows up.  This patch
fixes that situation without adding more ways that mount can fail.

> > > Note that the sb verifier /does/ require ATTR2 for V5 filesystems, so we
> > > can fix both problems by amending xfs_init_new_inode to set up the attr
> > > fork for either ATTR or ATTR2.
> 
> True, but it still seems to me like we should be fixing mkfs.xfs and
> the superblock verifier to do the right thing given this is all
> still experimental and we're allowed to fix on-disk format bugs
> like this.
> 
> Can we add that to the superblock verifier so that the parent
> pointer filesystems will not mount if mkfs is not setting the 
> XFS_SB_VERSION_ATTRBIT correctly when the parent pointer feature is
> enabled?
> 
> Worst case is that early testers will need to use xfs_db to set the
> XFS_SB_VERSION_ATTRBIT and then the filesystem will mount again...

Patches welcome.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>