Re: [PATCH] xfs: reserve quota for directory expansion when hardlinking files

[Dave Chinner <david@fromorbit.com>]

On Mon, Feb 28, 2022 at 06:51:18PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The XFS implementation of the linkat call does not reserve quota for the
> potential directory expansion.  This means that we don't reject the
> expansion with EDQUOT when we're at or near a hard limit, which means
> that one can use linkat() to exceed quota.  Fix this by adding a quota
> reservation.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_inode.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 04bf467b1090..6e556c9069e8 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1249,6 +1249,10 @@ xfs_link(
>  	xfs_trans_ijoin(tp, sip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, tdp, XFS_ILOCK_EXCL);
>  
> +	error = xfs_trans_reserve_quota_nblks(tp, tdp, resblks, 0, false);
> +	if (error)
> +		goto error_return;
> +
>  	error = xfs_iext_count_may_overflow(tdp, XFS_DATA_FORK,
>  			XFS_IEXT_DIR_MANIP_CNT(mp));
>  	if (error)

Yup, ok, but doesn't xfs_remove have exactly the same problem? i.e.
removing a directory entry can punch a hole in the bmbt and require
new allocations for a BMBT split, thereby increasing the number of
blocks allocated to the directory? e.g. remove a single data block,
need to then allocate half a dozen BMBT blocks for the shape change.

If so, then both xfs_link() and xfs_remove() have exactly the same
dquot, inode locking and transaction setup code and requirements,
and probably should be factored out into xfs_trans_alloc_dir() (i.e.
equivalent of xfs_trans_alloc_icreate() used by all the inode create
functions).  That way we only have one copy of this preamble and
only need to fix the bug in one place?

Alternatively, fix the bug in both places first and add a followup
patch that factors out this code as per above.

Hmmm - looking further a callers of xfs_lock_two_inodes(), it would
appear that xfs_swap_extents() needs the same quota reservation
and also largely has the same transaction setup and inode locking
preamble as link and remove...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com