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

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

[Darrick J. Wong]

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.  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.

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.

Fixes: 2442ee15bb1e ("xfs: eager inode attr fork init needs attr feature awareness")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index b699fa6ee3b64..b2aab91a86d30 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -870,7 +870,7 @@ xfs_init_new_inode(
 	 * this saves us from needing to run a separate transaction to set the
 	 * fork offset in the immediate future.
 	 */
-	if (init_xattrs && xfs_has_attr(mp)) {
+	if (init_xattrs && (xfs_has_attr(mp) || xfs_has_attr2(mp))) {
 		ip->i_forkoff = xfs_default_attroffset(ip) >> 3;
 		xfs_ifork_init_attr(ip, XFS_DINODE_FMT_EXTENTS, 0);
 	}

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

[Darrick J. Wong]

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.  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.
> 
> 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.
> 
> Fixes: 2442ee15bb1e ("xfs: eager inode attr fork init needs attr feature awareness")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_inode.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index b699fa6ee3b64..b2aab91a86d30 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -870,7 +870,7 @@ xfs_init_new_inode(
>  	 * this saves us from needing to run a separate transaction to set the
>  	 * fork offset in the immediate future.
>  	 */
> -	if (init_xattrs && xfs_has_attr(mp)) {
> +	if (init_xattrs && (xfs_has_attr(mp) || xfs_has_attr2(mp))) {

NAK, this patch is still not correct -- if we add an attr fork here, we
also have to xfs_add_attr().  ATTR protects attr forks in general,
whereas ATTR2 only protects dynamic fork sizes.

	if (init_xattrs && (xfs_has_attr(mp) || xfs_has_attr2(mp))) {
		ip->i_forkoff = xfs_default_attroffset(ip) >> 3;
		xfs_ifork_init_attr(ip, XFS_DINODE_FMT_EXTENTS, 0);

		if (!xfs_has_attr(mp)) {
			spin_lock(&mp->m_sb_lock);
			xfs_add_attr2(mp);
			spin_unlock(&mp->m_sb_lock);
			xfs_log_sb(tp);
		}
	}

--D

>  		ip->i_forkoff = xfs_default_attroffset(ip) >> 3;
>  		xfs_ifork_init_attr(ip, XFS_DINODE_FMT_EXTENTS, 0);
>  	}
> 

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

[Christoph Hellwig]

On Tue, Jun 18, 2024 at 06:06:22PM -0700, Darrick J. Wong wrote:
> NAK, this patch is still not correct -- if we add an attr fork here, we
> also have to xfs_add_attr().  ATTR protects attr forks in general,
> whereas ATTR2 only protects dynamic fork sizes.

Yes.  Note that I was kinda surprised we wouldn't always set the attr
bit by default in mkfs, but indeed we can create a file system without
attrs, which felt odd.

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

[Darrick J. Wong]

On Tue, Jun 18, 2024 at 10:25:49PM -0700, Christoph Hellwig wrote:
> On Tue, Jun 18, 2024 at 06:06:22PM -0700, Darrick J. Wong wrote:
> > NAK, this patch is still not correct -- if we add an attr fork here, we
> > also have to xfs_add_attr().  ATTR protects attr forks in general,
> > whereas ATTR2 only protects dynamic fork sizes.
> 
> Yes.  Note that I was kinda surprised we wouldn't always set the attr
> bit by default in mkfs, but indeed we can create a file system without
> attrs, which felt odd.

Yeah.  If you turn on parent pointers but don't use a protofile or turn
on metadir in mkfs, then the fs you get actually won't have /any/ parent
pointers in it at all.  One of Allison's mkfs patches actually would
turn on ATTR at format time, but the V5 sb verifier only requires ATTR2
(dynamic forkoff), not ATTR (a file has/had an attr fork).

I guess I should document that in xfs_format.h, eh? :)

--D

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

[Dave Chinner]

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....

> > 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?

> > 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...

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

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

[Darrick J. Wong]

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
> 

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

[Dave Chinner]

On Mon, Jul 01, 2024 at 04:37:49PM -0700, Darrick J. Wong wrote:
> 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.

Right, that's the oversight we need to correct. mkfs.xfs is already
setting it, so adding checks to the suerpblock verifier, repair, etc
isn't going to break anything.

> > > > 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.

No, it's not. The parent pointer feature bit it set, mkfs.xfs always
sets the ATTRBIT in that case, so the superblock feature bit checker
needs to error out if (PARENT | ATTRBIT) == PARENT.

This is basic on-disk format verification stuff, Darrick, and you
know this as well as I do.

> 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.

Which is a classic demonstration of an oversight. We're not
omniscent, and hindsight is wonderful for making things we missed
obvious.

Also, xfs_repair failing to catch something doesn't mean the
behaviour is correct or good. It just means we failed to consider
that specific failure mode and so haven't added the code to catch
it. Fixing this sort of oversight is exactly why we have
EXPERIMENTAL tags....

> 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.

But that's the issue: we should not be working around feature bit
bugs in core XFS code. PARENT is dependent on ATTRBIT being set, and
the whole point of our feature bit checks is to catch on-disk format
bugs like this at mount time. Ignoring the bad feature flag
combination and workign around them in core code is the antithesis
of the XFS on-disk format verification architecture.

The fact is that mount *should fail* if PARENT is set and ATTRBIT is
not. xfs_repair should flag this as an error and fix it. mkfs.xfs
has set both PARENT and ATTRBIT for a long long time, and so we
should be enforcing that config everywhere. We should not be working
around an invalid feature bit combination anywhere in the XFS code,
ever.

> > > > 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.

It's taken me a long time to calm down after reading these two
words.

One of the reasons we have an EXPERIMENTAL period for new features
is to provide scope for correcting on-disk format screwups like this
properly. This means we don't need to carry hacks around stupid
thinko's and straight out bugs in the on-disk format and initial
implementation forever.

We also work under the guidelines that it is largely the
responsibility of the engineer who asked for the new functionality
to be merged to do most of the heavy lifting to fix such issues
during the EXPERIMENTAL period.

Suggesting that someone else should do the work to tidy up the loose
on-disk format ends in the kernel and userspace in preference to a
one line hack that works around the problem goes against all the
agreements we've had for bringing experimental code up to production
quality. This isn't how to engineer high quality, robust long lived
code...

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

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

[Christoph Hellwig]

On Fri, Jul 12, 2024 at 10:31:57AM +1000, Dave Chinner wrote:
> Suggesting that someone else should do the work to tidy up the loose
> on-disk format ends in the kernel and userspace in preference to a
> one line hack that works around the problem goes against all the
> agreements we've had for bringing experimental code up to production
> quality. This isn't how to engineer high quality, robust long lived
> code...

I don't think it is entirely unreasonable.  The code works totally
fine by not forcing attr for parent as Darrick pointed out.  It's
also kinda suboptimal and stupid as you pointed out.  So if you care
about it, just sending a patch would be a lot nicer than ranting.