xfs: fix capabily check in xfs_setattr_nonsize

PATCH In review [email protected] 4 participants 9 messages Jun 24 – Jun 26

Subsystems (3)

Stable branch VFS XFS filesystem

lore

Share & tools

lore

[email protected]

author maintainer

2026-06-24 10:15 UTC #0 lore raw

PATCH 1/1 xfs: fix capabily check in xfs_setattr_nonsize

From: Carlos Maiolino <[email protected]>

An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.

Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:

capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials

This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.

Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.

Fixes eba0549bc7d1 ("xfs: don't generate selinux audit messages for capability testing")

Cc <[email protected]> # v5.18

Cc Darrick J. Wong <[email protected]>

Cc Dave Chinner <[email protected]>

Cc Eric Sandeen <[email protected]>

Cc Christoph Hellwig <[email protected]>

Reported-by Dr. Thomas Orgis <[email protected]>

Signed-off-by Carlos Maiolino <[email protected]>

---

If people do agree with the fix I do plan to send a patch to kernel/capability.c to ad a new capable_noaudit() helper which would come in handy here, but I believe we should backport this all the way back to 5.18 and replacing it by ns_capable_noaudit() is the easiest way to do it. Then if capable_noaudit() is acceptable we could just call this instead. This should also be a test in xfstests.

The patch is still running on my testing suite, I'm sending it ahead of having the testing finished for discussion/review, so for now it fixes the problem but I am not sure it doesn't break anything else :)

 fs/xfs/xfs_iops.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/xfs/xfs_iops.c +3 -1

index 325c2200c501..df0eba26dda3 100644
@@ -792,6 +792,8 @@ xfs_setattr_nonsize(
 	kgid_t			gid = GLOBAL_ROOT_GID;
 	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
 	struct xfs_dquot	*old_udqp = NULL, *old_gdqp = NULL;
+	bool			force = ns_capable_noaudit(&init_user_ns,
+							   CAP_FOWNER);
 
 	ASSERT((mask & ATTR_SIZE) == 0);

@@ -835,7 +837,7 @@ xfs_setattr_nonsize(
 	}
 
 	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
-			has_capability_noaudit(current, CAP_FOWNER), &tp);
+			force, &tp);
 	if (error)
 		goto out_dqrele;

Signature · 1 line

2.54.0

Christoph Hellwig

2026-06-24 13:40 UTC #1 lore raw

Adding Jan and Christian for quota and user_ns knowledge.

Quoted 7 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:

From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.

Quoted 7 lines

Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials

Eww..

Quoted 4 lines

This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.

Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?

Quoted 2 lines

Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.

The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 796

Quoted 2 lines

fs/xfs/xfs_iops.c +2 -0

+	bool			force = ns_capable_noaudit(&init_user_ns,
+							   CAP_FOWNER);

I'd do away with the local variable here.

Carlos Maiolino

author maintainer

2026-06-24 19:00 UTC #2 lore raw

Quoted 20 lines · Christoph Hellwig

Adding Jan and Christian for quota and user_ns knowledge.
Quoted 7 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:
From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.
Quoted 7 lines
Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials
Eww..

Quoted 7 lines

Quoted 4 lines
This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.
Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?

I don't know yet, but I do hope to be able to convert everything back to a capable_noaudit() helper. I need some time to look at the other conversions, but giving the different credentials likely the are all victim of the same issue.

Quoted 7 lines

Quoted 3 lines
Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.
The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?

Yes, I looked into that to check the feasibility to use the generic code. This specific path goes through __dquot_transfer() which uses ignore_hardlimit() helper to decide between bypassing quota enforcement or not.
The later though uses capable(CAP_SYS_RESOURCE) which would trigger selinux audit messages too at least for our case.

This though made me wonder if we shouldn't be using CAP_SYS_RESOURCE in lieu of CAP_FOWNER in this path which seems more appropriate to bypass quota enforcement. Either way though, I think we should at least have an initial simple to backport patch to older releases.

Quoted 5 lines

