xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

PATCH In review Darrick J. Wong 7 participants 12 messages Aug 23, 2019 – Aug 25, 2019 XFS filesystem

lore

Share & tools

lore

Darrick J. Wong

author

2019-08-23 03:56 UTC #0 lore raw

PATCH 1/1 xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT

From: Darrick J. Wong <[email protected]>

Benjamin Moody reported to Debian that XFS partially wedges when a chgrp fails on account of being out of disk quota. I ran his reproducer script:

# adduser dummy
# adduser dummy plugdev

# dd if=/dev/zero bs=1M count=100 of=test.img
# mkfs.xfs test.img
# mount -t xfs -o gquota test.img /mnt
# mkdir -p /mnt/dummy
# chown -c dummy /mnt/dummy
# xfs_quota -xc 'limit -g bsoft=100k bhard=100k plugdev' /mnt

(and then as user dummy)

$ dd if=/dev/urandom bs=1M count=50 of=/mnt/dummy/foo
$ chgrp plugdev /mnt/dummy/foo

and saw:

================================================
WARNING: lock held when returning to user space!
5.3.0-rc5 #rc5 Tainted: G W
------------------------------------------------
chgrp/47006 is leaving the kernel with locks still held!
1 lock held by chgrp/47006:
#0: 000000006664ea2d (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0xd2/0x290 [xfs]

...which is clearly caused by xfs_setattr_nonsize failing to unlock the ILOCK after the xfs_qm_vop_chown_reserve call fails. Add the missing unlock.

Reported-by [email protected]

Fixes 253f4911f297 ("xfs: better xfs_trans_alloc interface")

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/xfs_iops.c |    1 +
 1 file changed, 1 insertion(+)

fs/xfs/xfs_iops.c +1 -0

index dd4076ae228a..ea614b4ae052 100644
@@ -804,6 +804,7 @@ xfs_setattr_nonsize(
 
 out_cancel:
 	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out_dqrele:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);

Darrick J. Wong

author

2019-08-23 03:58 UTC #1 lore raw

PATCH 1/1 generic: test for failure to unlock inode after chgrp fails with EDQUOT

From: Darrick J. Wong <[email protected]>

This is a regression test that checks for xfs drivers that fail to unlock the inode after changing the group id fails with EDQUOT. It pairs with "xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT".

Signed-off-by Darrick J. Wong <[email protected]>

---
 tests/generic/719     |   56 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/719.out |    2 ++
 tests/generic/group   |    1 +
 3 files changed, 59 insertions(+)
 create mode 100755 tests/generic/719
 create mode 100644 tests/generic/719.out

tests/generic/719 new file +56 -0

new file mode 100755
index 00000000..2771a1f3
@@ -0,0 +1,56 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0-or-newer
+# Copyright (c) 2019, Oracle and/or its affiliates.  All Rights Reserved.
+#
+# FS QA Test No. 719
+#
+# Regression test for chgrp returning to userspace with ILOCK held after a
+# hard quota error.  This causes the filesystem to hang, so it is (for now)
+# a dangerous test.
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1    # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/quota
+. ./common/filter
+
+# real QA test starts here
+_supported_os Linux
+_supported_fs generic
+_require_scratch
+_require_quota
+_require_user
+
+rm -f $seqres.full
+
+_qmount_option "grpquota"
+_scratch_mkfs > $seqres.full
+_qmount
+
+dir="$SCRATCH_MNT/dummy"
+mkdir -p $dir
+chown $qa_user $dir
+$XFS_QUOTA_PROG -x -f -c "limit -g bsoft=100k bhard=100k $qa_user" $SCRATCH_MNT
+
+$XFS_IO_PROG -f -c 'pwrite -S 0x58 0 1m' $dir/foo >> $seqres.full
+chown $qa_user "${dir}/foo"
+su $qa_user -c "chgrp $qa_user ${dir}/foo" 2>&1 | _filter_scratch
+ls -la ${dir} >> $seqres.full
+$XFS_QUOTA_PROG -x -f -c 'report -hag' $SCRATCH_MNT >> $seqres.full
+
+# success, all done
+status=0
+exit

