Re: [PATCH] xfs: allow SECURE namespace xattrs to use reserved pool

[Dave Chinner <david@fromorbit.com>]

On Mon, Jul 22, 2024 at 10:05:03AM -0500, Eric Sandeen wrote:
> On 7/22/24 9:41 AM, Christoph Hellwig wrote:
> > On Fri, Jul 19, 2024 at 05:48:53PM -0500, Eric Sandeen wrote:
> >>  	xfs_attr_sethash(args);
> >>  
> >> -	return xfs_attr_set(args, op, args->attr_filter & XFS_ATTR_ROOT);
> >> +	rsvd = args->attr_filter & (XFS_ATTR_ROOT | XFS_ATTR_SECURE);
> >> +	return xfs_attr_set(args, op, rsvd);
> > 
> > This looks fine, although I'd probably do without the extra local
> > variable.  More importantly though, please write a comment documenting
> > why we are dipping into the reserved pool here.  We should have had that
> > since the beginning, but this is a better time than never.
> > 
> > 
> 
> Ok, I thought the local var was a little prettier but *shrug* can do it
> either way.
> 
> To be honest I'm not sure why it was done for ROOT; dchinnner mentioned
> something about DMAPI requirements, long ago...

Because the xattrs created with inode allocation are not atomic
(which they could be now because parent pointers added the
infrastructure to add xattrs atomically in the create transaction),
stuff like ACLs, security xattrs and, historically, DMAPI xattrs
could fail to be created when the inode was allocated.

For DMAPI/DMF, this was a big issue if the xattr creation got ENOSPC
or the system crashed between inode creation (i.e the DMAPI CREATE
notification being processed by DMF) and the xattr being written on
the newly allocated inode. This would leave leave "untracked" inodes
in the filesystem, and the only way DMF could discover inodes
lacking in DMAPI xattrs was to run a full filesystem DMAPI-bulkstat
scan to synchronise the filesystem state with the DMF database held
in userspace. When you're tracking hundreds of millions to billions
of inodes, being forced to do a full fs inode scan after crashes or
ENOSPC before everything works properly again is, well, kinda
annoying.

Similar issues afflicted Trix (Trusted Irix) where security xattrs
(such as ACLs) went missing on crash or ENOSPC. On Irix, they were
stored in the XFS_ATTR_ROOT namespace, and the use of reserved block
space for XFS_ATTR_ROOT was introduced in 1997 on Irix.

commit 32d7e9a0d0fbca91a3d036c8518a87e10abfafb3
Author: gnuss <gnuss>
Date:   Fri Dec 19 19:35:42 1997 +0000

    pv: 553766 rv: lord@cray.com
    Add reserved flag param to routines in block allocation call sequence

This commit contains just the addition of XFS_TRANS_RESERVE for
XFS_ATTR_ROOT xattrs. Nothing else used it - this was specically a
fix for ACL/DMAPI xattr creation at ENOSPC....

However, ACL support on linux, and hence XFS_ATTR_SECURE, didn't
exist until 2004:

commit af80e14283d9475582dfb2d91395b674b9827fa8
Author: Nathan Scott <nathans@sgi.com>
Date:   Thu Jan 29 03:56:41 2004 +0000

    Add the security extended attributes namespace.

This added the XFS_ATTR_SECURE namespace because ACLs are in a
different xattr namespace in Linux (i.e. TRUSTED -> XFS_ATTR_ROOT,
SECURITY -> XFS_ATTR_SECURE), but the xattr set/change code never
added the XFS_ATTR_SECURE flag to the XFS_TRANS_RESERVE case.

It wasn't until 2007 that we started to use the reserve block pool
for other ENOSPC avoidance cases (like indirect delalloc BMBT block
reservation exhaustion in writeback) here:

commit bdebc6a4aca2ac056b8174f5b6a3bf27b28f6a5d
Author: Dave Chinner <dgc@sgi.com>
Date:   Fri Jun 8 16:03:59 2007 +0000

    Prevent ENOSPC from aborting transactions that need to succeed

So, essentially, for the first 10 years of it's life,
XFS_TRANS_RESERVE was used supposed to be to prevent ENOSPC at inode
creation for security and trusted xattrs....

> It seems reasonable, and it's been there forever but also not
> obviously required, AFAICT.

In hindsight, it looks to me like this was an oversight made back
in 2004 when XFS_ATTR_SECURE was added to linux for security
xattrs. As Christoph says: "it should have been there since the
beginning".

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