[PATCH] block: fix memleak of bio integrity data

[PATCH] block: fix memleak of bio integrity data

[Ming Lei]

From: Justin Tee <justin.tee@broadcom.com>

7c20f11680a4 ("bio-integrity: stop abusing bi_end_io") moves
bio_integrity_free from bio_uninit() to bio_integrity_verify_fn()
and bio_endio(). This way looks wrong because bio may be freed
without calling bio_endio(), for example, blk_rq_unprep_clone() is
called from dm_mq_queue_rq() when the underlying queue of dm-mpath
is busy.

So memory leak of bio integrity data is caused by commit 7c20f11680a4.

Fixes this issue by re-adding bio_integrity_free() to bio_uninit().

Fixes: 7c20f11680a4 ("bio-integrity: stop abusing bi_end_io")
Cc: Justin Tee <justin.tee@broadcom.com>
Cc: Christoph Hellwig <hch@lst.de>

Add commit log, and simplify/fix the original patch wroten by Justin.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio-integrity.c | 2 +-
 block/bio.c           | 3 +++
 block/blk.h           | 4 ++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index fb95dbb21dd8..bf62c25cde8f 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -87,7 +87,7 @@ EXPORT_SYMBOL(bio_integrity_alloc);
  * Description: Used to free the integrity portion of a bio. Usually
  * called from bio_free().
  */