fs/xfs/xfs_iops.c xfs_setattr_nonsize( · line 796
Quoted 3 lines
fs/xfs/xfs_iops.c +2 -0
+	bool			force = ns_capable_noaudit(&init_user_ns,
+							   CAP_FOWNER);
I'd do away with the local variable here.

Sounds find to me. I wanted to avoid the function call with two arguments as an argument. But I honestly have no strong preference.

Cheers!

Jan Kara

reviewer

2026-06-25 10:44 UTC #3 lore raw

In reply to Christoph Hellwig

Quoted 15 lines · Christoph Hellwig

Adding Jan and Christian for quota and user_ns knowledge.
Quoted 12 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:
From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.
Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:

Quoted 4 lines

Quoted 2 lines
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials
Eww..

Yeah, that's a catch.

Quoted 8 lines

Quoted 4 lines
This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.
Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?

Quoted 6 lines

Quoted 2 lines
Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.
The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?

Actually no. Generic quota code has equivalent checks in ignore_hardlimit() function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why nobody complained about generic quota code is that we call ignore_hardlimit() only if we are above hardlimit whereas XFS calls this for every transaction...

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Carlos Maiolino

author maintainer

2026-06-25 11:01 UTC #4 lore raw

Quoted 38 lines · Jan Kara

Quoted 15 lines · Christoph Hellwig
Adding Jan and Christian for quota and user_ns knowledge.
Quoted 12 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:
From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.
Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:
Quoted 4 lines
Quoted 2 lines
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials
Eww..
Yeah, that's a catch.
Quoted 8 lines
Quoted 4 lines
This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.
Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?
Quoted 6 lines
Quoted 2 lines
Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.
The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?

Quoted 5 lines

Actually no. Generic quota code has equivalent checks in ignore_hardlimit() function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why nobody complained about generic quota code is that we call ignore_hardlimit() only if we are above hardlimit whereas XFS calls this for every transaction...

But the capable() call works fine, it's the replacement by has_capability_noaudit() that enables quota bypass I *think* generic quota code is safe from this point.

Quoted 6 lines

Honza
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR

Christoph Hellwig

2026-06-25 12:37 UTC #5 lore raw

In reply to Jan Kara

Quoted 38 lines · Jan Kara

Quoted 15 lines · Christoph Hellwig
Adding Jan and Christian for quota and user_ns knowledge.
Quoted 12 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:
From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.
Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:
Quoted 4 lines
Quoted 2 lines
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials
Eww..
Yeah, that's a catch.
Quoted 8 lines
Quoted 4 lines
This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.
Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?
Quoted 6 lines
Quoted 2 lines
Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.
The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?

Quoted 5 lines

Actually no. Generic quota code has equivalent checks in ignore_hardlimit() function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why nobody complained about generic quota code is that we call ignore_hardlimit() only if we are above hardlimit whereas XFS calls this for every transaction...

I guess we should aim for the same to avoid the spurious audit logs.

I.e. xfs_trans_alloc_ichange is currently always called either with force = true or force = this capable check. So as a first step we can move the check into xfs_trans_alloc_ichange for the !force case, and the propagate that through XFS_QMOPT_FORCE_RES into xfs_trans_dqresv, i.e. only set XFS_QMOPT_FORCE_RES for the real forced case and instead have the capable check down in xfs_trans_dqresv.

Carlos Maiolino

author maintainer

2026-06-25 15:00 UTC #6 lore raw

Quoted 45 lines · Christoph Hellwig

Quoted 38 lines · Jan Kara
Quoted 15 lines · Christoph Hellwig
Adding Jan and Christian for quota and user_ns knowledge.
Quoted 12 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:
From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.
Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:
Quoted 4 lines
Quoted 2 lines
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials
Eww..
Yeah, that's a catch.
Quoted 8 lines
Quoted 4 lines
This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.
Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?
Quoted 6 lines
Quoted 2 lines
Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.
The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?
Quoted 5 lines
Actually no. Generic quota code has equivalent checks in ignore_hardlimit() function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why nobody complained about generic quota code is that we call ignore_hardlimit() only if we are above hardlimit whereas XFS calls this for every transaction...

Quoted 9 lines

I guess we should aim for the same to avoid the spurious audit logs.
I.e. xfs_trans_alloc_ichange is currently always called either with force = true or force = this capable check. So as a first step we can move the check into xfs_trans_alloc_ichange for the !force case, and the propagate that through XFS_QMOPT_FORCE_RES into xfs_trans_dqresv, i.e. only set XFS_QMOPT_FORCE_RES for the real forced case and instead have the capable check down in xfs_trans_dqresv.

Sounds fair, I'll give it a try tomorrow.

Darrick J. Wong

2026-06-25 16:03 UTC #7 lore raw

Quoted 18 lines · Carlos Maiolino

Quoted 17 lines · Christoph Hellwig
Quoted 16 lines · Jan Kara
Quoted 15 lines · Christoph Hellwig
Adding Jan and Christian for quota and user_ns knowledge.
Quoted 12 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:
From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.
Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials
Eww..
Yeah, that's a catch.

I spent a while trying to figure out how I went wrong in selecting the function name:

* capable - Determine if the current task has a superior capability in effect
* @cap: The capability to be tested for
*
* Return true if the current task has the given superior capability currently
* available for use, false if not.

vs.

* has_capability_noaudit - Does a task have a capability (unaudited) in the
* initial user ns
* @t: The task in question
* @cap: The capability to be tested for
*
* Return true if the specified task has the given superior capability
* currently in effect to init_user_ns, false if not. Don't write an
* audit message for the check.

vs.

* ns_capable_noaudit - Determine if the current task has a superior capability
* (unaudited) in effect
* @ns: The usernamespace we want the capability in
* @cap: The capability to be tested for
*
* Return true if the current task has the given superior capability currently
* available for use, false if not.

All three of these sound the same to me. But what about the call path?

capable -> ns_capable -> ns_capable_common

has_capability_noaudit -> has_ns_capability_noaudit

ns_capable_noaudit -> ns_capable_common

Hum, maybe that's the difference -- ns_capable_common vs. has_ns_capability_noaudit?

ns_capable_common calls security_capable() with current_cred(), whereas has_ns_capability_noaudit calls it with __task_cred(t), which is @current in the xfs_trans_alloc_ichange case.

Aha! current_cred is current->cred, whereas __task_cred(current) is current->real_cred, and these aren't the same thing:

	/* Objective and real subjective task credentials (COW): */
	const struct cred __rcu		*real_cred;

	/* Effective (overridable) subjective task credentials (COW): */
	const struct cred __rcu		*cred;

So I guess ns_capable_common (and wrappers) are testing the process' effective credentials, whereas has_ns_capability_noaudit is testing the process' real credentials?

It would have been *really* nice if the documentation for those functions mentioned that distinction!

Quoted 31 lines

Quoted 21 lines
Quoted 15 lines
Quoted 8 lines
Quoted 4 lines
This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.
Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?
Quoted 6 lines
Quoted 2 lines
Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.
The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?
Quoted 5 lines
Actually no. Generic quota code has equivalent checks in ignore_hardlimit() function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why nobody complained about generic quota code is that we call ignore_hardlimit() only if we are above hardlimit whereas XFS calls this for every transaction...
Quoted 9 lines
I guess we should aim for the same to avoid the spurious audit logs.
I.e. xfs_trans_alloc_ichange is currently always called either with force = true or force = this capable check. So as a first step we can move the check into xfs_trans_alloc_ichange for the !force case, and the propagate that through XFS_QMOPT_FORCE_RES into xfs_trans_dqresv, i.e. only set XFS_QMOPT_FORCE_RES for the real forced case and instead have the capable check down in xfs_trans_dqresv.

Quoted 1 line

Sounds fair, I'll give it a try tomorrow.

So yeah, I agree we should change that to:

ns_capable_noaudit(&init_user_ns, CAP_FOWNER)

Though it's also weird that XFS gates it on CAP_FOWNER whereas the VFS checks CAP_SYS_RESOURCE. Though I would have added this:

static inline bool
current_may_ignore_quota_limits(void)

{
	/*
	 * If the current process' effective credentials include
	 * CAP_FOWNER, then they're allowed to ignore the hard limit.
	 */
	return ns_capable_noaudit(&init_user_ns, CAP_FOWNER);
}

and then changed the callsites in xfs_ioctl/xfs_iops.c to:

	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
			current_may_ignore_quota_limits(), &tp);

The has_capability_noaudit call in xfs_fsmap.c should change to ns_capable_noaudit.

--D

Carlos Maiolino

author maintainer

2026-06-26 07:34 UTC #8 lore raw

Quoted 56 lines · Darrick J. Wong

Quoted 18 lines · Carlos Maiolino
Quoted 17 lines · Christoph Hellwig
Quoted 16 lines · Jan Kara
Quoted 15 lines · Christoph Hellwig
Adding Jan and Christian for quota and user_ns knowledge.
Quoted 12 lines · On Wed, Jun 24, 2026 at 12:14:29PM +0200, [email protected] wrote:
From: Carlos Maiolino <[email protected]>
An user reported a bug where he managed to evade group's quota by changing a file's gid to a different group id the same user belonged to, even though quotas were enforced on both gids and the file's size was big enough to exceed the quota's hardlimit.
Commit eba0549bc7d1 replaced a capable() call by a has_capability_noaudit() to prevent unnecessary selinux audit messages. Turns out that both calls have slightly different semantics even though their documentation seems similar. Where in a nutshell:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
capable() - Tests the task's effective credentials has_ns_capability_noaudit() - Tests the task's real credentials
Eww..
Yeah, that's a catch.
I spent a while trying to figure out how I went wrong in selecting the function name:
* capable - Determine if the current task has a superior capability in effect
* @cap: The capability to be tested for
*
* Return true if the current task has the given superior capability currently
* available for use, false if not.
vs.
* has_capability_noaudit - Does a task have a capability (unaudited) in the
* initial user ns
* @t: The task in question
* @cap: The capability to be tested for
*
* Return true if the specified task has the given superior capability
* currently in effect to init_user_ns, false if not. Don't write an
* audit message for the check.
vs.
* ns_capable_noaudit - Determine if the current task has a superior capability
* (unaudited) in effect
* @ns: The usernamespace we want the capability in
* @cap: The capability to be tested for
*
* Return true if the current task has the given superior capability currently
* available for use, false if not.

