On Thu, Nov 13, 2025 at 04:22:10PM -0800, Eric Biggers wrote:
> > --- a/block/blk-crypto-fallback.c
> > +++ b/block/blk-crypto-fallback.c
> > @@ -152,35 +152,26 @@ static void blk_crypto_fallback_encrypt_endio(struct
> > bio *enc_bio)
> >
> > src_bio->bi_status = enc_bio->bi_status;
>
> There can now be multiple enc_bios completing for the same src_bio, so
> this needs something like:
>
> if (enc_bio->bi_status)
> cmpxchg(&src_bio->bi_status, 0, enc_bio->bi_status);
Yes.
> > -static struct bio *blk_crypto_fallback_clone_bio(struct bio *bio_src)
> > +static struct bio *blk_crypto_alloc_enc_bio(struct bio *bio_src,
> > + unsigned int nr_segs)
> > {
> > - unsigned int nr_segs = bio_segments(bio_src);
> > - struct bvec_iter iter;
> > - struct bio_vec bv;
> > struct bio *bio;
> >
> > - bio = bio_kmalloc(nr_segs, GFP_NOIO);
> > - if (!bio)
> > - return NULL;
> > - bio_init_inline(bio, bio_src->bi_bdev, nr_segs, bio_src->bi_opf);
> > + bio = bio_alloc_bioset(bio_src->bi_bdev, nr_segs, bio_src->bi_opf,
> > + GFP_NOIO, &crypto_bio_split);
>
> Rename crypto_bio_split => enc_bio_set?
Sure.
> > static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
> > {
>
> I don't think this patch makes sense by itself, since it leaves the
> bio_ptr argument that is used to return a single enc_bio. It does get
> updated later in the series, but it seems that additional change to how
> this function is called should go earlier in the series.
I'll look into it.
>
> > + /* Encrypt each page in the origin bio */
>
> Maybe origin => source, so that consistent terminology is used.
Ok.
>
> > + if (++enc_idx == enc_bio->bi_max_vecs) {
> > + /*
> > + * Each encrypted bio will call bio_endio in the
> > + * completion handler, so ensure the remaining count
> > + * matches the number of submitted bios.
> > + */
> > + bio_inc_remaining(src_bio);
> > + submit_bio(enc_bio);
>
> The above comment is a bit confusing and could be made clearer. When we
> get here for the first time for example, we increment remaining from 1
> to 2. It doesn't match the number of bios submitted so far, but rather
> is one more than it. The extra one pairs with the submit_bio() outside
> the loop. Maybe consider the following:
>
> /*
> * For each additional encrypted bio submitted,
> * increment the source bio's remaining count. Each
> * encrypted bio's completion handler calls bio_endio on
> * the source bio, so this keeps the source bio from
> * completing until the last encrypted bio does.
> */
Yeah. The comment is a leftover from a previous version that worked a
little differently.
> > +out_ioerror:
> > + while (enc_idx > 0)
> > + mempool_free(enc_bio->bi_io_vec[enc_idx--].bv_page,
> > + blk_crypto_bounce_page_pool);
> > + bio_put(enc_bio);
> > + src_bio->bi_status = BLK_STS_IOERR;
>
> This error path doesn't seem correct at all. It would need to free the
> full set of pages in enc_bio, not just the ones initialized so far.
As of this patch the pages are allocated as we go, so I think this is
correct.
> It
> would also need to use cmpxchg() to correctly set an error on the
> src_bio considering that blk_crypto_fallback_encrypt_endio() be trying
> to do it concurrently too, and then call bio_endio() on it.
Yeah.
> (It's annoying that encryption errors need to be handled at all. When I
> eventually convert this to use lib/crypto/, the encryption functions are
> just going to return void. But for now this is using the traditional
> API, which can fail, so technically errors need to be handled...)
I can't wait for the library conversion to happen :)
