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 <ce3g8jdj@umail.furryterror.org>

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 52aab510aea0d158f0e8a898f69e8d51ef5b43fe..d22ae2dcbe054c6f436d777d09d7d1f95eaf97e3 100644 (file)
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -216,9 +216,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 | \
@@ -227,19 +227,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;
 }
 
@@ -288,6 +279,7 @@ 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;
 
@@ -298,8 +290,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;
 
@@ -355,6 +346,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)) {
@@ -421,18 +437,32 @@ 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 (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);
                prop_set = (ret == 0);
                /* If ret == -ENODATA ignore and proceed to update inode item. */
-               if (ret && ret != -ENODATA)
+               if (ret && ret != -ENODATA) {
+                       inode->flags = old_inode_flags;
                        goto out_end_trans;
+               }
        }
 
 update_flags: