On Sun, 8 Feb 2026 at 16:12, Chris Mason <[email protected]> wrote:
> Daniel Vacek <[email protected]> wrote:
> > From: Josef Bacik <[email protected]>
> >
> > We are going to be checksumming the encrypted data, so we have to
> > implement the ->process_bio fscrypt callback. This will provide us with
> > the original bio and the encrypted bio to do work on. For WRITE's this
> > will happen after the encrypted bio has been encrypted. For READ's this
> > will happen after the read has completed and before the decryption step
> > is done.
> >
> > For write's this is straightforward, we can just pass in the encrypted
> > bio to btrfs_csum_one_bio and then the csums will be added to the bbio
> > as normal.
> >
> > For read's this is relatively straightforward, but requires some care.
> > We assume (because that's how it works currently) that the encrypted bio
> > match the original bio, this is important because we save the iter of
> > the bio before we submit. If this changes in the future we'll need a
> > hook to give us the bi_iter of the decryption bio before it's submitted.
> > We check the csums before decryption. If it doesn't match we simply
> > error out and we let the normal path handle the repair work.
>
> 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 2b25418669e46c939ff864b1144e2c27d618920c
> Author: Josef Bacik <[email protected]>
>
> btrfs: implement process_bio cb for fscrypt
>
> This commit implements the ->process_bio fscrypt callback for btrfs,
> enabling checksum verification of encrypted data before decryption and
> checksum generation after encryption.
>
> > diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> > index 3b420866f3b82..c28f3f53bad28 100644
> > --- a/fs/btrfs/bio.c
> > +++ b/fs/btrfs/bio.c
> > @@ -300,6 +300,34 @@ static struct btrfs_failed_bio
> > *repair_one_sector(struct btrfs_bio *failed_bbio,
> > return fbio;
> > }
> >
> > +blk_status_t btrfs_check_encrypted_read_bio(struct btrfs_bio *bbio, struct
> > bio *enc_bio)
> > +{
> > + struct btrfs_inode *inode = bbio->inode;
> > + struct btrfs_fs_info *fs_info = inode->root->fs_info;
> > + struct bvec_iter iter = bbio->saved_iter;
> > + struct btrfs_device *dev = bbio->bio.bi_private;
> > + const u32 step = min(fs_info->sectorsize, PAGE_SIZE);
> > + const u32 nr_steps = iter.bi_size / step;
> > + phys_addr_t paddrs[BTRFS_MAX_BLOCKSIZE / PAGE_SIZE];
>
> Can paddrs[] overflow here? BTRFS_MAX_BLOCKSIZE is 64KB, so the array
> has 16 elements (64KB / 4KB). However, nr_steps is calculated from
> iter.bi_size which can be much larger than 64KB for merged bios.
>
> If a bio has bi_size > 64KB, the loop below will write beyond the array
> bounds when filling paddrs[slot].
Indeed, that's very true and I can easily reproduce it.
> > + phys_addr_t paddr;
> > + unsigned int slot = 0;
> > +
> > + /*
> > + * We have to use a copy of iter in case there's an error,
> > + * btrfs_check_read_bio will handle submitting the repair bios.
> > + */
> > + btrfs_bio_for_each_block(paddr, enc_bio, &iter, step) {
> > + ASSERT(slot < nr_steps);
> > + paddrs[slot] = paddr;
> > + slot++;
> > + }
> > + if (!btrfs_data_csum_ok(bbio, dev, 0, paddrs))
>
> Does this verify checksums for all sectors in the bio? Looking at
> btrfs_data_csum_ok() in inode.c, it verifies one sector's worth of data
> (one blocksize). The function is called with bio_offset=0, meaning it
> only checksums the first sector.
>
> However, the bio can contain multiple sectors (iter.bi_size could be
> much larger than one sectorsize). If the bio has multiple sectors,
> shouldn't this call btrfs_data_csum_ok() for each sector, or pass the
> actual bio size so all sectors are verified?
>
> The existing btrfs_check_read_bio() function loops through all sectors
> and verifies each one individually.
This is also correct. Both points fixed this way:
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -343,11 +343,13 @@ blk_status_t
btrfs_check_encrypted_read_bio(struct btrfs_bio *bbio, struct bio *
struct btrfs_fs_info *fs_info = inode->root->fs_info;
struct bvec_iter iter = bbio->saved_iter;
struct btrfs_device *dev = bbio->bio.bi_private;
- const u32 step = min(fs_info->sectorsize, PAGE_SIZE);
+ const u32 blocksize = fs_info->sectorsize;
+ const u32 step = min(blocksize, PAGE_SIZE);
const u32 nr_steps = iter.bi_size / step;
phys_addr_t paddrs[BTRFS_MAX_BLOCKSIZE / PAGE_SIZE];
phys_addr_t paddr;
unsigned int slot = 0;
+ u32 offset = 0;
/*
* We have to use a copy of iter in case there's an error,
@@ -357,9 +359,13 @@ blk_status_t
btrfs_check_encrypted_read_bio(struct btrfs_bio *bbio, struct bio *
ASSERT(slot < nr_steps);
paddrs[slot] = paddr;
slot++;
+ offset += step;
+ if (IS_ALIGNED(offset, blocksize)) {
+ if (!btrfs_data_csum_ok(bbio, dev, offset - blocksize, paddrs))
+ return BLK_STS_IOERR;
+ slot = 0;
+ }
}
- if (!btrfs_data_csum_ok(bbio, dev, 0, paddrs))
- return BLK_STS_IOERR;
/*
* Read repair is slightly different for encrypted bio's. This
Thanks!
--nX
> > + return BLK_STS_IOERR;
> > +
> > + bbio->csum_ok = true;
> > + return BLK_STS_OK;
> > +}
>
> [ ... ]
>
>
