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

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

[Eric Sandeen]

We got a report from the podman folks that selinux relabels that happen
as part of their process were returning ENOSPC when the filesystem is
completely full. This is because xattr changes reserve about 15 blocks
for the worst case, but the common case is for selinux contexts to be
the sole, in-inode xattr and consume no blocks.

We already allow reserved space consumption for XFS_ATTR_ROOT for things
such as ACLs, and selinux / SECURE attributes are not so very different,
so allow them to use the reserved space as well.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index ab3d22f662f2..e59193609003 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -82,6 +82,7 @@ xfs_attr_change(
 {
 	struct xfs_mount	*mp = args->dp->i_mount;
 	int			error;
+	bool			rsvd;
 
 	if (xfs_is_shutdown(mp))
 		return -EIO;
@@ -110,7 +111,8 @@ xfs_attr_change(
 	args->whichfork = XFS_ATTR_FORK;
 	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);
 }
 
 

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

[Christoph Hellwig]

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.

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

[Eric Sandeen]

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

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

What would your explanation be? ;) 

Thanks,
-Eric

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

[Christoph Hellwig]

On Mon, Jul 22, 2024 at 10:05:03AM -0500, Eric Sandeen wrote:
> 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...
> 
> It seems reasonable, and it's been there forever but also not obviously
> required, AFAICT.
> 
> What would your explanation be? ;) 

Based on your explanation it's probably ACLs for the same reason it
applies to the security attributes.

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

[mark.tinguely]

On 7/22/24 10:05 AM, 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...


The older Data Mover Framework (DMF v6) kept an extended attribute that 
denoted the file status (online/offline/partial online) and some region 
information (which was never used). Yeah DMF uses Data Management API 
(DMAPI) as the hooks to move data in when offline.

>
> It seems reasonable, and it's been there forever but also not obviously
> required, AFAICT.
>
> What would your explanation be? ;)
>
> Thanks,
> -Eric
>

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

[Eric Sandeen]

We got a report from the podman folks that selinux relabels that happen
as part of their process were returning ENOSPC when the filesystem is
completely full. This is because xattr changes reserve about 15 blocks
for the worst case, but the common case is for selinux contexts to be
the sole, in-inode xattr and consume no blocks.

We already allow reserved space consumption for XFS_ATTR_ROOT for things
such as ACLs, and selinux / SECURE attributes are not so very different,
so allow them to use the reserved space as well.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Remove local variable, add comment.

diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index ab3d22f662f2..09f004af7672 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -110,7 +110,16 @@ xfs_attr_change(
 	args->whichfork = XFS_ATTR_FORK;
 	xfs_attr_sethash(args);
 
-	return xfs_attr_set(args, op, args->attr_filter & XFS_ATTR_ROOT);
+	/*
+	 * Allow xattrs for ACLs (ROOT namespace) and SELinux contexts
+	 * (SECURE namespace) to use the reserved block pool for these
+	 * security-related operations. xattrs typically reside in the inode,
+	 * so in many cases the reserved pool won't actually get consumed,
+	 * but this will help the worst-case transaction reservations to
+	 * succeed.
+	 */
+	return xfs_attr_set(args, op,
+		    args->attr_filter & (XFS_ATTR_ROOT | XFS_ATTR_SECURE));
 }
 
 

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

[Dave Chinner]

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

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

[Dave Chinner]

On Mon, Jul 22, 2024 at 02:25:33PM -0500, Eric Sandeen wrote:
> We got a report from the podman folks that selinux relabels that happen
> as part of their process were returning ENOSPC when the filesystem is
> completely full. This is because xattr changes reserve about 15 blocks
> for the worst case, but the common case is for selinux contexts to be
> the sole, in-inode xattr and consume no blocks.
> 
> We already allow reserved space consumption for XFS_ATTR_ROOT for things
> such as ACLs, and selinux / SECURE attributes are not so very different,
> so allow them to use the reserved space as well.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: Remove local variable, add comment.
> 
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index ab3d22f662f2..09f004af7672 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -110,7 +110,16 @@ xfs_attr_change(
>  	args->whichfork = XFS_ATTR_FORK;
>  	xfs_attr_sethash(args);
>  
> -	return xfs_attr_set(args, op, args->attr_filter & XFS_ATTR_ROOT);
> +	/*
> +	 * Allow xattrs for ACLs (ROOT namespace) and SELinux contexts

It's not just SELinux - it's security xattrs set by LSMs in general
that use the SECURE namespace. These come through:

xfs_generic_create()
  xfs_inode_init_security()
    security_inode_init_security()
      <LSM>
        xfs_initxattrs()
          xfs_attr_change(XFS_ATTR_SECURE)

> +	 * (SECURE namespace) to use the reserved block pool for these
> +	 * security-related operations. xattrs typically reside in the inode,
> +	 * so in many cases the reserved pool won't actually get consumed,
> +	 * but this will help the worst-case transaction reservations to
> +	 * succeed.
> +	 */

It doesn't explain why we need this - it's got the what and the
expected behaviour, but no why. :)

