linux-ext4 - Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_yhpwBQz7Xs4WLI@infradead.org>
Date: Sun, 13 Apr 2025 22:48:23 -0700
From: Christoph Hellwig <hch@...radead.org>
To: "Nirjhar Roy (IBM)" <nirjhar.roy.lists@...il.com>
Cc: linux-xfs@...r.kernel.org, linux-ext4@...r.kernel.org,
	fstests@...r.kernel.org, ritesh.list@...il.com,
	ojaswin@...ux.ibm.com, djwong@...nel.org, zlang@...nel.org,
	david@...morbit.com
Subject: Re: [PATCH v1] xfs: Fail remount with noattr2 on a v5 xfs with
 CONFIG_XFS_SUPPORT_V4=y

On Fri, Apr 11, 2025 at 11:44:52PM +0530, Nirjhar Roy (IBM) wrote:
> mkfs.xfs -f /dev/loop0
> mount /dev/loop0 /mnt/scratch
> mount -o remount,noattr2 /dev/loop0 /mnt/scratch # This should fail but it doesn't

Please reflow your commit log to not exceed the standard 73 characters

> xfs_has_attr2() returns true when CONFIG_XFS_SUPPORT_V4=n and hence, the
> 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.

But that also means the mount time check is wrong as well.

> +	/* attr2 -> noattr2 */
> +	if (xfs_has_noattr2(new_mp)) {
> +		if (xfs_has_crc(mp)) {
> +			xfs_warn(mp, "attr2 and noattr2 cannot both be specified.");
> +			return -EINVAL;
> +		}

So this check should probably go into xfs_fs_validate_params, and
also have a more useful warning like:

	if (xfs_has_crc(mp) && xfs_has_noattr2(new_mp)) {
		xfs_warn(mp,
"noattr2 cannot be specified for v5 file systems.");
                return -EINVAL;
	}


> +		else {
> +			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;
> +	}

Some of the indentation here looks broken.  Please always use one
tab per indentation level, places the closing brace before the else,
and don't use else after a return statement.