Re: [PATCH 2/2] xfs: lift quota capability check to xfs_trans_dqresv

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

On Fri, Jun 26, 2026 at 12:29:25PM +0200, cem@kernel.org wrote:
> From: Carlos Maiolino <cem@kernel.org>
> 
> Instead of calling into ns_capable_noaudit() as an argument to
> xfs_trans_alloc_ichange(), just pass false where XFS_QMOPT_FORCE_RES is
> not explicitly required and let xfs_trans_dqresv() decide if quotas
> should be bypassed or not.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_ioctl.c       | 3 +--
>  fs/xfs/xfs_iops.c        | 4 +---
>  fs/xfs/xfs_trans_dquot.c | 6 ++++--
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 852ff2ab4531..480feddc0e51 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -646,8 +646,7 @@ xfs_ioctl_setattr_get_trans(
>  	if (xfs_is_shutdown(mp))
>  		goto out_error;
>  
> -	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp,
> -			ns_capable_noaudit(&init_user_ns, CAP_FOWNER), &tp);
> +	error = xfs_trans_alloc_ichange(ip, NULL, NULL, pdqp, false, &tp);
>  	if (error)
>  		goto out_error;
>  
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 9db9ef1d8c3a..d2ae682f749f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -834,9 +834,7 @@ xfs_setattr_nonsize(
>  			return error;
>  	}
>  
> -	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL,
> -				ns_capable_noaudit(&init_user_ns, CAP_FOWNER),
> -				&tp);
> +	error = xfs_trans_alloc_ichange(ip, udqp, gdqp, NULL, false, &tp);

Hrm.  On second viewing, I don't really like burying the capability
check down in the transaction code because this overrides the higher
level logic that sets (or doesn't set) XFS_QMOPT_FORCE_RES.

>  	if (error)
>  		goto out_dqrele;
>  
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index eaf9de6e07fd..50e5b323f7f1 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -832,8 +832,10 @@ xfs_trans_dqresv(
>  		qlim = &defq->rtb;
>  	}
>  
> -	if ((flags & XFS_QMOPT_FORCE_RES) == 0 && dqp->q_id &&
> -	    xfs_dquot_is_enforced(dqp)) {
> +	if ((flags & XFS_QMOPT_FORCE_RES) == 0 &&
> +	    dqp->q_id &&
> +	    xfs_dquot_is_enforced(dqp) &&
> +	    !ns_capable_noaudit(&init_user_ns, CAP_SYS_RESOURCE)) {

The other thing that bugs me is changing CAP_FOWNER to CAP_SYS_RESOURCE
-- that's not mentioned in the commit message even though it's a
potentially breaking change for any program that already internalized
doing that on only XFS.  At a minimum we'd also have to change the
xfs_scrub service files to have CAP_SYS_RESOURCE (in addition to FOWNER)
avoid EDQUOT during online repairs.

Looking back in the history of XFS and VFS quotas, I guess XFS has been
doing CAP_FOWNER since 2.4.25, and VFS has been doing CAP_SYS_RESOURCE
since 2.3.30.  So XFS was the one that deviated (possibly because FOWNER
was the established privilege on Irix?) but I think that means XFS has
to accept either.

Going back to my idea from yesterday, I now amend it to:

static inline bool
current_may_ignore_quota_limits(void)
{
	/*
	 * Linux VFS quota code allows ignoring hard limits on
	 * chown/chgrp if the effective credentials include
	 * CAP_SYS_RESOURCE.
	 */
	if (ns_capable_noaudit(&init_user_ns, CAP_SYS_RESOURCE))
		return true;

	/*
	 * Historically, XFS also allows that 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);
}

The callsites are still:

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

--D

>  		int		quota_nl;
>  		bool		fatal;
>  
> -- 
> 2.54.0
> 
>