Quoted 1 line

All three of these sound the same to me. But what about the call path?

Yup, I reached the same conclusion, they are all the same, so I decided to look at the implementation...

Quoted 23 lines

capable -> ns_capable -> ns_capable_common
has_capability_noaudit -> has_ns_capability_noaudit
ns_capable_noaudit -> ns_capable_common
Hum, maybe that's the difference -- ns_capable_common vs. has_ns_capability_noaudit?
ns_capable_common calls security_capable() with current_cred(), whereas has_ns_capability_noaudit calls it with __task_cred(t), which is @current in the xfs_trans_alloc_ichange case.
Aha! current_cred is current->cred, whereas __task_cred(current) is current->real_cred, and these aren't the same thing:
	/* Objective and real subjective task credentials (COW): */
	const struct cred __rcu		*real_cred;

	/* Effective (overridable) subjective task credentials (COW): */
	const struct cred __rcu		*cred;

Quoted 6 lines

So I guess ns_capable_common (and wrappers) are testing the process' effective credentials, whereas has_ns_capability_noaudit is testing the process' real credentials?
It would have been *really* nice if the documentation for those functions mentioned that distinction!

Yup, this is a much more detailed versions from my same findings :)

Quoted 32 lines

Quoted 32 lines
Quoted 31 lines
Quoted 16 lines
Quoted 9 lines
Quoted 5 lines
This most of the time has no practical difference but in some cases like changing attrs (specifically group id in this case) through a NFS client this will allow the quota code to use XFS_QMOPT_FORCE_RES, effectively bypassing quota accounting checks.
Yeah, this does look wrong. Do the other conversion in the above commit have tthe same issue?
Quoted 6 lines
Quoted 2 lines
Using instead ns_capable_noaudit() should fix this issue and prevent selinux audit messages.
The generic quota code manages to do without either has_capability_noaudit or ns_capable_noaudit. I think this might be hidden behind inode_owner_or_capable calls. Any idea why we're different?
Quoted 5 lines
Actually no. Generic quota code has equivalent checks in ignore_hardlimit() function which does capable(CAP_SYS_RESOURCE) check. I guess the reason why nobody complained about generic quota code is that we call ignore_hardlimit() only if we are above hardlimit whereas XFS calls this for every transaction...
I guess we should aim for the same to avoid the spurious audit logs.
I.e. xfs_trans_alloc_ichange is currently always called either with force = true or force = this capable check. So as a first step we can move the check into xfs_trans_alloc_ichange for the !force case, and the propagate that through XFS_QMOPT_FORCE_RES into xfs_trans_dqresv, i.e. only set XFS_QMOPT_FORCE_RES for the real forced case and instead have the capable check down in xfs_trans_dqresv.

Quoted 8 lines

Quoted 1 line
Sounds fair, I'll give it a try tomorrow.
So yeah, I agree we should change that to:
ns_capable_noaudit(&init_user_ns, CAP_FOWNER)
Though it's also weird that XFS gates it on CAP_FOWNER whereas the VFS checks CAP_SYS_RESOURCE. Though I would have added this:

Same to me, I think there is no real reason for us to be using CAP_FOWNER? I thought about changing it to CAP_SYS_RESOURCE but honestly I'd avoid doing that for LTS and just stick with CAP_FOWNER initially.

Quoted 11 lines

static inline bool
current_may_ignore_quota_limits(void)

{
	/*
	 * If the current process' effective credentials include
	 * CAP_FOWNER, then they're allowed to ignore the hard limit.
	 */
	return ns_capable_noaudit(&init_user_ns, CAP_FOWNER);
}

Quoted 7 lines

and then changed the callsites in xfs_ioctl/xfs_iops.c to:
	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
			current_may_ignore_quota_limits(), &tp);
The has_capability_noaudit call in xfs_fsmap.c should change to ns_capable_noaudit.

I'm working on a version now adding some bits hch suggested. At the end I think we should add a new helper in kernel/capabililty, but I want to avoid doing that now so LTS can cleanly backport a fix. Then we can update it.
I'm working on both things in parallel and see how it goes. I'll Cc you folks.

Quoted 3 lines

--D