[V4,10/12] block: call __bio_free in bio_endio

Commit Message

Shaohua Li June 28, 2017, 4:30 p.m. UTC

From: Shaohua Li <[e-mail]>

bio_free isn't a good place to free cgroup/integrity info. There are a
lot of cases bio is allocated in special way (for example, in stack) and
never gets called by bio_put hence bio_free, we are leaking memory. This
patch moves the free to bio endio, which should be called anyway. The
__bio_free call in bio_free is kept, in case the bio never gets called
bio endio.

This assumes ->bi_end_io() doesn't access cgroup/integrity info, which
seems true in my audit. Otherwise, we probably must add a flag to
distinguish if bio will be called by bio_put.

Signed-off-by: Shaohua Li <[e-mail]>
---
 block/bio-integrity.c | 1 +
 block/bio.c           | 2 ++
 2 files changed, 3 insertions(+)

Comments

Christoph Hellwig June 28, 2017, 9:29 p.m. UTC | #1

On Wed, Jun 28, 2017 at 09:30:00AM -0700, Shaohua Li wrote:
> From: Shaohua Li <[e-mail]>
> 
> bio_free isn't a good place to free cgroup/integrity info. There are a
> lot of cases bio is allocated in special way (for example, in stack) and
> never gets called by bio_put hence bio_free, we are leaking memory. This
> patch moves the free to bio endio, which should be called anyway. The
> __bio_free call in bio_free is kept, in case the bio never gets called
> bio endio.
> 
> This assumes ->bi_end_io() doesn't access cgroup/integrity info, which
> seems true in my audit. Otherwise, we probably must add a flag to
> distinguish if bio will be called by bio_put.

bio_integrity_endio -> bio_integrity_verify_fn -> bio_integrity_process
access the integrity data, so I don't think this works as-is.

Shaohua Li June 28, 2017, 9:42 p.m. UTC | #2

On Wed, Jun 28, 2017 at 11:29:08PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 28, 2017 at 09:30:00AM -0700, Shaohua Li wrote:
> > From: Shaohua Li <[e-mail]>
> > 
> > bio_free isn't a good place to free cgroup/integrity info. There are a
> > lot of cases bio is allocated in special way (for example, in stack) and
> > never gets called by bio_put hence bio_free, we are leaking memory. This
> > patch moves the free to bio endio, which should be called anyway. The
> > __bio_free call in bio_free is kept, in case the bio never gets called
> > bio endio.
> > 
> > This assumes ->bi_end_io() doesn't access cgroup/integrity info, which
> > seems true in my audit. Otherwise, we probably must add a flag to
> > distinguish if bio will be called by bio_put.
> 
> bio_integrity_endio -> bio_integrity_verify_fn -> bio_integrity_process
> access the integrity data, so I don't think this works as-is.

oh, I probably missed the integrity endio. could we let bio_integrity_verify_fn
free integrity info and and bio_endio free cgroup info?

Patch

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index b8a3a65..e7fa7d0 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -120,6 +120,7 @@  void bio_integrity_free(struct bio *bio)
 	}
 
 	bio->bi_integrity = NULL;
+	bio->bi_opf &= ~REQ_INTEGRITY;
 }
 EXPORT_SYMBOL(bio_integrity_free);
 
diff --git a/block/bio.c b/block/bio.c
index 9cf98b2..4bf3a29 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1828,6 +1828,8 @@  void bio_endio(struct bio *bio)
 	}
 
 	blk_throtl_bio_endio(bio);
+	/* release cgroup/integrity info */
+	__bio_free(bio);
 	if (bio->bi_end_io)
 		bio->bi_end_io(bio);
 }