| Message ID | 20240702151047.1746127-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] block: split integrity support out of bio.h | expand |
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@lst.de> > --- > 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);
On Wed, Jul 03, 2024 at 09:50:08AM +0000, Johannes Thumshirn wrote:
> Can we have sth. like this on top to avoid the duplication?:
I hope I can just get rid of it over the next merge window or two.
Right now the only two offenders I've found are bcache and bcachefs,
so it might not be that much work.
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); }
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@lst.de> --- block/bio.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)