[PATCH] block: call __bio_free in bio_endio

[PATCH] block: call __bio_free in bio_endio

[Shaohua Li]

bio_free isn't a good place to free cgroup 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 info, which seems true
in my audit.

This along with Christoph's integrity patch should fix the memory leak
issue.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 block/bio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/bio.c b/block/bio.c
index 9cf98b2..e8cda9c 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 info */
+	__bio_free(bio);
 	if (bio->bi_end_io)
 		bio->bi_end_io(bio);
 }
-- 
2.9.3

Re: [PATCH] block: call __bio_free in bio_endio

[Christoph Hellwig]

On Thu, Jun 29, 2017 at 11:54:17AM -0700, Shaohua Li wrote:
> bio_free isn't a good place to free cgroup 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.

Jens already renamed __bio_free to bio_uninit in Linus' tree.

That being said I think we should just kill __bio_free once all
the work is in.  But I think we should defer this post the
initial pull for 4.12-rc to reduce the merge pain.

Re: [PATCH] block: call __bio_free in bio_endio

[Jens Axboe]

On 06/29/2017 02:24 PM, Christoph Hellwig wrote:
> On Thu, Jun 29, 2017 at 11:54:17AM -0700, Shaohua Li wrote:
>> bio_free isn't a good place to free cgroup 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.
> 
> Jens already renamed __bio_free to bio_uninit in Linus' tree.
> 
> That being said I think we should just kill __bio_free once all
> the work is in.  But I think we should defer this post the
> initial pull for 4.12-rc to reduce the merge pain.

Yes, let's pick up that work after the existing for-4.13/block has
been merged upstream.

-- 
Jens Axboe

Re: [PATCH] block: call __bio_free in bio_endio

[Jens Axboe]

On 07/10/2017 12:25 PM, Shaohua Li wrote:
> On Thu, Jun 29, 2017 at 03:23:47PM -0600, Jens Axboe wrote:
>> On 06/29/2017 02:24 PM, Christoph Hellwig wrote:
>>> On Thu, Jun 29, 2017 at 11:54:17AM -0700, Shaohua Li wrote:
>>>> bio_free isn't a good place to free cgroup 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.
>>>
>>> Jens already renamed __bio_free to bio_uninit in Linus' tree.
>>>
>>> That being said I think we should just kill __bio_free once all
>>> the work is in.  But I think we should defer this post the
>>> initial pull for 4.12-rc to reduce the merge pain.
>>
>> Yes, let's pick up that work after the existing for-4.13/block has
>> been merged upstream.
> 
> Jens,
> should I repost the patch or you can take care of it (eg, change __bio_free to
> bio_uninit in the patch)?

Can you repost it, please?

-- 
Jens Axboe