tests/generic/719.out new file +2 -0

new file mode 100644
index 00000000..8f9d51b5
@@ -0,0 +1,2 @@
+QA output created by 719
+chgrp: changing group of 'SCRATCH_MNT/dummy/foo': Disk quota exceeded

tests/generic/group +1 -0

index e998d1d5..bb93bccc 100644
@@ -572,3 +572,4 @@
 716 dangerous_norepair
 717 auto quick rw swap
 718 auto quick rw swap
+719 dangerous

Christoph Hellwig

Reviewed-by on v1 1/1

2019-08-24 23:05 UTC #2 lore raw

Re: [PATCH] generic: test for failure to unlock inode after chgrp fails with EDQUOT

tests/generic/group line 575

Quoted 9 lines

tests/generic/group +1 -0

index e998d1d5..bb93bccc 100644
@@ -572,3 +572,4 @@
 716 dangerous_norepair
 717 auto quick rw swap
 718 auto quick rw swap
+719 dangerous

+ quota + metadata?

Otherwise this looks fine to me:

Reviewed-by Christoph Hellwig <[email protected]>

and after a while we should add it to auto. I'm not even sure if we shouldn't do that ASAP. It's not that dangerous and we should test for this everywhere..

Dave Chinner

Reviewed-by on v1 1/1

2019-08-23 04:56 UTC #3 lore raw

In reply to Darrick J. Wong

Quoted 47 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
Benjamin Moody reported to Debian that XFS partially wedges when a chgrp fails on account of being out of disk quota. I ran his reproducer script:
# adduser dummy
# adduser dummy plugdev
# dd if=/dev/zero bs=1M count=100 of=test.img
# mkfs.xfs test.img
# mount -t xfs -o gquota test.img /mnt
# mkdir -p /mnt/dummy
# chown -c dummy /mnt/dummy
# xfs_quota -xc 'limit -g bsoft=100k bhard=100k plugdev' /mnt
(and then as user dummy)
$ dd if=/dev/urandom bs=1M count=50 of=/mnt/dummy/foo
$ chgrp plugdev /mnt/dummy/foo
and saw:
================================================
WARNING: lock held when returning to user space!
5.3.0-rc5 #rc5 Tainted: G W
------------------------------------------------
chgrp/47006 is leaving the kernel with locks still held!
1 lock held by chgrp/47006:
#0: 000000006664ea2d (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0xd2/0x290 [xfs]
...which is clearly caused by xfs_setattr_nonsize failing to unlock the ILOCK after the xfs_qm_vop_chown_reserve call fails. Add the missing unlock.
Reported-by [email protected]
Fixes 253f4911f297 ("xfs: better xfs_trans_alloc interface")
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/xfs_iops.c |    1 +
 1 file changed, 1 insertion(+)
fs/xfs/xfs_iops.c +0 -0
index dd4076ae228a..ea614b4ae052 100644

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

Quoted 7 lines

fs/xfs/xfs_iops.c +1 -0

@@ -804,6 +804,7 @@ xfs_setattr_nonsize(
 
 out_cancel:
 	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out_dqrele:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);

/me goes back an looks at 253f4911f297 ("xfs: better xfs_trans_alloc interface")

Fmeh. The original patch posting did:

out_unlock:
xfs_iunlock(ip, XFS_ILOCK_EXCL);
-out_trans_cancel:
- xfs_trans_cancel(tp);
+out_dqrele:
xfs_qm_dqrele(udqp);
xfs_qm_dqrele(gdqp);
return error;

Which leaked the transaction. Looks like I screwed up fixing that up on commit - it no longer leaked the transaction, but leaked the lock instead. And 3 and half years later someone notices it...

Oh, gawd that code is so grotty! I started saying "maybe we should..." and then stopped when I realised just how much cleanup needs to be done to that function...

The above patch fixes the issue, iso consider it:

Reviewed-by Dave Chinner <[email protected]>

Signature · 2 lines

Dave Chinner
[email protected]

Linus Torvalds

2019-08-23 16:29 UTC #4 lore raw

In reply to Darrick J. Wong

Quoted 4 lines · Darrick J. Wong

