[PATCH] xfs: not allow rename if src is quota enabled and project IDs are different

[PATCH] xfs: not allow rename if src is quota enabled and project IDs are different

[renlei1]

From: Ren Lei <renlei1@chinatelecom.cn>

xfs not allow rename if target is using project inheritance and
project IDs are different to avoid tree quota mechanism not work.

But if only src with directory quota enabled, rename to other directory
without quota enabled can succeed and skip quota mechanism. which might
result to unexpected quota behavior.

This patch fix this by disable rename if src is using project inheritance
and the project IDs are not the same.

following steps can easy reproduce this issue:
1. first init a directory quota /mnt/test
	mount -o prjquota /dev/sdb  /mnt
	mkdir /mnt/test
	echo 1:/mnt/test >> /etc/projects
	echo test:1 >> /etc/projid
	xfs_quota -x -c 'project -s test' /mnt
	xfs_quota -x -c 'limit -p bhard=10m test' /mnt

2. fill /mnt/test with tesfile util directory full:
	[root@rhost1 test]# dd if=/dev/zero of=/mnt/test/testfile
	dd: writing to '/mnt/test/testfile': No space left on device

3. mv testfile out to /mnt,  test is empty but cannot create files:
	[root@rhost1 test]# mv testfile ../
	[root@rhost1 test]# ls -a
	.  ..
	[root@rhost1 test]# touch aaa
	touch: cannot touch 'aaa': Disk quota exceeded

