* [PATCH] block: fix memleak of bio integrity data
@ 2019-12-05 2:09 Ming Lei
[not found] ` <CAAmqgVN6huL60c9aw41yC6wz6fG0w-T4xR0Tuoz0PqX2BqwKDA@mail.gmail.com>
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Ming Lei @ 2019-12-05 2:09 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Justin Tee, Christoph Hellwig, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix memleak of bio integrity data
[not found] ` <CAAmqgVN6huL60c9aw41yC6wz6fG0w-T4xR0Tuoz0PqX2BqwKDA@mail.gmail.com>
@ 2019-12-05 7:49 ` Ming Lei
2019-12-05 8:09 ` Ming Lei
0 siblings, 1 reply; 7+ messages in thread
From: Ming Lei @ 2019-12-05 7:49 UTC (permalink / raw)
To: Justin Tee; +Cc: Jens Axboe, linux-block, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix memleak of bio integrity data
2019-12-05 7:49 ` Ming Lei
@ 2019-12-05 8:09 ` Ming Lei
2019-12-05 18:30 ` Justin Tee
0 siblings, 1 reply; 7+ messages in thread
From: Ming Lei @ 2019-12-05 8:09 UTC (permalink / raw)
To: Justin Tee; +Cc: Jens Axboe, linux-block, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix memleak of bio integrity data
2019-12-05 2:09 [PATCH] block: fix memleak of bio integrity data Ming Lei
[not found] ` <CAAmqgVN6huL60c9aw41yC6wz6fG0w-T4xR0Tuoz0PqX2BqwKDA@mail.gmail.com>
@ 2019-12-05 9:52 ` Christoph Hellwig
2019-12-05 10:08 ` John Garry
2 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2019-12-05 9:52 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Justin Tee, Christoph Hellwig
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix memleak of bio integrity data
2019-12-05 2:09 [PATCH] block: fix memleak of bio integrity data Ming Lei
[not found] ` <CAAmqgVN6huL60c9aw41yC6wz6fG0w-T4xR0Tuoz0PqX2BqwKDA@mail.gmail.com>
2019-12-05 9:52 ` Christoph Hellwig
@ 2019-12-05 10:08 ` John Garry
2 siblings, 0 replies; 7+ messages in thread
From: John Garry @ 2019-12-05 10:08 UTC (permalink / raw)
To: Ming Lei, Jens Axboe; +Cc: linux-block, Justin Tee, Christoph Hellwig
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 +-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix memleak of bio integrity data
2019-12-05 8:09 ` Ming Lei
@ 2019-12-05 18:30 ` Justin Tee
2019-12-05 18:38 ` Jens Axboe
0 siblings, 1 reply; 7+ messages in thread
From: Justin Tee @ 2019-12-05 18:30 UTC (permalink / raw)
To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix memleak of bio integrity data
2019-12-05 18:30 ` Justin Tee
@ 2019-12-05 18:38 ` Jens Axboe
0 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2019-12-05 18:38 UTC (permalink / raw)
To: Justin Tee, Ming Lei; +Cc: linux-block, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-05 18:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-05 2:09 [PATCH] block: fix memleak of bio integrity data Ming Lei
[not found] ` <CAAmqgVN6huL60c9aw41yC6wz6fG0w-T4xR0Tuoz0PqX2BqwKDA@mail.gmail.com>
2019-12-05 7:49 ` Ming Lei
2019-12-05 8:09 ` Ming Lei
2019-12-05 18:30 ` Justin Tee
2019-12-05 18:38 ` Jens Axboe
2019-12-05 9:52 ` Christoph Hellwig
2019-12-05 10:08 ` John Garry
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.