> +	return xfs_attr_set(args, op,
> +		    args->attr_filter & (XFS_ATTR_ROOT | XFS_ATTR_SECURE));
>  }

Perhaps it would be better to say something like:

	/*
	 * Some xattrs must be resistent to allocation failure at
	 * ENOSPC. e.g. creating an inode with ACLs or security
	 * attributes requires the allocation of the xattr holding
	 * that information to succeed. Hence we allow xattrs in the
	 * VFS TRUSTED, SYSTEM, POSIX_ACL and SECURITY (LSM xattr)
	 * namespaces to dip into the reserve block pool to allow
	 * manipulation of these xattrs when at ENOSPC. These VFS
	 * xattr namespaces translate to the XFS_ATTR_ROOT and
	 * XFS_ATTR_SECURE on-disk namespaces.
	 *
	 * For most of these cases, these special xattrs will fit in
	 * the inode itself and so consume no extra space or only
	 * require temporary extra space while an overwrite is being
	 * made. Hence the use of the reserved pool is largely to
	 * avoid the worst case reservation from preventing the
	 * xattr from being created at ENOSPC.
	 */

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

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

[Eric Sandeen]

We got a report from the podman folks that selinux relabels that happen
as part of their process were returning ENOSPC when the filesystem is
completely full. This is because xattr changes reserve about 15 blocks
for the worst case, but the common case is for selinux contexts to be
the sole, in-inode xattr and consume no blocks.

We already allow reserved space consumption for XFS_ATTR_ROOT for things
such as ACLs, and SECURE namespace attributes are not so very different,
so allow them to use the reserved space as well.

Code-comment-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Remove local variable, add comment.
V3: Add Dave's preferred comment

diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index ab3d22f662f2..85e7be094943 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -110,7 +110,26 @@ xfs_attr_change(
 	args->whichfork = XFS_ATTR_FORK;
 	xfs_attr_sethash(args);
 
-	return xfs_attr_set(args, op, args->attr_filter & XFS_ATTR_ROOT);
+	/*
+	 * Some xattrs must be resistent to allocation failure at
+	 * ENOSPC. e.g. creating an inode with ACLs or security
+	 * attributes requires the allocation of the xattr holding
+	 * that information to succeed. Hence we allow xattrs in the
+	 * VFS TRUSTED, SYSTEM, POSIX_ACL and SECURITY (LSM xattr)
+	 * namespaces to dip into the reserve block pool to allow
+	 * manipulation of these xattrs when at ENOSPC. These VFS
+	 * xattr namespaces translate to the XFS_ATTR_ROOT and
+	 * XFS_ATTR_SECURE on-disk namespaces.
+	 *
+	 * For most of these cases, these special xattrs will fit in
+	 * the inode itself and so consume no extra space or only
+	 * require temporary extra space while an overwrite is being
+	 * made. Hence the use of the reserved pool is largely to
+	 * avoid the worst case reservation from preventing the
+	 * xattr from being created at ENOSPC.
+	 */
+	return xfs_attr_set(args, op,
+			args->attr_filter & (XFS_ATTR_ROOT | XFS_ATTR_SECURE));
 }
 
 

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

[Darrick J. Wong]

