Re: [PATCH v6 42/43] btrfs: disable encryption on RAID5/6
Daniel Vacek <[email protected]> Thu, 26 Mar 2026 17:16:24 +0100
| Newsgroups | org.kernel.vger.linux-fscrypt,org.kernel.vger.linux-block,org.kernel.vger.linux-btrfs,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <CAPjX3FexW6D4m23OQQ0bNZRCJRfx0Armr3Lb8nvDR_qNMx_PeA@mail.gmail.com> |
On Sun, 8 Feb 2026 at 14:18, Chris Mason <[email protected]> wrote: > Daniel Vacek <[email protected]> wrote: > > From: Josef Bacik <[email protected]> > > > > The RAID5/6 code will re-arrange bios and submit them through a > > different mechanism. This is problematic with inline encryption as we > > have to get the bio and csum it after it's been encrypted, and the > > radi5/6 bio's don't have the btrfs_bio embedded, so we have no way to > > get the csums put on disk. > > > > This isn't an unsolvable problem, but would require a bit of reworking. > > Since we discourage users from using this code currently simply don't > > allow encryption on RAID5/6 setups. If there's sufficient demand in the > > future we can add the support for this. > > > > Hi everyone, > > I'm running the btrfs for-next branch, along with the encryption patch set > through my AI review prompts: > > https://github.com/masoncl/review-prompts > > I'm working on some btrfs specific instructions, but hopefully these reviews > are already useful. I've kept the headers to make it easier to double check > that I'm actually replying to the right email, but please let me know if any > parts of this are incorrect: > > > commit 6a39a21e95e116ad02c6ce9f832838acc111692f > Author: Josef Bacik <[email protected]> > > btrfs: disable encryption on RAID5/6 > > The RAID5/6 code will re-arrange bios and submit them through a > different mechanism to handle parity. This is problematic with inline > encryption as we have to get the bio and csum it after it's been > encrypted, and the RAID5/6 bios don't have the btrfs_bio embedded, so > there's no way to get the csums put on disk. > > > commit 6a39a21e95e116ad02c6ce9f832838acc111692f > > Author: Josef Bacik <[email protected]> > > Date: Fri Feb 6 19:23:14 2026 +0100 > > > > btrfs: disable encryption on RAID5/6 > > > > The RAID5/6 code will re-arrange bios and submit them through a > > different mechanism. This is problematic with inline encryption as we > > have to get the bio and csum it after it's been encrypted, and the > > radi5/6 bio's don't have the btrfs_bio embedded, so we have no way to > ^^^^ > > This isn't a bug, but there's a typo: 'radi5/6' should be 'raid5/6'. > > [ ... ] > > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index a8adf99ad0a81..1bade8fea16e8 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -5198,6 +5198,10 @@ long btrfs_ioctl(struct file *file, unsigned int > > return -EOPNOTSUPP; > > if (sb_rdonly(fs_info->sb)) > > return -EROFS; > > + if (btrfs_fs_incompat(fs_info, RAID56)) { > > + btrfs_warn(fs_info, "can't enable encryption with RAID5/6"); > > + return -EINVAL; > > + } > > This check prevents enabling encryption when RAID56 is already active, > but does NOT prevent converting to RAID5/6 after encryption is enabled. > The check_raid56_incompat_flag() function in fs/btrfs/volumes.c:5558 > sets the RAID56 flag during balance operations without checking if > ENCRYPT is already set. This allows users to: > > 1. Enable encryption via FS_IOC_SET_ENCRYPTION_POLICY (sets ENCRYPT at > line 5210 in this function) > 2. Later run btrfs balance to convert to RAID5/6 (sets RAID56 flag at > volumes.c:5558) > 3. End up with both ENCRYPT and RAID56 flags set Indeed this should be sealed. I think this will do: --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -5742,6 +5742,11 @@ struct btrfs_block_group *btrfs_create_chunk(struct btrfs_trans_handle *trans, lockdep_assert_held(&info->chunk_mutex); + if (btrfs_fs_incompat(info, ENCRYPT)) { + btrfs_warn(info, "RAID5/6 not yet supported on encrypted filesystem"); + return ERR_PTR(-EINVAL); + } + if (!alloc_profile_is_valid(type, 0)) { DEBUG_WARN("invalid alloc profile for type %llu", type); return ERR_PTR(-EINVAL); > The commit message states "simply don't allow encryption on RAID5/6 > setups" which implies preventing the incompatible state entirely, not > just one entry path. The volumes.c code path needs a corresponding > check: > > static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type) > { > if (!(type & BTRFS_BLOCK_GROUP_RAID56_MASK)) > return; > // MISSING: check for ENCRYPT flag here > btrfs_set_fs_incompat(info, RAID56); > } > > Without this check, data corruption can occur via the RAID5/6 bio path > that cannot handle encrypted data properly, as explained in the commit > message. > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > index 4a2887147ead4..aefcbe56e85ad 100644 > > --- a/fs/btrfs/super.c > > +++ b/fs/btrfs/super.c > > @@ -734,6 +734,12 @@ bool btrfs_check_options(const struct btrfs_fs_info *info, > > if (btrfs_check_mountopts_zoned(info, mount_opt)) > > ret = false; > > > > + if (btrfs_fs_incompat(info, RAID56) && > > + btrfs_raw_test_opt(*mount_opt, TEST_DUMMY_ENCRYPTION)) { > > + btrfs_err(info, "cannot use test_dummy_encryption with RAID5/6"); > > + ret = false; > > + } > > + > > Similar to the ioctl.c issue, this prevents mounting with > test_dummy_encryption when RAID56 is already active, but does NOT > prevent converting to RAID5/6 after mounting with test_dummy_encryption. > > The check_raid56_incompat_flag() function in fs/btrfs/volumes.c:5558 > sets the RAID56 flag during balance operations without checking if > test_dummy_encryption is in use. Since test_dummy_encryption cannot be > changed after mount (enforced by btrfs_check_test_dummy_encryption at > line 1547-1560), a user can: > > 1. Mount filesystem with -o test_dummy_encryption > 2. Later run btrfs balance to convert to RAID5/6 (sets RAID56 flag) > 3. End up with both test_dummy_encryption active and RAID56 flag set > > While test_dummy_encryption is primarily a testing/development feature, > the patch explicitly protects against this combination, indicating it's > an important incompatibility. The volumes.c code path needs a check for > the TEST_DUMMY_ENCRYPTION mount option: > > static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type) > { > if (!(type & BTRFS_BLOCK_GROUP_RAID56_MASK)) > return; > // MISSING: check for TEST_DUMMY_ENCRYPTION mount option > btrfs_set_fs_incompat(info, RAID56); > } > > Does this allow the same encrypted bio / RAID5/6 bio incompatibility > described in the commit message? The above fix also covers this case. Thanks. --nX