Signed-off-by: Ren Lei <renlei1@chinatelecom.cn>
---
 fs/xfs/xfs_inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 6771f357ad2c..f8c115b014f9 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3177,7 +3177,8 @@ xfs_rename(
 	 * into our tree when the project IDs are the same; else the
 	 * tree quota mechanism would be circumvented.
 	 */
-	if (unlikely((target_dp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
+	if (unlikely((target_dp->i_diflags & XFS_DIFLAG_PROJINHERIT ||
+		     src_dp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
 		     target_dp->i_projid != src_ip->i_projid)) {
 		error = -EXDEV;
 		goto out_trans_cancel;
-- 
2.27.0

Re: [PATCH] xfs: not allow rename if src is quota enabled and project IDs are different

[Dave Chinner]

On Tue, Dec 14, 2021 at 11:15:17AM +0800, renlei1@chinatelecom.cn wrote:
> From: Ren Lei <renlei1@chinatelecom.cn>
> 
> xfs not allow rename if target is using project inheritance and
> project IDs are different to avoid tree quota mechanism not work.

Lesson #1: project quotas are *not* directory quotas.

> But if only src with directory quota enabled, rename to other directory
> without quota enabled can succeed and skip quota mechanism. which might
> result to unexpected quota behavior.

Yes, this is explicitly done this way, because project quotas are
not directory quotas.

Lesson #2: directory tree semantics use PROJINHERIT to define an
entry barrier to the directory, not an exit barrier.

That is, we restrict moving inodes with incompatible project IDs
into a directory that might be used as a directory tree, but we
don't prevent moving inodes out of PROJINHERIT directories into
locations that have no PROJINHERIT set.

The reason for this is simple: destinations that don't have
PROJINHERIT set are unrestricted and can contain inodes with any
valid projid. This is the traditional use of project quotas,
because...

Lesson #3a: PROJINHERIT only defines the default project ID for newly
created inodes in a directory.

Lesson #3b: Unprivileged users in the init namespace are allowed to
change PROJINHERIT and projid on any inode they have write
permissions on.

That is, PROJINHERIT does not prevent users from changing the
project ID of files within the directory, or even that of the
directory so that it no longer matches the projid of the existing
directory contents. Hence directory tree quotas will only remain
valid with the co-operation of unprivileged users, as project ID and
PROJINHERIT are user modifiable inode attributes.

Lesson #4: Using project quotas to provide directory tree quotas
does not result in an access-based space usage enforcement mechanism
without some other mechanism for preventing users from accessing and
changing project quota information. (e.g. containers and user
namespaces)

> This patch fix this by disable rename if src is using project inheritance
> and the project IDs are not the same.
> 
> following steps can easy reproduce this issue:
> 1. first init a directory quota /mnt/test
> 	mount -o prjquota /dev/sdb  /mnt
> 	mkdir /mnt/test
> 	echo 1:/mnt/test >> /etc/projects
> 	echo test:1 >> /etc/projid
> 	xfs_quota -x -c 'project -s test' /mnt
> 	xfs_quota -x -c 'limit -p bhard=10m test' /mnt
>
> 2. fill /mnt/test with tesfile util directory full:
> 	[root@rhost1 test]# dd if=/dev/zero of=/mnt/test/testfile
> 	dd: writing to '/mnt/test/testfile': No space left on device
> 3. mv testfile out to /mnt,  test is empty but cannot create files:
> 	[root@rhost1 test]# mv testfile ../
> 	[root@rhost1 test]# ls -a
> 	.  ..
> 	[root@rhost1 test]# touch aaa
> 	touch: cannot touch 'aaa': Disk quota exceeded

Yup, exfiltration is not prohibited, as per above. What you need to
do here is prevent infiltration to the "../" directory by use of
a default directory quota for all the "non-controlled" part of the
directory heirarchy. That is:

> 	mount -o prjquota /dev/sdb  /mnt
> 	mkdir /mnt/test
> 	echo 1:/mnt > /etc/projects
> 	echo 2:/mnt/test >> /etc/projects
> 	echo default:1 >> /etc/projid
> 	echo test:2 >> /etc/projid
> 	xfs_quota -x -c 'project -s default' /mnt
> 	xfs_quota -x -c 'project -s test' /mnt
> 	xfs_quota -x -c 'limit -p bhard=10m test' /mnt

So now you have the default "unlimited" directory quota on the
entire filesytem, with the sub-tree "test" set up with a hard limit.
Now step #3 in your test will behave as you expect, because ".." has
a PROJINHERIT w/ projid = 1 set and that will trigger the
"destination directory has directory quota and different projid"
-EXDEV error case in rename.

i.e. you fix this problem by setting up the directory tree quota
configuration properly, not by changing the kernel code behaviour...

Note: you can set the default directory tree project ID at mkfs time
via:

# mkfs.xfs -f -d projinherit=42 /dev/sdb

So you don't actually need to set up a default project in
/etc/projects to make this work correctly.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

回复: [PATCH] xfs: not allow rename if src is quota enabled and project IDs are different

[renlei1]

That's make sense, thanks very much for your detailed response!

Regards,
RenLei

-----邮件原件-----
发件人: david@fromorbit.com <david@fromorbit.com> 
发送时间: 2021年12月14日 12:49
收件人: renlei1@chinatelecom.cn
抄送: djwong@kernel.org; linux-xfs@vger.kernel.org;
linux-kernel@vger.kernel.org
主题: Re: [PATCH] xfs: not allow rename if src is quota enabled and project
IDs are different

On Tue, Dec 14, 2021 at 11:15:17AM +0800, renlei1@chinatelecom.cn wrote:
> From: Ren Lei <renlei1@chinatelecom.cn>
> 
> xfs not allow rename if target is using project inheritance and 
> project IDs are different to avoid tree quota mechanism not work.

Lesson #1: project quotas are *not* directory quotas.

> But if only src with directory quota enabled, rename to other 
> directory without quota enabled can succeed and skip quota mechanism. 
> which might result to unexpected quota behavior.

Yes, this is explicitly done this way, because project quotas are not
directory quotas.

Lesson #2: directory tree semantics use PROJINHERIT to define an entry
barrier to the directory, not an exit barrier.

That is, we restrict moving inodes with incompatible project IDs into a
directory that might be used as a directory tree, but we don't prevent
moving inodes out of PROJINHERIT directories into locations that have no
PROJINHERIT set.

The reason for this is simple: destinations that don't have PROJINHERIT set
are unrestricted and can contain inodes with any valid projid. This is the
traditional use of project quotas, because...

Lesson #3a: PROJINHERIT only defines the default project ID for newly
created inodes in a directory.

Lesson #3b: Unprivileged users in the init namespace are allowed to change
PROJINHERIT and projid on any inode they have write permissions on.

That is, PROJINHERIT does not prevent users from changing the project ID of
files within the directory, or even that of the directory so that it no
longer matches the projid of the existing directory contents. Hence
directory tree quotas will only remain valid with the co-operation of
unprivileged users, as project ID and PROJINHERIT are user modifiable inode
attributes.

Lesson #4: Using project quotas to provide directory tree quotas does not
result in an access-based space usage enforcement mechanism without some
other mechanism for preventing users from accessing and changing project
quota information. (e.g. containers and user
namespaces)

> This patch fix this by disable rename if src is using project 
> inheritance and the project IDs are not the same.
> 
> following steps can easy reproduce this issue:
> 1. first init a directory quota /mnt/test
> 	mount -o prjquota /dev/sdb  /mnt
> 	mkdir /mnt/test
> 	echo 1:/mnt/test >> /etc/projects
> 	echo test:1 >> /etc/projid
> 	xfs_quota -x -c 'project -s test' /mnt
> 	xfs_quota -x -c 'limit -p bhard=10m test' /mnt
>
> 2. fill /mnt/test with tesfile util directory full:
> 	[root@rhost1 test]# dd if=/dev/zero of=/mnt/test/testfile
> 	dd: writing to '/mnt/test/testfile': No space left on device 3. mv 
> testfile out to /mnt,  test is empty but cannot create files:
> 	[root@rhost1 test]# mv testfile ../
> 	[root@rhost1 test]# ls -a
> 	.  ..
> 	[root@rhost1 test]# touch aaa
> 	touch: cannot touch 'aaa': Disk quota exceeded

Yup, exfiltration is not prohibited, as per above. What you need to do here
is prevent infiltration to the "../" directory by use of a default directory
quota for all the "non-controlled" part of the directory heirarchy. That is:

> 	mount -o prjquota /dev/sdb  /mnt
> 	mkdir /mnt/test
> 	echo 1:/mnt > /etc/projects
> 	echo 2:/mnt/test >> /etc/projects
> 	echo default:1 >> /etc/projid
> 	echo test:2 >> /etc/projid
> 	xfs_quota -x -c 'project -s default' /mnt
> 	xfs_quota -x -c 'project -s test' /mnt
> 	xfs_quota -x -c 'limit -p bhard=10m test' /mnt

So now you have the default "unlimited" directory quota on the entire
filesytem, with the sub-tree "test" set up with a hard limit.
Now step #3 in your test will behave as you expect, because ".." has a
PROJINHERIT w/ projid = 1 set and that will trigger the "destination
directory has directory quota and different projid"
-EXDEV error case in rename.

i.e. you fix this problem by setting up the directory tree quota
configuration properly, not by changing the kernel code behaviour...

Note: you can set the default directory tree project ID at mkfs time
via:

# mkfs.xfs -f -d projinherit=42 /dev/sdb

So you don't actually need to set up a default project in /etc/projects to
make this work correctly.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com