...which is clearly caused by xfs_setattr_nonsize failing to unlock the ILOCK after the xfs_qm_vop_chown_reserve call fails. Add the missing unlock.

Thanks for the quick fix.

I assume there's no real embargo on this, and we can just add it asap to the xfs tree and mark it for stable, rather than do anything outside the usual development path?

The security list rules do allow for a 5-working-day delay, but this being "just" a local DoS thing I expect nobody really will argue for it.

Anybody?

Linus

Benjamin Moody

2019-08-23 17:15 UTC #5 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 4 lines · Darrick J. Wong
...which is clearly caused by xfs_setattr_nonsize failing to unlock the ILOCK after the xfs_qm_vop_chown_reserve call fails. Add the missing unlock.

Quoted 5 lines

Thanks for the quick fix.
I assume there's no real embargo on this, and we can just add it asap to the xfs tree and mark it for stable, rather than do anything outside the usual development path?

As the person who reported the issue, I don't see a problem with that. I reported it to the Debian security team just to be on the safe side, in case the problem was something bigger than what I was seeing.

I haven't yet tested the patch, but thank you, Darrick, for the quick response!

Benjamin

Salvatore Bonaccorso

2019-08-23 19:26 UTC #6 lore raw

In reply to Linus Torvalds

Hi Linus,

Quoted 12 lines · Linus Torvalds

Quoted 4 lines · Darrick J. Wong
...which is clearly caused by xfs_setattr_nonsize failing to unlock the ILOCK after the xfs_qm_vop_chown_reserve call fails. Add the missing unlock.
Thanks for the quick fix.
I assume there's no real embargo on this, and we can just add it asap to the xfs tree and mark it for stable, rather than do anything outside the usual development path?

Quoted 5 lines

The security list rules do allow for a 5-working-day delay, but this being "just" a local DoS thing I expect nobody really will argue for it.
Anybody?

Agreed, there is no real embargo on this, it is public anyway on the xfs list and actually we just wanted to be on the safe side by asking or bringing it to [email protected].

Thanks for the quick fix.

Regards,
Salvatore

Salvatore Bonaccorso

2019-08-24 18:22 UTC #7 lore raw

Hi,

Quoted 19 lines · Salvatore Bonaccorso

Hi Linus,
Quoted 16 lines · Linus Torvalds
Quoted 4 lines · Darrick J. Wong
...which is clearly caused by xfs_setattr_nonsize failing to unlock the ILOCK after the xfs_qm_vop_chown_reserve call fails. Add the missing unlock.
Thanks for the quick fix.
I assume there's no real embargo on this, and we can just add it asap to the xfs tree and mark it for stable, rather than do anything outside the usual development path?
The security list rules do allow for a 5-working-day delay, but this being "just" a local DoS thing I expect nobody really will argue for it.

Quoted 7 lines

Quoted 1 line
Anybody?
Agreed, there is no real embargo on this, it is public anyway on the xfs list and actually we just wanted to be on the safe side by asking or bringing it to [email protected].
Thanks for the quick fix.

Not changing my mind on the above, given the fixing commit is public.

But thinking a bit more on it, I guess this should not considered local DoS only. If the filesystem is exosed then a remote user might trigger it serverside.

The issue could be reproduced on a NFS exported XFS as well. Just export the filesystem rw to another host via NFS and mount it there. A local user on the client could then trigger the issue on the server as well.

Regards,
Salvatore

Salvatore Bonaccorso

2019-08-23 19:24 UTC #8 lore raw

In reply to Darrick J. Wong

Hi Darrick,

