Re: [PATCH v6 33/43] btrfs: implement read repair for encryption

[Daniel Vacek]

On Sun, 8 Feb 2026 at 16:09, Chris Mason <clm@xxxxxxxx> wrote:
> Daniel Vacek <neelx@xxxxxxxx> wrote:
> > From: Josef Bacik <josef@xxxxxxxxxxxxxx>
> >
> > In order to do read repair we will allocate sectorsize bio's and read
> > them one at a time, repairing any sectors that don't match their csum.
> > In order to do this we re-submit the IO's after it's failed, and at this
> > point we still need the fscrypt_extent_info for these new bio's.
> >
> > Add the fscrypt_extent_info to the read part of the union in the
> > btrfs_bio, and then pass this through all the places where we do reads.
> > Additionally add the orig_start, because we need to be able to put the
> > correct extent offset for the encryption context.
> >
> > With these in place we can utilize the normal read repair path.  The
> > only exception is that the actual repair of the bad copies has to be
> > triggered from the ->process_bio callback, because this is the encrypted
> > data.  If we waited until the end_io we would have the decrypted data
> > and we don't want to write that to the disk.  This is the only change to
> > the normal read repair path, we trigger the fixup of the broken sectors
> > in ->process_bio, and then we skip that part if we successfully repair
> > the sector in ->process_bio once we get to the endio.
>
> 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 096c17a6a951ed0c1100e863c4ee3a95bab0ad51
> Author: Josef Bacik <josef@xxxxxxxxxxxxxx>
>
> btrfs: implement read repair for encryption
>
> This commit implements read repair functionality for encrypted btrfs
> files by adding fscrypt_extent_info tracking to the btrfs_bio structure
> and handling repair operations before decryption occurs.
>
> > diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> > index c28f3f53bad28..e82de96b533cb 100644
> > --- a/fs/btrfs/bio.c
> > +++ b/fs/btrfs/bio.c
>
> [ ... ]
>
> > @@ -293,6 +323,13 @@ static struct btrfs_failed_bio *repair_one_sector(struct btrfs_bio *failed_bbio,
> >       repair_bbio = btrfs_bio(repair_bio);
> >       btrfs_bio_init(repair_bbio, failed_bbio->inode, failed_bbio->file_offset + bio_offset,
> >                      NULL, fbio);
> > +     repair_bbio->fscrypt_info = fscrypt_get_extent_info(failed_bbio->fscrypt_info);
> > +     repair_bbio->orig_start = failed_bbio->orig_start;
> > +
> > +     btrfs_set_bio_crypt_ctx_from_extent(repair_bio, repair_bbio->inode,
> > +                                         failed_bbio->fscrypt_info,
> > +                                         repair_bbio->file_offset -
> > +                                         failed_bbio->orig_start);
> >
> >       mirror = next_repair_mirror(fbio, failed_bbio->mirror_num);
> >       btrfs_debug(fs_info, "submitting repair read to mirror %d", mirror);
>
> [ ... ]
>
> > @@ -214,17 +244,17 @@ static void btrfs_end_repair_bio(struct btrfs_bio *repair_bbio,
> >                       goto done;
> >               }
> >
> > +             btrfs_set_bio_crypt_ctx_from_extent(&repair_bbio->bio,
> > +                                                 repair_bbio->inode,
> > +                                                 repair_bbio->fscrypt_info,
> > +                                                 repair_bbio->file_offset -
> > +                                                 repair_bbio->orig_start);
> > +
> >               btrfs_submit_bbio(repair_bbio, mirror);
> >               return;
> >       }
> >
> > -     do {
> > -             mirror = prev_repair_mirror(fbio, mirror);
> > -             btrfs_repair_io_failure(fs_info, btrfs_ino(inode),
> > -                               repair_bbio->file_offset, fs_info->sectorsize,
> > -                               logical, paddrs, step, mirror);
> > -     } while (mirror != fbio->bbio->mirror_num);
> > -
> > +     handle_repair(repair_bbio, paddrs);
> >  done:
> >       btrfs_repair_done(fbio);
> >       bio_put(&repair_bbio->bio);
>
> Does this leak the fscrypt_extent_info reference acquired at line 326?
> In repair_one_sector(), repair_bbio->fscrypt_info is set via
> fscrypt_get_extent_info(failed_bbio->fscrypt_info), but when
> btrfs_end_repair_bio() completes at the done label, it calls
> bio_put(&repair_bbio->bio) without calling
> fscrypt_put_extent_info(repair_bbio->fscrypt_info) first.

repair_one_sector() is called from btrfs_check_read_bio() which always
has (bbio a.k.a. failed_bbio)->fscrypt_info NULL. So there is no
object which could leak due to refcounting imbalance. Encrypted read
bbio passes through btrfs_check_encrypted_read_bio() instead which
does not yet implement repair.

Moreover btrfs_end_repair_bio() is called before repair_one_sector().
And when it is called btrfs_check_read_bio() returns and no
repair_one_sector() is caled at all.
The fscrypt_put_extent_info() is eventually called from
btrfs_simple_end_io() => simple_end_io_work() => btrfs_bio_end_io().
So this looks clean, unless I missed something.

> > diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h
> > index 456d32db9e9e9..7a8ff4378cba0 100644
> > --- a/fs/btrfs/bio.h
> > +++ b/fs/btrfs/bio.h
>
> [ ... ]
>
> > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> > index 5a39f707c2933..afc461d7642e4 100644
> > --- a/fs/btrfs/compression.c
> > +++ b/fs/btrfs/compression.c
> > @@ -562,6 +562,8 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio)
> >       cb->compress_type = btrfs_extent_map_compression(em);
> >       cb->orig_bbio = bbio;
> >       cb->bbio.csum_search_commit_root = bbio->csum_search_commit_root;
> > +     cb->bbio.fscrypt_info = fscrypt_get_extent_info(em->fscrypt_info);
> > +     cb->bbio.orig_start = 0;
> >
> >       btrfs_set_bio_crypt_ctx_from_extent(&cb->bbio.bio, inode, em->fscrypt_info, 0);
> >       btrfs_free_extent_map(em);
>
> Does this leak the fscrypt_extent_info reference in the compressed read
> path? At line 565, fscrypt_get_extent_info() acquires a reference and
> stores it in cb->bbio.fscrypt_info. Looking at end_bbio_compressed_read()
> in compression.c, it calls btrfs_bio_end_io(cb->orig_bbio, status) which
> releases the ORIGINAL bbio's fscrypt_info, but then calls bio_put(cb_bio)
> without calling fscrypt_put_extent_info(cb->bbio.fscrypt_info). The error
> path in cleanup_compressed_bio() also calls bio_put() without releasing
> the cb->bbio.fscrypt_info reference.

IIUC, the fscrypt_info is put in btrfs_bio_end_io() before calling
end_bbio_compressed_read(). So again, this looks clean to me.

--nX

> [ ... ]
>
>