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

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

[Darrick J. Wong]

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)

[PATCH] generic: test that linking into a directory fails with EDQUOT

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Add a regression test to make sure that unprivileged userspace linking
into a directory fails with EDQUOT when the directory quota limits have
been exceeded.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/732     |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/732.out |    3 ++
 2 files changed, 70 insertions(+)
 create mode 100755 tests/generic/732
 create mode 100644 tests/generic/732.out

diff --git a/tests/generic/732 b/tests/generic/732
new file mode 100755
index 00000000..c4a3dcb6
--- /dev/null
+++ b/tests/generic/732
@@ -0,0 +1,67 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 732
+#
+# Ensure that unprivileged userspace hits EDQUOT while linking files into a
+# directory when the directory's quota limits have been exceeded.
+#
+# Regression test for commit:
+#
+# XXXXXXXXXXXX ("xfs: reserve quota for directory expansion when hardlinking files")
+#
+. ./common/preamble
+_begin_fstest auto quick quota
+
+# Import common functions.
+. ./common/filter
+. ./common/quota
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_quota
+_require_user
+_require_scratch
+
+_scratch_mkfs > "$seqres.full" 2>&1
+_qmount_option usrquota
+_qmount
+
+blocksize=$(_get_block_size $SCRATCH_MNT)
+scratchdir=$SCRATCH_MNT/dir
+scratchfile=$SCRATCH_MNT/file
+mkdir $scratchdir
+touch $scratchfile
+
+# Create a 2-block directory for our 1-block quota limit
+total_size=$((blocksize * 2))
+dirents=$((total_size / 255))
+
+for ((i = 0; i < dirents; i++)); do
+	name=$(printf "x%0254d" $i)
+	ln $scratchfile $scratchdir/$name
+done
+
+# Set a low quota hardlimit for an unprivileged uid and chown the files to it
+echo "set up quota" >> $seqres.full
+setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
+chown $qa_user $scratchdir $scratchfile
+repquota -upn $SCRATCH_MNT >> $seqres.full
+
+# Fail at appending the directory as qa_user to ensure quota enforcement works
+echo "fail quota" >> $seqres.full
+for ((i = 0; i < dirents; i++)); do
+	name=$(printf "y%0254d" $i)
+	su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
+		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
+	test "${PIPESTATUS[0]}" -ne 0 && break
+done
+repquota -upn $SCRATCH_MNT >> $seqres.full
+
+# success, all done
+echo Silence is golden
+status=0
+exit
diff --git a/tests/generic/732.out b/tests/generic/732.out
new file mode 100644
index 00000000..15d2cb61
--- /dev/null
+++ b/tests/generic/732.out
@@ -0,0 +1,3 @@
+QA output created by 732
+ln: failed to create hard link 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
+Silence is golden

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

[Darrick J. Wong]

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.

Could someone review this, please?  I'd like to get a 5.18-fixes branch
rolling along with the other patches that have already been sent.

--D

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

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

[Dave Chinner]

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

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

[Darrick J. Wong]

On Wed, Mar 09, 2022 at 09:18:55AM +1100, Dave Chinner wrote:
> 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.

Yes, it does, however, the reason I don't have a fix for that ready is
that...

> removing a directory entry can punch a hole in the bmbt and require
> new allocations for a BMBT split, thereby increasing the number of

...rejecting a directory unlink with EDQUOT creates the situation where
a user who's gone over the soft limit cannot rm a file to get themselves
back under quota because the removal asked for enough bmbt-expansion
quota reservation to push the quota over the hard limit...

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

...and while the next thing that occurred to me was to retry the quota
reservation with FORCE_RES, having such a path means that one can still
overrun the hard limit (albeit slowly) by creating a fragmented
directory and selectively removing entries to cause bmbt splits.

I /think/ I'm ok with the "retry with FORCE_QUOTA" solution for
xfs_remove, but I'm hanging onto it for now for further consideration
and QA testing.

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

They're not the same problem -- adding hardlinks is not a known strategy
for reducing quota usage below the limits, whereas unlinking files is.

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

I sent a patch for the link situation because I thought it looked like
an obvious fix, and left the unlink() problem until a full solution is
presented or proved impossible.

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

Yes, I know about that problem.  I've *solved* that problem with the
atomic extent swap rewrite that's been hanging out in djwong-dev since
late 2019 as part of the online fsck series.  Perhaps I will have time
to send that in late 2022.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Dave Chinner]

On Tue, Mar 08, 2022 at 03:17:42PM -0800, Darrick J. Wong wrote:
> On Wed, Mar 09, 2022 at 09:18:55AM +1100, Dave Chinner wrote:
> > 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;
> > > +

Hmmm - I just noticed that trans_alloc_icreate and trans_alloc_inode
also run a blockgc pass on EDQUOT or ENOSPC when they fail to
reserve quota to try to free up some space before retrying. Do we
need that here, too?

> > >  	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.
> 
> Yes, it does, however, the reason I don't have a fix for that ready is
> that...
> 
> > removing a directory entry can punch a hole in the bmbt and require
> > new allocations for a BMBT split, thereby increasing the number of
> 
> ...rejecting a directory unlink with EDQUOT creates the situation where
> a user who's gone over the soft limit cannot rm a file to get themselves
> back under quota because the removal asked for enough bmbt-expansion
> quota reservation to push the quota over the hard limit...