On Tue, Jul 23, 2024 at 09:59:41AM -0500, Eric Sandeen wrote:
> We got a report from the podman folks that selinux relabels that happen
> as part of their process were returning ENOSPC when the filesystem is
> completely full. This is because xattr changes reserve about 15 blocks
> for the worst case, but the common case is for selinux contexts to be
> the sole, in-inode xattr and consume no blocks.
> 
> We already allow reserved space consumption for XFS_ATTR_ROOT for things
> such as ACLs, and SECURE namespace attributes are not so very different,
> so allow them to use the reserved space as well.
> 
> Code-comment-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: Remove local variable, add comment.
> V3: Add Dave's preferred comment
> 
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index ab3d22f662f2..85e7be094943 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -110,7 +110,26 @@ xfs_attr_change(
>  	args->whichfork = XFS_ATTR_FORK;
>  	xfs_attr_sethash(args);
>  
> -	return xfs_attr_set(args, op, args->attr_filter & XFS_ATTR_ROOT);
> +	/*
> +	 * Some xattrs must be resistent to allocation failure at

Nit: resistant

> +	 * ENOSPC. e.g. creating an inode with ACLs or security
> +	 * attributes requires the allocation of the xattr holding
> +	 * that information to succeed. Hence we allow xattrs in the
> +	 * VFS TRUSTED, SYSTEM, POSIX_ACL and SECURITY (LSM xattr)
> +	 * namespaces to dip into the reserve block pool to allow
> +	 * manipulation of these xattrs when at ENOSPC. These VFS
> +	 * xattr namespaces translate to the XFS_ATTR_ROOT and
> +	 * XFS_ATTR_SECURE on-disk namespaces.
> +	 *
> +	 * For most of these cases, these special xattrs will fit in
> +	 * the inode itself and so consume no extra space or only
> +	 * require temporary extra space while an overwrite is being
> +	 * made. Hence the use of the reserved pool is largely to
> +	 * avoid the worst case reservation from preventing the
> +	 * xattr from being created at ENOSPC.
> +	 */
> +	return xfs_attr_set(args, op,
> +			args->attr_filter & (XFS_ATTR_ROOT | XFS_ATTR_SECURE));

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  }
>  
>  
> 
> 
> 

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

[Christoph Hellwig]

> +	/*
> +	 * Some xattrs must be resistent to allocation failure at
> +	 * ENOSPC. e.g. creating an inode with ACLs or security
> +	 * attributes requires the allocation of the xattr holding
> +	 * that information to succeed. Hence we allow xattrs in the

If you repin this anyway because of the spelling nit maybe use up all
80 characters to make the comment a bit easier to read.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Eric Sandeen]

We got a report from the podman folks that selinux relabels that happen
as part of their process were returning ENOSPC when the filesystem is
completely full. This is because xattr changes reserve about 15 blocks
for the worst case, but the common case is for selinux contexts to be
the sole, in-inode xattr and consume no blocks.

We already allow reserved space consumption for XFS_ATTR_ROOT for things
such as ACLs, and SECURE namespace attributes are not so very different,
so allow them to use the reserved space as well.

Code-comment-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---

V2: Remove local variable, add comment.
V3: Add Dave's preferred comment
V4: Spelling and comment beautification

diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index ab3d22f662f2..eaf849260bd6 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -110,7 +110,24 @@ xfs_attr_change(
 	args->whichfork = XFS_ATTR_FORK;
 	xfs_attr_sethash(args);
 
-	return xfs_attr_set(args, op, args->attr_filter & XFS_ATTR_ROOT);
+	/*
+	 * Some xattrs must be resistant to allocation failure at ENOSPC, e.g.
+	 * creating an inode with ACLs or security attributes requires the
+	 * allocation of the xattr holding that information to succeed. Hence
+	 * we allow xattrs in the VFS TRUSTED, SYSTEM, POSIX_ACL and SECURITY
+	 * (LSM xattr) namespaces to dip into the reserve block pool to allow
+	 * manipulation of these xattrs when at ENOSPC. These VFS xattr
+	 * namespaces translate to the XFS_ATTR_ROOT and XFS_ATTR_SECURE on-disk
+	 * namespaces.
+	 *
+	 * For most of these cases, these special xattrs will fit in the inode
+	 * itself and so consume no extra space or only require temporary extra
+	 * space while an overwrite is being made. Hence the use of the reserved
+	 * pool is largely to avoid the worst case reservation from preventing
+	 * the xattr from being created at ENOSPC.
+	 */
+	return xfs_attr_set(args, op,
+			args->attr_filter & (XFS_ATTR_ROOT | XFS_ATTR_SECURE));
 }