xfs: Fail remount with noattr2 on a v5 with v4 enabled

Bug: When we compile the kernel with CONFIG_XFS_SUPPORT_V4=y,
remount with "-o remount,noattr2" on a v5 XFS does not
fail explicitly.

Reproduction:
mkfs.xfs -f /dev/loop0
mount /dev/loop0 /mnt/scratch
mount -o remount,noattr2 /dev/loop0 /mnt/scratch

However, with CONFIG_XFS_SUPPORT_V4=n, the remount
correctly fails explicitly. This is because the way the
following 2 functions are defined:

static inline bool xfs_has_attr2 (struct xfs_mount *mp)
{
	return !IS_ENABLED(CONFIG_XFS_SUPPORT_V4) ||
		(mp->m_features & XFS_FEAT_ATTR2);
}
static inline bool xfs_has_noattr2 (const struct xfs_mount *mp)
{
	return mp->m_features & XFS_FEAT_NOATTR2;
}

xfs_has_attr2() returns true when CONFIG_XFS_SUPPORT_V4=n
and hence, the following if condition in
xfs_fs_validate_params() succeeds and returns -EINVAL:

/*
 * We have not read the superblock at this point, so only the attr2
 * mount option can set the attr2 feature by this stage.
 */

if (xfs_has_attr2(mp) && xfs_has_noattr2(mp)) {
	xfs_warn(mp, "attr2 and noattr2 cannot both be specified.");
	return -EINVAL;
}

With CONFIG_XFS_SUPPORT_V4=y, xfs_has_attr2() always return
false and hence no error is returned.

Fix: Check if the existing mount has crc enabled(i.e, of
type v5 and has attr2 enabled) and the
remount has noattr2, if yes, return -EINVAL.

I have tested xfs/{189,539} in fstests with v4
and v5 XFS with both CONFIG_XFS_SUPPORT_V4=y/n and
they both behave as expected.

This patch also fixes remount from noattr2 -> attr2 (on a v4 xfs).

Related discussion in [1]

[1] https://lore.kernel.org/all/Z65o6nWxT00MaUrW@dread.disaster.area/

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Signed-off-by: Carlos Maiolino <cem@kernel.org>

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 3be041647ec17e3423cdfb5242e62a92a39ab8a0..4a11ddccc563ad3c2704a78243af98aea5181735 100644 (file)
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2114,6 +2114,21 @@ xfs_fs_reconfigure(
        if (error)
                return error;
 
+       /* attr2 -> noattr2 */
+       if (xfs_has_noattr2(new_mp)) {
+               if (xfs_has_crc(mp)) {
+                       xfs_warn(mp,
+                       "attr2 is always enabled for a V5 filesystem - can't be changed.");
+                       return -EINVAL;
+               }
+               mp->m_features &= ~XFS_FEAT_ATTR2;
+               mp->m_features |= XFS_FEAT_NOATTR2;
+       } else if (xfs_has_attr2(new_mp)) {
+               /* noattr2 -> attr2 */
+               mp->m_features &= ~XFS_FEAT_NOATTR2;
+               mp->m_features |= XFS_FEAT_ATTR2;
+       }
+
        /* inode32 -> inode64 */
        if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {
                mp->m_features &= ~XFS_FEAT_SMALL_INUMS;
@@ -2126,6 +2141,17 @@ xfs_fs_reconfigure(
                mp->m_maxagi = xfs_set_inode_alloc(mp, mp->m_sb.sb_agcount);
        }
 
+       /*
+        * Now that mp has been modified according to the remount options, we
+        * do a final option validation with xfs_finish_flags() just like it is
+        * just like it is done during mount. We cannot use
+        * done during mount. We cannot use xfs_finish_flags() on new_mp as it
+        * contains only the user given options.
+        */
+       error = xfs_finish_flags(mp);
+       if (error)
+               return error;
+
        /* ro -> rw */
        if (xfs_is_readonly(mp) && !(flags & SB_RDONLY)) {
                error = xfs_remount_rw(mp);