[PATCH] xfs: don't expose misaligned extszinherit hints to userspace

[PATCH] xfs: don't expose misaligned extszinherit hints to userspace

[Darrick J. Wong]

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

Commit 603f000b15f2 changed xfs_ioctl_setattr_check_extsize to reject an
attempt to set an EXTSZINHERIT extent size hint on a directory with
RTINHERIT set if the hint isn't a multiple of the realtime extent size.
However, I have recently discovered that it is possible to change the
realtime extent size when adding a rt device to a filesystem, which
means that the existence of directories with misaligned inherited hints
is not an accident.

As a result, it's possible that someone could have set a valid hint and
added an rt volume with a different rt extent size, which invalidates
the ondisk hints.  After such a sequence, FSGETXATTR will report a
misaligned hint, which FSSETXATTR will trip over, causing confusion if
the user was doing the usual GET/SET sequence to change some other
attribute.  Change xfs_fill_fsxattr to omit the hint if it isn't aligned
properly.

Fixes: 603f000b15f2 ("xfs: validate extsz hints against rt extent size when rtinherit is set")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_ioctl.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index cfc2e099d558..72441c7be644 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1065,7 +1065,24 @@ xfs_fill_fsxattr(
 
 	fileattr_fill_xflags(fa, xfs_ip2xflags(ip));
 
-	fa->fsx_extsize = XFS_FSB_TO_B(mp, ip->i_extsize);
+	if (ip->i_diflags & XFS_DIFLAG_EXTSIZE) {
+		fa->fsx_extsize = XFS_FSB_TO_B(mp, ip->i_extsize);
+	} else if (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
+		/*
+		 * Don't let a misaligned extent size hint on a directory
+		 * escape to userspace if it won't pass the setattr checks
+		 * later.
+		 */
+		if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
+		    ip->i_extsize % mp->m_sb.sb_rextsize > 0) {
+			fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE |
+					    FS_XFLAG_EXTSZINHERIT);
+			fa->fsx_extsize = 0;
+		} else {
+			fa->fsx_extsize = XFS_FSB_TO_B(mp, ip->i_extsize);
+		}
+	}
+
 	if (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE)
 		fa->fsx_cowextsize = XFS_FSB_TO_B(mp, ip->i_cowextsize);
 	fa->fsx_projid = ip->i_projid;

[RFC PATCH] xfs: test adding realtime sections to filesystem

[Darrick J. Wong]

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

Add a functional test to exercise using "xfs_growfs -e XXX -r" to add a
realtime section to a filesystem while changing the extent size.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/779     |  112 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/779.out |    2 +
 2 files changed, 114 insertions(+)
 create mode 100755 tests/xfs/779
 create mode 100644 tests/xfs/779.out

