| Message ID | 7c4202348f67788db55c7ec445cbe3f2d587daf2.1744394929.git.nirjhar.roy.lists@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] xfs: Fail remount with noattr2 on a v5 xfs with CONFIG_XFS_SUPPORT_V4=y | expand |
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.
On 4/14/25 11:18, Christoph Hellwig wrote: > 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 Noted. I will update this in the next revision. > >> 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. So during mount, xfs_fs_fill_super() calls the following functions are called in sequence : xfs_fs_validate_params() <...> xfs_readsb() xfs_finish_flags(). If I am trying to "mount -o noattr2 /dev/loop0 /mnt1/test", then the invalid condition(noattr2 on v5) is not caught in xfs_fs_validate_params() because the superblock isn't read yet and "struct xfs_mount *mp" is still not aware of whether the underlying filesystem is v5 or v4 (i.e, whether crc=0 or crc=1). So, even if the underlying filesystem is v5, xfs_has_attr2() will return false, because the m_features isn't populated yet. However, once xfs_readsb() is done, m_features is populated (mp->m_features |= xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()). After that, when xfs_finish_flags() is called, the invalid mount option (i.e, noattr2 with crc=1) is caught, and the mount fails correctly. So, m_features is partially populated while xfs_fs_validate_params() is getting executed, I am not sure if that is done intentionally. IMO, we should have read the superblock, made sure that the m_features is fully populated within xfs_fs_validate_params() with the existing configurations of the underlying disk/fs and the ones supplied the by mount program - this can avoid such false negatives. Can you please let me know if my understanding is correct? > >> + /* 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; > } xfs_fs_validate_params() takes only one parameter. Are you suggesting to add another optional (NULLable) parameter "new_mp" and add the above check there? In that case, all other remount related checks in xfs_fs_reconfigure() qualify to be moved to xfs_fs_validate_params(), right? Is my understanding correct? > > >> + 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. Okay, I will fix this in the next revision. Thank you for pointing this out. --NR
On Tue, Apr 15, 2025 at 12:48:39PM +0530, Nirjhar Roy (IBM) wrote: > condition(noattr2 on v5) is not caught in xfs_fs_validate_params() because > the superblock isn't read yet and "struct xfs_mount *mp" is still not > aware of whether the underlying filesystem is v5 or v4 (i.e, whether crc=0 > or crc=1). So, even if the underlying filesystem is v5, xfs_has_attr2() will > return false, because the m_features isn't populated yet. Yes. > However, once > xfs_readsb() is done, m_features is populated (mp->m_features |= > xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()). After > that, when xfs_finish_flags() is called, the invalid mount option (i.e, > noattr2 with crc=1) is caught, and the mount fails correctly. So, m_features > is partially populated while xfs_fs_validate_params() is getting executed, I > am not sure if that is done intentionally. As you pointed out above it can't be fully populated becaue we don't have all the information. And that can't be fixed because some of the options are needed to even get to reading the superblock. So we do need a second pass of verification for everything that depends on informationtion from the superblock. The fact that m_features mixes user options and on-disk feature bits is unfortunately not very helpful for a clear structure here.
On 4/16/25 11:39, Christoph Hellwig wrote: > On Tue, Apr 15, 2025 at 12:48:39PM +0530, Nirjhar Roy (IBM) wrote: >> condition(noattr2 on v5) is not caught in xfs_fs_validate_params() because >> the superblock isn't read yet and "struct xfs_mount *mp" is still not >> aware of whether the underlying filesystem is v5 or v4 (i.e, whether crc=0 >> or crc=1). So, even if the underlying filesystem is v5, xfs_has_attr2() will >> return false, because the m_features isn't populated yet. > Yes. > >> However, once >> xfs_readsb() is done, m_features is populated (mp->m_features |= >> xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()). After >> that, when xfs_finish_flags() is called, the invalid mount option (i.e, >> noattr2 with crc=1) is caught, and the mount fails correctly. So, m_features >> is partially populated while xfs_fs_validate_params() is getting executed, I >> am not sure if that is done intentionally. > As you pointed out above it can't be fully populated becaue we don't > have all the information. And that can't be fixed because some of the > options are needed to even get to reading the superblock. > > So we do need a second pass of verification for everything that depends Yes, we need a second pass and I think that is already being done by xfs_finish_flags() in xfs_fs_fill_super(). However, in xfs_fs_reconfigure(), we still need a check to make a transition from /* attr2 -> noattr2 */ and /* noattr2 -> attr2 */ (only if it is permitted to) and update mp->m_features accordingly, just like it is being done for inode32 <-> inode64, right? Also, in your previous reply[1], you did suggest moving the crc+noattr2 check to xfs_fs_validate_params() - Are you suggesting to add another optional (NULLable) parameter "new_mp" to xfs_fs_validate_params() and then moving the check to xfs_fs_validate_params()? [1] https://lore.kernel.org/all/Z_yhpwBQz7Xs4WLI@infradead.org/ --NR > on informationtion from the superblock. The fact that m_features > mixes user options and on-disk feature bits is unfortunately not very > helpful for a clear structure here. >
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index b2dd0c0bf509..fd72c8fcd3a7 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -2114,6 +2114,23 @@ xfs_fs_reconfigure( if (error) return error; + /* 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; + } + 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; + } + /* inode32 -> inode64 */ if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) { mp->m_features &= ~XFS_FEAT_SMALL_INUMS;
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 # This should fail but it doesn't 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 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 is 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> --- fs/xfs/xfs_super.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)