From 857fbba8f85f6355b83890867e727dfd9a34664d Mon Sep 17 00:00:00 2001 From: Zygo Blaxell Date: Sat, 16 Aug 2025 22:20:01 -0400 Subject: [PATCH] btrfs: fix nodatacow vs compression inode flag conflict check Applications expect inode flags to be orthogonal: changes can be combined or applied separately in any order, as long as each intermediate state is valid and unchanged flags are left untouched. Commit f37c563bab42 ("btrfs: add missing check for nocow and compression inode flags") intended to forbid combining FS_NOCOW_FL with either FS_NOCOMP_FL or FS_COMPR_FL. The implementation contained a bug and introduced multiple regressions. Bug: FS_NOCOW_FL (+C) and FS_NOCOMP_FL (+m) can still be set together in a single FS_IOC_SETFLAGS call, even though the commit message states this combination should be rejected. Regression 1: Switching from +C+m back to -C-m only works if both flag changes are combined into one ioctl; separate -C and -m calls are rejected. Regression 2: Switching between +C-c and -C+c only works if the changes are split across multiple ioctls; a combined -C+c call is rejected. Regression 3: Inodes created on kernels before commit f37c563bab42 ("btrfs: add missing check for nocow and compression inode flags") with both COMPR and NOCOW set cannot have any other fsattrs changed on newer kernels. Even unrelated operations (such as adding +i, or clearing just one of the compression bits) fail with EINVAL, because the conflict check rejects the entire ioctl whenever conflicting bits are present, even if those bits are not being modified. This makes it impossible to manage older files without first undoing their existing flags. Fix by: * Rewriting the conflict checks so FS_NOCOW_FL cannot be combined with FS_NOCOMP_FL or FS_COMPR_FL in any ioctl (fixes the original bug and regressions 1-2). * Allowing existing conflicting flags to remain if they are not modified by the ioctl (fixes regression 3). * Moving the check later in the flag-validation sequence so that it occurs after handling the long-standing rule that FS_NOCOW_FL changes are silently ignored on non-empty files. This preserves the pre-existing behavior while still applying the corrected conflict logic. Also commit the new inode flags to the inode before setting the compression property, restoring them if the property cannot be set: property validation rejects compression on nodatacow inodes based on the inode's current flags, which would otherwise reject a single ioctl that both clears NOCOW and sets COMPR (regression 2) even with the conflict check corrected. Fixes: f37c563bab42 ("btrfs: add missing check for nocow and compression inode flags") Signed-off-by: Zygo Blaxell --- fs/btrfs/ioctl.c | 72 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 18 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 5039db10a47e8..9e99236d3989a 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -191,9 +191,9 @@ void btrfs_sync_inode_flags_to_i_flags(struct btrfs_inode *inode) /* * Check if @flags are a supported and valid set of FS_*_FL flags and that - * the old and new flags are not conflicting + * the flags are not conflicting */ -static int check_fsflags(unsigned int old_flags, unsigned int flags) +static int check_fsflags(unsigned int flags) { if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | \ FS_NOATIME_FL | FS_NODUMP_FL | \ @@ -202,19 +202,10 @@ static int check_fsflags(unsigned int old_flags, unsigned int flags) FS_NOCOW_FL)) return -EOPNOTSUPP; - /* COMPR and NOCOMP on new/old are valid */ + /* Only one of COMPR and NOCOMP is valid at a time */ if ((flags & FS_NOCOMP_FL) && (flags & FS_COMPR_FL)) return -EINVAL; - if ((flags & FS_COMPR_FL) && (flags & FS_NOCOW_FL)) - return -EINVAL; - - /* NOCOW and compression options are mutually exclusive */ - if ((old_flags & FS_NOCOW_FL) && (flags & (FS_COMPR_FL | FS_NOCOMP_FL))) - return -EINVAL; - if ((flags & FS_NOCOW_FL) && (old_flags & (FS_COMPR_FL | FS_NOCOMP_FL))) - return -EINVAL; - return 0; } @@ -263,7 +254,9 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, unsigned int fsflags, old_fsflags; int ret; const char *comp = NULL; + u32 old_inode_flags; u32 inode_flags; + bool prop_set = false; if (btrfs_root_readonly(root)) return -EROFS; @@ -272,8 +265,7 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, return -EOPNOTSUPP; fsflags = btrfs_mask_fsflags_for_type(&inode->vfs_inode, fa->flags); - old_fsflags = btrfs_inode_flags_to_fsflags(inode); - ret = check_fsflags(old_fsflags, fsflags); + ret = check_fsflags(fsflags); if (ret) return ret; @@ -329,6 +321,31 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, } else { inode_flags |= BTRFS_INODE_NODATACOW; } + /* + * NODATACOW files will ignore COMPRESS, so forbid + * COMPRESS | NODATACOW. + * + * We do this check here, after we've decided whether + * to silently drop the NODATACOW bit for non-empty files. + * + * NODATACOW files will inherit flags from NODATACOW + * directories, so COMPRESS is forbidden for them too. + * + * Old kernels allowed the forbidden combination of flags + * to be set, and they may still be found on existing + * filesystems. If the forbidden combination of flags + * are already set, we will allow an application to set + * or clear unrelated flags (like IMMUTABLE or NOATIME) + * as long as the existing COMPRESS | NODATACOW flags + * are not changed. + */ + if ((inode_flags & BTRFS_INODE_NODATACOW) && + (fsflags & (FS_COMPR_FL | FS_NOCOMP_FL))) { + old_fsflags = btrfs_inode_flags_to_fsflags(inode); + if ((old_fsflags & (FS_NOCOW_FL | FS_COMPR_FL | FS_NOCOMP_FL)) != + (fsflags & (FS_NOCOW_FL | FS_COMPR_FL | FS_NOCOMP_FL))) + return -EINVAL; + } } else { /* We can only change NODATACOW for zero-sized regular file. */ if (S_ISREG(inode->vfs_inode.i_mode) && (inode->vfs_inode.i_size == 0)) { @@ -382,17 +399,30 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, if (IS_ERR(trans)) return PTR_ERR(trans); + /* + * Commit the new flags to the inode before setting the compression + * property: property validation rejects compression on nodatacow + * inodes based on the inode's current flags, which would incorrectly + * reject a single ioctl that both clears NOCOW and sets COMPR. + * Restore the original flags if the property cannot be set. + */ + old_inode_flags = inode->flags; + inode->flags = inode_flags; + if (comp) { ret = btrfs_set_prop(trans, inode, "btrfs.compression", comp, strlen(comp), 0); - if (unlikely(ret)) { - btrfs_abort_transaction(trans, ret); + if (ret) { + inode->flags = old_inode_flags; goto out_end_trans; } + prop_set = true; } else { ret = btrfs_set_prop(trans, inode, "btrfs.compression", NULL, 0, 0); - if (unlikely(ret && ret != -ENODATA)) { - btrfs_abort_transaction(trans, ret); + prop_set = (ret == 0); + /* If ret == -ENODATA ignore and proceed to update inode item. */ + if (ret && ret != -ENODATA) { + inode->flags = old_inode_flags; goto out_end_trans; } } @@ -404,6 +434,12 @@ update_flags: inode_inc_iversion(&inode->vfs_inode); inode_set_ctime_current(&inode->vfs_inode); ret = btrfs_update_inode(trans, inode); + /* + * If we set a property or deleted one, we must abort if we fail to + * update the inode, to avoid persisting an inconsistent state. + */ + if (unlikely(ret && prop_set)) + btrfs_abort_transaction(trans, ret); out_end_trans: btrfs_end_transaction(trans); -- 2.53.0