diff --git a/tests/xfs/779 b/tests/xfs/779
new file mode 100755
index 00000000..1cb8f75a
--- /dev/null
+++ b/tests/xfs/779
@@ -0,0 +1,112 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Oracle.  All Rights Reserved.
+#
+# FS QA Test 779
+#
+# Test for xfs_growfs to make sure that we can add a realtime device and set
+# its extent size hint at the same time.  This also checks for the presence of
+# these patches:
+#
+#	xfs: improve FSGROWFSRT precondition checking
+#	xfs: fix an integer overflow error in xfs_growfs_rt
+#	xfs: correct the narrative around misaligned rtinherit/extszinherit dirs
+#	xfs: don't expose misaligned extszinherit hints to userspace
+#
+. ./common/preamble
+_begin_fstest auto quick realtime growfs
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_realtime
+_require_scratch
+
+# Format scratch fs with no realtime section.
+SCRATCH_RTDEV="" _scratch_mkfs | _filter_mkfs 2> $tmp.mkfs >> $seqres.full
+_scratch_mount
+
+# Check that there's no realtime section.
+source $tmp.mkfs
+test $rtblocks -eq 0 || echo "expected 0 rtblocks, got $rtblocks"
+
+# Compute a new rt extent size and a separate rt extent size hint to exercise
+# the code that ignores hints that aren't a multiple of the extent size.
+XFS_MAX_RTEXTSIZE=$((1024 * 1024 * 1024))
+new_rtextsz=$((rtextsz + dbsize))
+if [ $new_rtextsz -ge $XFS_MAX_RTEXTSIZE ]; then
+	new_rtextsz=$((rtextsz - dbsize))
+fi
+new_rtextsz_blocks=$(( new_rtextsz / dbsize ))
+
+new_extszhint=$((rtextsz * 2))
+if [ $new_extszhint -eq $new_rtextsz ]; then
+	new_extszhint=$((rtextsz * 3))
+fi
+
+# Set the inheritable extent size hint and rt status.
+$XFS_IO_PROG -c 'chattr +t' -c "extsize $new_extszhint" $SCRATCH_MNT
+
+# Check that the hint was set correctly
+after_extszhint=$($XFS_IO_PROG -c 'stat' $SCRATCH_MNT | \
+	grep 'fsxattr.extsize' | cut -d ' ' -f 3)
+test $after_extszhint -eq $new_extszhint || \
+	echo "expected extszhint $new_extszhint, got $after_extszhint"
+
+# Add a realtime section and change the extent size.
+echo $XFS_GROWFS_PROG -e $new_rtextsz_blocks -r $SCRATCH_MNT >> $seqres.full
+$XFS_GROWFS_PROG -e $new_rtextsz_blocks -r $SCRATCH_MNT >> $seqres.full 2> $tmp.growfs
+res=$?
+cat $tmp.growfs
+
+# If the growfs failed, skip the post-test check because the scratch fs does
+# not have SCRATCH_RTDEV configured.  If the kernel didn't support adding the
+# rt volume, skip everything else.
+if [ $res -ne 0 ]; then
+	rm -f ${RESULT_DIR}/require_scratch
+	if grep -q "Operation not supported" $tmp.growfs; then
+		_notrun "growfs not supported on rt volume"
+	fi
+fi
+
+# Now that the root directory's extsize hint is no longer aligned to the rt
+# extent size, check that we don't report it to userspace any more.
+grow_extszhint=$($XFS_IO_PROG -c 'stat' $SCRATCH_MNT | \
+	grep 'fsxattr.extsize' | cut -d ' ' -f 3)
+test $grow_extszhint -eq 0 || \
+	echo "expected post-grow extszhint 0, got $grow_extszhint"
+
+# Check that we now have rt extents.
+rtextents=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | \
+	grep 'geom.rtextents' | cut -d ' ' -f 3)
+test $rtextents -gt 0 || echo "expected rtextents > 0"
+
+# Check the new rt extent size.
+after_rtextsz_blocks=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | \
+	grep 'geom.rtextsize' | cut -d ' ' -f 3)
+test $after_rtextsz_blocks -eq $new_rtextsz_blocks || \
+	echo "expected rtextsize $new_rtextsz_blocks, got $after_rtextsz_blocks"
+
+# Create a new realtime file to prove that we can.
+echo moo > $SCRATCH_MNT/a
+sync
+$XFS_IO_PROG -c 'lsattr -v' $SCRATCH_MNT/a | \
+	cut -d ' ' -f 1 | \
+	grep -q realtime || \
+	echo "$SCRATCH_MNT/a is not a realtime file?"
+
+# Check that the root directory's hint (which was aligned before the grow and
+# misaligned after) did not propagate to the new realtime file.
+file_extszhint=$($XFS_IO_PROG -c 'stat' $SCRATCH_MNT/a | \
+	grep 'fsxattr.extsize' | cut -d ' ' -f 3)
+test $file_extszhint -eq 0 || \
+	echo "expected file extszhint 0, got $file_extszhint"
+
+# success, all done
+echo Silence is golden
+status=0
+exit
diff --git a/tests/xfs/779.out b/tests/xfs/779.out
new file mode 100644
index 00000000..1f79fae2
--- /dev/null
+++ b/tests/xfs/779.out
@@ -0,0 +1,2 @@
+QA output created by 779
+Silence is golden