-static void bio_integrity_free(struct bio *bio)
+void bio_integrity_free(struct bio *bio)
 {
 	struct bio_integrity_payload *bip = bio_integrity(bio);
 	struct bio_set *bs = bio->bi_pool;
diff --git a/block/bio.c b/block/bio.c
index b1170ec18464..9d54aa37ce6c 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -233,6 +233,9 @@ struct bio_vec *bvec_alloc(gfp_t gfp_mask, int nr, unsigned long *idx,
 void bio_uninit(struct bio *bio)
 {
 	bio_disassociate_blkg(bio);
+
+	if (bio_integrity(bio))
+		bio_integrity_free(bio);
 }
 EXPORT_SYMBOL(bio_uninit);
 
diff --git a/block/blk.h b/block/blk.h
index 2bea40180b6f..6842f28c033e 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -121,6 +121,7 @@ static inline void blk_rq_bio_prep(struct request *rq, struct bio *bio,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 void blk_flush_integrity(void);
 bool __bio_integrity_endio(struct bio *);
+void bio_integrity_free(struct bio *bio);
 static inline bool bio_integrity_endio(struct bio *bio)
 {
 	if (bio_integrity(bio))
@@ -166,6 +167,9 @@ static inline bool bio_integrity_endio(struct bio *bio)
 {
 	return true;
 }
+static inline void bio_integrity_free(struct bio *bio)
+{
+}
 #endif /* CONFIG_BLK_DEV_INTEGRITY */
 
 unsigned long blk_rq_timeout(unsigned long timeout);
-- 
2.20.1

Re: [PATCH] block: fix memleak of bio integrity data

[Ming Lei]

On Wed, Dec 04, 2019 at 09:41:24PM -0800, Justin Tee wrote:
> Hi Ming,
> 
> I understand the patch, but I have a concern.
> 
> Is it possible to come across a double-free?  from bio_endio ->
> bio_integrity_endio -> __bio_integrity_endio -> bio_integrity_free;  And
> then, resuming in bio_endio -> bio_uninit -> bio_integrity_free;.  Maybe
> it's even possible queue_work  bio_integrity_verify_fn was scheduled and
> called bio_integrity_free from there as well.  So, should also remove
> bio_integrity_free from bio_integrity_verify_fn and __bio_integrity_endio
> routines?

Yeah, double-free could be caused for READ between bio_integrity_verify_fn()
and bio_uninit().

I think it is enough to remove bio_integrity_free() from both
bio_integrity_verify_fn() and __bio_integrity_endio().

Will do it in V2.


Thanks, 
Ming

Re: [PATCH] block: fix memleak of bio integrity data

[Ming Lei]

On Thu, Dec 05, 2019 at 03:49:32PM +0800, Ming Lei wrote:
> On Wed, Dec 04, 2019 at 09:41:24PM -0800, Justin Tee wrote:
> > Hi Ming,
> > 
> > I understand the patch, but I have a concern.
> > 
> > Is it possible to come across a double-free?  from bio_endio ->
> > bio_integrity_endio -> __bio_integrity_endio -> bio_integrity_free;  And
> > then, resuming in bio_endio -> bio_uninit -> bio_integrity_free;.  Maybe
> > it's even possible queue_work  bio_integrity_verify_fn was scheduled and
> > called bio_integrity_free from there as well.  So, should also remove
> > bio_integrity_free from bio_integrity_verify_fn and __bio_integrity_endio
> > routines?
> 
> Yeah, double-free could be caused for READ between bio_integrity_verify_fn()
> and bio_uninit().

ooops, the above race doesn't exist because __bio_integrity_endio()
returns false and bio_endio() won't call bio_uninit(). And bio_uninit()
is only called from bio_endio() when bio_integrity_verify_fn() exits.

Also we can't remove the bio_integrity_free() from bio_integrity_verify_fn(),
otherwise this bio may never be ended because bio_integrity_endio() will
schedule the verify_fn again if bio_integrity_verify_fn() won't clear
REQ_INTEGRITY.

So bio_integrity_free() is always called serially, and the flag of REQ_INTEGRITY 
guarantees that it is only freed once.

I think there isn't such double free you mentioned.

Thanks,
Ming

Re: [PATCH] block: fix memleak of bio integrity data

[Justin Tee]

On 12/5/2019 12:09 AM, Ming Lei wrote:
> On Thu, Dec 05, 2019 at 03:49:32PM +0800, Ming Lei wrote:
>> On Wed, Dec 04, 2019 at 09:41:24PM -0800, Justin Tee wrote:
>>> Hi Ming,
>>>
>>> I understand the patch, but I have a concern.
>>>
>>> Is it possible to come across a double-free?  from bio_endio ->
>>> bio_integrity_endio -> __bio_integrity_endio -> bio_integrity_free;  And
>>> then, resuming in bio_endio -> bio_uninit -> bio_integrity_free;.  Maybe
>>> it's even possible queue_work  bio_integrity_verify_fn was scheduled and
>>> called bio_integrity_free from there as well.  So, should also remove
>>> bio_integrity_free from bio_integrity_verify_fn and __bio_integrity_endio
>>> routines?
>>
>> Yeah, double-free could be caused for READ between bio_integrity_verify_fn()
>> and bio_uninit().
> 
> ooops, the above race doesn't exist because __bio_integrity_endio()
> returns false and bio_endio() won't call bio_uninit(). And bio_uninit()
> is only called from bio_endio() when bio_integrity_verify_fn() exits.
> 
> Also we can't remove the bio_integrity_free() from bio_integrity_verify_fn(),
> otherwise this bio may never be ended because bio_integrity_endio() will
> schedule the verify_fn again if bio_integrity_verify_fn() won't clear
> REQ_INTEGRITY.
> 
> So bio_integrity_free() is always called serially, and the flag of REQ_INTEGRITY
> guarantees that it is only freed once.
> 
> I think there isn't such double free you mentioned.
> 
> Thanks,
> Ming
> 
> 
> 

Right agreed, the REQ_INTEGRITY flag is what is guaranteeing freeing 
only once.

Thanks for clearing this up.  I'm good with the patch.

Thanks,
Justin

Re: [PATCH] block: fix memleak of bio integrity data

[Jens Axboe]

On 12/5/19 11:30 AM, Justin Tee wrote:
> On 12/5/2019 12:09 AM, Ming Lei wrote:
>> On Thu, Dec 05, 2019 at 03:49:32PM +0800, Ming Lei wrote:
>>> On Wed, Dec 04, 2019 at 09:41:24PM -0800, Justin Tee wrote:
>>>> Hi Ming,
>>>>
>>>> I understand the patch, but I have a concern.
>>>>
>>>> Is it possible to come across a double-free?  from bio_endio ->
>>>> bio_integrity_endio -> __bio_integrity_endio -> bio_integrity_free;  And
>>>> then, resuming in bio_endio -> bio_uninit -> bio_integrity_free;.  Maybe
>>>> it's even possible queue_work  bio_integrity_verify_fn was scheduled and
>>>> called bio_integrity_free from there as well.  So, should also remove
>>>> bio_integrity_free from bio_integrity_verify_fn and __bio_integrity_endio
>>>> routines?
>>>
>>> Yeah, double-free could be caused for READ between bio_integrity_verify_fn()
>>> and bio_uninit().
>>
>> ooops, the above race doesn't exist because __bio_integrity_endio()
>> returns false and bio_endio() won't call bio_uninit(). And bio_uninit()
>> is only called from bio_endio() when bio_integrity_verify_fn() exits.
>>
>> Also we can't remove the bio_integrity_free() from bio_integrity_verify_fn(),
>> otherwise this bio may never be ended because bio_integrity_endio() will
>> schedule the verify_fn again if bio_integrity_verify_fn() won't clear
>> REQ_INTEGRITY.
>>
>> So bio_integrity_free() is always called serially, and the flag of REQ_INTEGRITY
>> guarantees that it is only freed once.
>>
>> I think there isn't such double free you mentioned.
>>
>> Thanks,
>> Ming
>>
>>
>>
> 
> Right agreed, the REQ_INTEGRITY flag is what is guaranteeing freeing 
> only once.
> 
> Thanks for clearing this up.  I'm good with the patch.

Thanks all - I have applied the patch, and re-instated Justin's
signed-off-by.

-- 
Jens Axboe

Re: [PATCH] block: fix memleak of bio integrity data

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] block: fix memleak of bio integrity data

[John Garry]

On 05/12/2019 02:09, Ming Lei wrote:
> From: Justin Tee <justin.tee@broadcom.com>
> 
> 7c20f11680a4 ("bio-integrity: stop abusing bi_end_io") moves
> bio_integrity_free from bio_uninit() to bio_integrity_verify_fn()
> and bio_endio(). This way looks wrong because bio may be freed
> without calling bio_endio(), for example, blk_rq_unprep_clone() is
> called from dm_mq_queue_rq() when the underlying queue of dm-mpath
> is busy.
> 
> So memory leak of bio integrity data is caused by commit 7c20f11680a4.
> 
> Fixes this issue by re-adding bio_integrity_free() to bio_uninit().
> 
> Fixes: 7c20f11680a4 ("bio-integrity: stop abusing bi_end_io")
> Cc: Justin Tee <justin.tee@broadcom.com>
> Cc: Christoph Hellwig <hch@lst.de>
> 
> Add commit log, and simplify/fix the original patch wroten by Justin.

written

> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Don't forget the author SoB...

> ---
>   block/bio-integrity.c | 2 +-