Both link and remove already have "zero reservation" paths for
ENOSPC - if they are to be made quota aware they'll end up with
resblks = 0 and so xfs_trans_reserve_quota_nblks() is a no-op at
ENOSPC. So ....

> 
> > 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.
> 
> ...and while the next thing that occurred to me was to retry the quota
> reservation with FORCE_RES, having such a path means that one can still
> overrun the hard limit (albeit slowly) by creating a fragmented
> directory and selectively removing entries to cause bmbt splits.

> I /think/ I'm ok with the "retry with FORCE_QUOTA" solution for
> xfs_remove, but I'm hanging onto it for now for further consideration
> and QA testing.

... yes, I think this would be just fine. I don't think we really
care in any way about people trying to grow their quota beyond the
hard limit by a few blocks by intentionally fragmenting really large
directories. If their quota allows them directories and inode counts
large enough for this to be an avenue to exceeding hard quota limits
by a block or two, nobody is going to notice about a block or two or
extra space usage.

> > 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?
> 
> They're not the same problem -- adding hardlinks is not a known strategy
> for reducing quota usage below the limits, whereas unlinking files is.
> 
> > Alternatively, fix the bug in both places first and add a followup
> > patch that factors out this code as per above.
> 
> I sent a patch for the link situation because I thought it looked like
> an obvious fix, and left the unlink() problem until a full solution is
> presented or proved impossible.

Ok. None of this was mentioned in the patch, so I had no idea about
any of the things you are doing behind the scenes. I simply saw the
same problem in other places....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Wed, Mar 09, 2022 at 12:12:09PM +1100, Dave Chinner wrote:
> On Tue, Mar 08, 2022 at 03:17:42PM -0800, Darrick J. Wong wrote:
> > On Wed, Mar 09, 2022 at 09:18:55AM +1100, Dave Chinner wrote:
> > > 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;
> > > > +
> 
> Hmmm - I just noticed that trans_alloc_icreate and trans_alloc_inode
> also run a blockgc pass on EDQUOT or ENOSPC when they fail to
> reserve quota to try to free up some space before retrying. Do we
> need that here, too?

(Re)trying to clear more space sounds like a good idea.

> > > >  	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.
> > 
> > Yes, it does, however, the reason I don't have a fix for that ready is
> > that...
> > 
> > > removing a directory entry can punch a hole in the bmbt and require
> > > new allocations for a BMBT split, thereby increasing the number of
> > 
> > ...rejecting a directory unlink with EDQUOT creates the situation where
> > a user who's gone over the soft limit cannot rm a file to get themselves
> > back under quota because the removal asked for enough bmbt-expansion
> > quota reservation to push the quota over the hard limit...
> 
> Both link and remove already have "zero reservation" paths for
> ENOSPC - if they are to be made quota aware they'll end up with
> resblks = 0 and so xfs_trans_reserve_quota_nblks() is a no-op at
> ENOSPC. So ....
> 
> > 
> > > 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.
> > 
> > ...and while the next thing that occurred to me was to retry the quota
> > reservation with FORCE_RES, having such a path means that one can still
> > overrun the hard limit (albeit slowly) by creating a fragmented
> > directory and selectively removing entries to cause bmbt splits.
> 
> > I /think/ I'm ok with the "retry with FORCE_QUOTA" solution for
> > xfs_remove, but I'm hanging onto it for now for further consideration
> > and QA testing.
> 
> ... yes, I think this would be just fine. I don't think we really
> care in any way about people trying to grow their quota beyond the
> hard limit by a few blocks by intentionally fragmenting really large
> directories. If their quota allows them directories and inode counts
> large enough for this to be an avenue to exceeding hard quota limits
> by a block or two, nobody is going to notice about a block or two or
> extra space usage.

At least for the link case, you can trivially continue to expand the
directory by hardlinking the same file over and over.  Part of the
weirdness here might be related to the fact that a transaction with no
quota reservation is allowed to commit the quota usage changes, even if
that would bump them past the limit.

Hm.  Perhaps the trick here should be that we reduce resblks to zero for
ENOSPC or EDQUOT, which means that you can continue link()ing files
into a directory so long as it won't cause the dir to expand.
xfs_remove (aka unlink()) handles reservationless removals by deferring
the directory shrink operation if there isn't space, so I think it can
be ported to use the new "alloc and reserve" function too.

> > > 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?
> > 
> > They're not the same problem -- adding hardlinks is not a known strategy
> > for reducing quota usage below the limits, whereas unlinking files is.
> > 
> > > Alternatively, fix the bug in both places first and add a followup
> > > patch that factors out this code as per above.
> > 
> > I sent a patch for the link situation because I thought it looked like
> > an obvious fix, and left the unlink() problem until a full solution is
> > presented or proved impossible.
> 
> Ok. None of this was mentioned in the patch, so I had no idea about
> any of the things you are doing behind the scenes. I simply saw the
> same problem in other places....

Yeah, there are more fiddly fixes for setattr coming down the line
too...

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com