Re: [PATCH v6 32/43] btrfs: implement process_bio cb for fscrypt

[Daniel Vacek <neelx@suse.com>]

On Sun, 8 Feb 2026 at 16:12, Chris Mason <clm@meta.com> wrote:
> Daniel Vacek <neelx@suse.com> wrote:
> > From: Josef Bacik <josef@toxicpanda.com>
> >
> > 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 <josef@toxicpanda.com>
>
> 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;
> > +}
>
> [ ... ]
>
>