Re: [PATCH 3/6] block: don't call bio_uninit from bio_endio

[Johannes Thumshirn <Johannes.Thumshirn@xxxxxxx>]

On 02.07.24 17:11, Christoph Hellwig wrote:
> Commit b222dd2fdd53 ("block: call bio_uninit in bio_endio") added a call
> to bio_uninit in bio_endio to work around callers that use bio_init but
> fail to call bio_uninit after they are done to release the resources.
> While this is an abuse of the bio_init API we still have quite a few of
> those left.  But this early uninit causes a problem for integrity data,
> as at least some users need the bio_integrity_payload.  Right now the
> only one is the NVMe passthrough which archives this by adding a special
> case to skip the freeing if the BIP_INTEGRITY_USER flag is set.
> 
> Sort this out by only putting bi_blkg in bio_endio as that is the cause
> of the actual leaks - the few users of the crypto context and integrity
> data all properly call bio_uninit, usually through bio_put for
> dynamically allocated bios.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>   block/bio.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 4ca3f31ce45fb5..68ce75fd9b7c89 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1630,8 +1630,18 @@ void bio_endio(struct bio *bio)
>   		goto again;
>   	}
>   
> -	/* release cgroup info */
> -	bio_uninit(bio);
> +#ifdef CONFIG_BLK_CGROUP
> +	/*
> +	 * Release cgroup info.  We shouldn't have to do this here, but quite
> +	 * a few callers of bio_init fail to call bio_uninit, so we cover up
> +	 * for that here at least for now.
> +	 */
> +	if (bio->bi_blkg) {
> +		blkg_put(bio->bi_blkg);
> +		bio->bi_blkg = NULL;
> +	}
> +#endif
> +
>   	if (bio->bi_end_io)
>   		bio->bi_end_io(bio);
>   }

Can we have sth. like this on top to avoid the duplication?:

diff --git a/block/bio.c b/block/bio.c
index 68ce75fd9b7c..f10c377b6899 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -210,14 +210,21 @@ struct bio_vec *bvec_alloc(mempool_t *pool, 
unsigned short *nr_vecs,
         return mempool_alloc(pool, gfp_mask);
  }

-void bio_uninit(struct bio *bio)
+static void bio_uninit_blkg(struct bio *bio)
  {
  #ifdef CONFIG_BLK_CGROUP
-       if (bio->bi_blkg) {
-               blkg_put(bio->bi_blkg);
-               bio->bi_blkg = NULL;
-       }
+       if (!bio->bi_blkg)
+               return;
+
+       blkg_put(bio->bi_blkg);
+       bio->bi_blkg = NULL;
  #endif
+}
+
+void bio_uninit(struct bio *bio)
+{
+       bio_uninit_blkg(bio);
+
         if (bio_integrity(bio))
                 bio_integrity_free(bio);

@@ -1630,17 +1637,12 @@ void bio_endio(struct bio *bio)
                 goto again;
         }

-#ifdef CONFIG_BLK_CGROUP
         /*
          * Release cgroup info.  We shouldn't have to do this here, but 
quite
          * a few callers of bio_init fail to call bio_uninit, so we 
cover up
          * for that here at least for now.
          */
-       if (bio->bi_blkg) {
-               blkg_put(bio->bi_blkg);
-               bio->bi_blkg = NULL;
-       }
-#endif
+       bio_uninit_blkg(bio);

         if (bio->bi_end_io)
                 bio->bi_end_io(bio);