Re: [PATCH] xfs: don't expose misaligned extszinherit hints to userspace

[Christoph Hellwig]

On Thu, Jul 08, 2021 at 09:12:09PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Commit 603f000b15f2 changed xfs_ioctl_setattr_check_extsize to reject an
> attempt to set an EXTSZINHERIT extent size hint on a directory with
> RTINHERIT set if the hint isn't a multiple of the realtime extent size.
> However, I have recently discovered that it is possible to change the
> realtime extent size when adding a rt device to a filesystem, which
> means that the existence of directories with misaligned inherited hints
> is not an accident.
> 
> As a result, it's possible that someone could have set a valid hint and
> added an rt volume with a different rt extent size, which invalidates
> the ondisk hints.  After such a sequence, FSGETXATTR will report a
> misaligned hint, which FSSETXATTR will trip over, causing confusion if
> the user was doing the usual GET/SET sequence to change some other
> attribute.  Change xfs_fill_fsxattr to omit the hint if it isn't aligned
> properly.

Looks sensible, but maybe we need a pr_info_ratelimited to inform
the user of this case?

Re: [PATCH] xfs: don't expose misaligned extszinherit hints to userspace

[Darrick J. Wong]

On Fri, Jul 09, 2021 at 07:25:09AM +0100, Christoph Hellwig wrote:
> On Thu, Jul 08, 2021 at 09:12:09PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Commit 603f000b15f2 changed xfs_ioctl_setattr_check_extsize to reject an
> > attempt to set an EXTSZINHERIT extent size hint on a directory with
> > RTINHERIT set if the hint isn't a multiple of the realtime extent size.
> > However, I have recently discovered that it is possible to change the
> > realtime extent size when adding a rt device to a filesystem, which
> > means that the existence of directories with misaligned inherited hints
> > is not an accident.
> > 
> > As a result, it's possible that someone could have set a valid hint and
> > added an rt volume with a different rt extent size, which invalidates
> > the ondisk hints.  After such a sequence, FSGETXATTR will report a
> > misaligned hint, which FSSETXATTR will trip over, causing confusion if
> > the user was doing the usual GET/SET sequence to change some other
> > attribute.  Change xfs_fill_fsxattr to omit the hint if it isn't aligned
> > properly.
> 
> Looks sensible, but maybe we need a pr_info_ratelimited to inform
> the user of this case?

Why?  Now that I've established that the system administrator is and
always has been allowed to invalidate the extent size hints when
realtime volumes are in play, I don't think we need to spam the kernel
log about the admin's strange choices.

The only reason I put that xfs_info_once thing in 603f00 is because I
mistakenly thought that the only way we could end up with a fs like that
was due to gaps in user input sanitization, i.e. fs isn't supposed to be
weird, but it is anyway.

--D

Re: [PATCH] xfs: don't expose misaligned extszinherit hints to userspace

[Christoph Hellwig]

On Fri, Jul 09, 2021 at 08:58:31PM -0700, Darrick J. Wong wrote:
> > Looks sensible, but maybe we need a pr_info_ratelimited to inform
> > the user of this case?
> 
> Why?  Now that I've established that the system administrator is and
> always has been allowed to invalidate the extent size hints when
> realtime volumes are in play, I don't think we need to spam the kernel
> log about the admin's strange choices.
> 
> The only reason I put that xfs_info_once thing in 603f00 is because I
> mistakenly thought that the only way we could end up with a fs like that
> was due to gaps in user input sanitization, i.e. fs isn't supposed to be
> weird, but it is anyway.

Ok:

Reviewed-by: Christoph Hellwig <hch@lst.de>