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

[Chris Mason <clm@meta.com>]

Daniel Vacek <neelx@suse.com> wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> 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@toxicpanda.com>

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.

> 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.

[ ... ]