Quoted 47 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
Benjamin Moody reported to Debian that XFS partially wedges when a chgrp fails on account of being out of disk quota. I ran his reproducer script:
# adduser dummy
# adduser dummy plugdev
# dd if=/dev/zero bs=1M count=100 of=test.img
# mkfs.xfs test.img
# mount -t xfs -o gquota test.img /mnt
# mkdir -p /mnt/dummy
# chown -c dummy /mnt/dummy
# xfs_quota -xc 'limit -g bsoft=100k bhard=100k plugdev' /mnt
(and then as user dummy)
$ dd if=/dev/urandom bs=1M count=50 of=/mnt/dummy/foo
$ chgrp plugdev /mnt/dummy/foo
and saw:
================================================
WARNING: lock held when returning to user space!
5.3.0-rc5 #rc5 Tainted: G W
------------------------------------------------
chgrp/47006 is leaving the kernel with locks still held!
1 lock held by chgrp/47006:
#0: 000000006664ea2d (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0xd2/0x290 [xfs]
...which is clearly caused by xfs_setattr_nonsize failing to unlock the ILOCK after the xfs_qm_vop_chown_reserve call fails. Add the missing unlock.
Reported-by [email protected]
Fixes 253f4911f297 ("xfs: better xfs_trans_alloc interface")
Signed-off-by Darrick J. Wong <[email protected]>
---
 fs/xfs/xfs_iops.c |    1 +
 1 file changed, 1 insertion(+)
fs/xfs/xfs_iops.c +0 -0
index dd4076ae228a..ea614b4ae052 100644

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

Quoted 7 lines

fs/xfs/xfs_iops.c +1 -0

@@ -804,6 +804,7 @@ xfs_setattr_nonsize(
 
 out_cancel:
 	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out_dqrele:
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);

Confirmed the fix work.

Feel free to add a Tested-by if wanted.

Can this be backported to the relevant stable versions as well?

Regards,
Salvatore

Linus Torvalds

2019-08-24 18:45 UTC #9 lore raw

Quoted 6 lines · Salvatore Bonaccorso

Confirmed the fix work.
Feel free to add a Tested-by if wanted.
Can this be backported to the relevant stable versions as well?

It's out there in my tree now. It's not explicitly marked for stable per se, but it does have the "Fixes:" tag which should mean that Greg and Sasha will pick it up automatically.

But just to make it explicit, since Greg is on the security list, Darrick's fix is commit 1fb254aa983b ("xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT").

Linus

Greg KH

2019-08-25 03:13 UTC #10 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 6 lines · Salvatore Bonaccorso
Confirmed the fix work.
Feel free to add a Tested-by if wanted.
Can this be backported to the relevant stable versions as well?

Quoted 7 lines

It's out there in my tree now. It's not explicitly marked for stable per se, but it does have the "Fixes:" tag which should mean that Greg and Sasha will pick it up automatically.
But just to make it explicit, since Greg is on the security list, Darrick's fix is commit 1fb254aa983b ("xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT").

Thanks, I'll pick this up.

Note, "Fixes:" will never guarantee that a patch ends up in a stable release. Sasha's scripts usually do a good job of catching a lot of them a week or so after-the-fact, but they are not guaranteed to do so. A CC: stable@ will always be caught by me, and I try to ensure that anything goes across the [email protected] list also gets picked up.

thanks,

greg k-h

Salvatore Bonaccorso

2019-08-25 15:45 UTC #11 lore raw

Hi Greg,

Quoted 17 lines · Greg KH

Quoted 8 lines · Linus Torvalds
Quoted 6 lines · Salvatore Bonaccorso
Confirmed the fix work.
Feel free to add a Tested-by if wanted.
Can this be backported to the relevant stable versions as well?
Quoted 7 lines
It's out there in my tree now. It's not explicitly marked for stable per se, but it does have the "Fixes:" tag which should mean that Greg and Sasha will pick it up automatically.
But just to make it explicit, since Greg is on the security list, Darrick's fix is commit 1fb254aa983b ("xfs: fix missing ILOCK unlock when xfs_setattr_nonsize fails due to EDQUOT").

Quoted 7 lines

Thanks, I'll pick this up.
Note, "Fixes:" will never guarantee that a patch ends up in a stable release. Sasha's scripts usually do a good job of catching a lot of them a week or so after-the-fact, but they are not guaranteed to do so. A CC: stable@ will always be caught by me, and I try to ensure that anything goes across the [email protected] list also gets picked up.

Thank you.

FTR, for those interested in tracking this fix for the DoS potential as vulnerability, MITRE has assigned CVE-2019-15538 for this issue.

Regards,
Salvatore