* [PATCH] blk-throttle: fix null pointer dereference while throttling writeback IOs
[not found] <9fdd7f4b-91b5-7612-bf91-7b18defffb58@gmail.com>
@ 2017-10-10 3:13 ` xuejiufei
2017-10-10 18:13 ` Shaohua Li
0 siblings, 1 reply; 5+ messages in thread
From: xuejiufei @ 2017-10-10 3:13 UTC (permalink / raw)
To: linux-block; +Cc: Shaohua Li, Jens Axboe, wenqing.lz, boyu.mt, jiufei.xjf
From: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
A null pointer dereference can occur when blkcg is removed manually
with writeback IOs inflight. This is caused by the following case:
Writeback kworker submit the bio and set bio->bi_cg_private to tg
in blk_throtl_assoc_bio.
Then we remove the block cgroup manually, the blkg and tg would be
freed if there is no request inflight.
When the submitted bio come back, blk_throtl_bio_endio() fetch the tg
which was already freed.
Fix this by increasing the refcount of blkg in funcion
blk_throtl_assoc_bio() so that the blkg will not be freed until the
bio_endio called.
Signed-off-by: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
---
block/blk-throttle.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 17816a0..d80c3f0 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2112,8 +2112,12 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
static void blk_throtl_assoc_bio(struct throtl_grp *tg, struct bio *bio)
{
#ifdef CONFIG_BLK_DEV_THROTTLING_LOW
- if (bio->bi_css)
+ if (bio->bi_css) {
+ if (bio->bi_cg_private)
+ blkg_put(tg_to_blkg(bio->bi_cg_private));
bio->bi_cg_private = tg;
+ blkg_get(tg_to_blkg(tg));
+ }
blk_stat_set_issue(&bio->bi_issue_stat, bio_sectors(bio));
#endif
}
@@ -2283,8 +2287,10 @@ void blk_throtl_bio_endio(struct bio *bio)
start_time = blk_stat_time(&bio->bi_issue_stat) >> 10;
finish_time = __blk_stat_time(finish_time_ns) >> 10;
- if (!start_time || finish_time <= start_time)
+ if (!start_time || finish_time <= start_time) {
+ blkg_put(tg_to_blkg(tg));
return;
+ }
lat = finish_time - start_time;
/* this is only for bio based driver */
@@ -2314,6 +2320,8 @@ void blk_throtl_bio_endio(struct bio *bio)
tg->bio_cnt /= 2;
tg->bad_bio_cnt /= 2;
}
+
+ blkg_put(tg_to_blkg(tg));
}
#endif
--
1.9.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-throttle: fix null pointer dereference while throttling writeback IOs
2017-10-10 3:13 ` [PATCH] blk-throttle: fix null pointer dereference while throttling writeback IOs xuejiufei
@ 2017-10-10 18:13 ` Shaohua Li
2017-10-10 18:48 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Shaohua Li @ 2017-10-10 18:13 UTC (permalink / raw)
To: xuejiufei
Cc: linux-block, Shaohua Li, Jens Axboe, wenqing.lz, boyu.mt,
jiufei.xjf
On Tue, Oct 10, 2017 at 11:13:32AM +0800, xuejiufei wrote:
> From: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
>
> A null pointer dereference can occur when blkcg is removed manually
> with writeback IOs inflight. This is caused by the following case:
>
> Writeback kworker submit the bio and set bio->bi_cg_private to tg
> in blk_throtl_assoc_bio.
> Then we remove the block cgroup manually, the blkg and tg would be
> freed if there is no request inflight.
> When the submitted bio come back, blk_throtl_bio_endio() fetch the tg
> which was already freed.
>
> Fix this by increasing the refcount of blkg in funcion
> blk_throtl_assoc_bio() so that the blkg will not be freed until the
> bio_endio called.
>
> Signed-off-by: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
> ---
> block/blk-throttle.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 17816a0..d80c3f0 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -2112,8 +2112,12 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
> static void blk_throtl_assoc_bio(struct throtl_grp *tg, struct bio *bio)
> {
> #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
> - if (bio->bi_css)
> + if (bio->bi_css) {
> + if (bio->bi_cg_private)
> + blkg_put(tg_to_blkg(bio->bi_cg_private));
> bio->bi_cg_private = tg;
> + blkg_get(tg_to_blkg(tg));
> + }
> blk_stat_set_issue(&bio->bi_issue_stat, bio_sectors(bio));
> #endif
> }
> @@ -2283,8 +2287,10 @@ void blk_throtl_bio_endio(struct bio *bio)
>
> start_time = blk_stat_time(&bio->bi_issue_stat) >> 10;
> finish_time = __blk_stat_time(finish_time_ns) >> 10;
> - if (!start_time || finish_time <= start_time)
> + if (!start_time || finish_time <= start_time) {
> + blkg_put(tg_to_blkg(tg));
> return;
> + }
>
> lat = finish_time - start_time;
> /* this is only for bio based driver */
> @@ -2314,6 +2320,8 @@ void blk_throtl_bio_endio(struct bio *bio)
> tg->bio_cnt /= 2;
> tg->bad_bio_cnt /= 2;
> }
> +
> + blkg_put(tg_to_blkg(tg));
> }
> #endif
Reviewed-by: Shaohua Li <shli@fb.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-throttle: fix null pointer dereference while throttling writeback IOs
2017-10-10 18:13 ` Shaohua Li
@ 2017-10-10 18:48 ` Jens Axboe
2017-10-10 19:05 ` Shaohua Li
0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2017-10-10 18:48 UTC (permalink / raw)
To: Shaohua Li, xuejiufei
Cc: linux-block, Shaohua Li, wenqing.lz, boyu.mt, jiufei.xjf
On 10/10/2017 12:13 PM, Shaohua Li wrote:
> On Tue, Oct 10, 2017 at 11:13:32AM +0800, xuejiufei wrote:
>> From: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
>>
>> A null pointer dereference can occur when blkcg is removed manually
>> with writeback IOs inflight. This is caused by the following case:
>>
>> Writeback kworker submit the bio and set bio->bi_cg_private to tg
>> in blk_throtl_assoc_bio.
>> Then we remove the block cgroup manually, the blkg and tg would be
>> freed if there is no request inflight.
>> When the submitted bio come back, blk_throtl_bio_endio() fetch the tg
>> which was already freed.
>>
>> Fix this by increasing the refcount of blkg in funcion
>> blk_throtl_assoc_bio() so that the blkg will not be freed until the
>> bio_endio called.
>>
>> Signed-off-by: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
>> ---
>> block/blk-throttle.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 17816a0..d80c3f0 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -2112,8 +2112,12 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
>> static void blk_throtl_assoc_bio(struct throtl_grp *tg, struct bio *bio)
>> {
>> #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
>> - if (bio->bi_css)
>> + if (bio->bi_css) {
>> + if (bio->bi_cg_private)
>> + blkg_put(tg_to_blkg(bio->bi_cg_private));
>> bio->bi_cg_private = tg;
>> + blkg_get(tg_to_blkg(tg));
>> + }
>> blk_stat_set_issue(&bio->bi_issue_stat, bio_sectors(bio));
>> #endif
>> }
>> @@ -2283,8 +2287,10 @@ void blk_throtl_bio_endio(struct bio *bio)
>>
>> start_time = blk_stat_time(&bio->bi_issue_stat) >> 10;
>> finish_time = __blk_stat_time(finish_time_ns) >> 10;
>> - if (!start_time || finish_time <= start_time)
>> + if (!start_time || finish_time <= start_time) {
>> + blkg_put(tg_to_blkg(tg));
>> return;
>> + }
>>
>> lat = finish_time - start_time;
>> /* this is only for bio based driver */
>> @@ -2314,6 +2320,8 @@ void blk_throtl_bio_endio(struct bio *bio)
>> tg->bio_cnt /= 2;
>> tg->bad_bio_cnt /= 2;
>> }
>> +
>> + blkg_put(tg_to_blkg(tg));
>> }
>> #endif
>
> Reviewed-by: Shaohua Li <shli@fb.com>
I was going to queue this up for 4.15, but was wondering if there's a
strong reason to include it for 4.14 instead?
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-throttle: fix null pointer dereference while throttling writeback IOs
2017-10-10 18:48 ` Jens Axboe
@ 2017-10-10 19:05 ` Shaohua Li
2017-10-10 19:09 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Shaohua Li @ 2017-10-10 19:05 UTC (permalink / raw)
To: Jens Axboe
Cc: xuejiufei, linux-block, Shaohua Li, wenqing.lz, boyu.mt,
jiufei.xjf
On Tue, Oct 10, 2017 at 12:48:38PM -0600, Jens Axboe wrote:
> On 10/10/2017 12:13 PM, Shaohua Li wrote:
> > On Tue, Oct 10, 2017 at 11:13:32AM +0800, xuejiufei wrote:
> >> From: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
> >>
> >> A null pointer dereference can occur when blkcg is removed manually
> >> with writeback IOs inflight. This is caused by the following case:
> >>
> >> Writeback kworker submit the bio and set bio->bi_cg_private to tg
> >> in blk_throtl_assoc_bio.
> >> Then we remove the block cgroup manually, the blkg and tg would be
> >> freed if there is no request inflight.
> >> When the submitted bio come back, blk_throtl_bio_endio() fetch the tg
> >> which was already freed.
> >>
> >> Fix this by increasing the refcount of blkg in funcion
> >> blk_throtl_assoc_bio() so that the blkg will not be freed until the
> >> bio_endio called.
> >>
> >> Signed-off-by: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
> >> ---
> >> block/blk-throttle.c | 12 ++++++++++--
> >> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> >> index 17816a0..d80c3f0 100644
> >> --- a/block/blk-throttle.c
> >> +++ b/block/blk-throttle.c
> >> @@ -2112,8 +2112,12 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
> >> static void blk_throtl_assoc_bio(struct throtl_grp *tg, struct bio *bio)
> >> {
> >> #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
> >> - if (bio->bi_css)
> >> + if (bio->bi_css) {
> >> + if (bio->bi_cg_private)
> >> + blkg_put(tg_to_blkg(bio->bi_cg_private));
> >> bio->bi_cg_private = tg;
> >> + blkg_get(tg_to_blkg(tg));
> >> + }
> >> blk_stat_set_issue(&bio->bi_issue_stat, bio_sectors(bio));
> >> #endif
> >> }
> >> @@ -2283,8 +2287,10 @@ void blk_throtl_bio_endio(struct bio *bio)
> >>
> >> start_time = blk_stat_time(&bio->bi_issue_stat) >> 10;
> >> finish_time = __blk_stat_time(finish_time_ns) >> 10;
> >> - if (!start_time || finish_time <= start_time)
> >> + if (!start_time || finish_time <= start_time) {
> >> + blkg_put(tg_to_blkg(tg));
> >> return;
> >> + }
> >>
> >> lat = finish_time - start_time;
> >> /* this is only for bio based driver */
> >> @@ -2314,6 +2320,8 @@ void blk_throtl_bio_endio(struct bio *bio)
> >> tg->bio_cnt /= 2;
> >> tg->bad_bio_cnt /= 2;
> >> }
> >> +
> >> + blkg_put(tg_to_blkg(tg));
> >> }
> >> #endif
> >
> > Reviewed-by: Shaohua Li <shli@fb.com>
>
> I was going to queue this up for 4.15, but was wondering if there's a
> strong reason to include it for 4.14 instead?
Either is ok to me, this isn't easy to trigger, but on the other hand it's
quite safe.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-throttle: fix null pointer dereference while throttling writeback IOs
2017-10-10 19:05 ` Shaohua Li
@ 2017-10-10 19:09 ` Jens Axboe
0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2017-10-10 19:09 UTC (permalink / raw)
To: Shaohua Li
Cc: xuejiufei, linux-block, Shaohua Li, wenqing.lz, boyu.mt,
jiufei.xjf
On 10/10/2017 01:05 PM, Shaohua Li wrote:
> On Tue, Oct 10, 2017 at 12:48:38PM -0600, Jens Axboe wrote:
>> On 10/10/2017 12:13 PM, Shaohua Li wrote:
>>> On Tue, Oct 10, 2017 at 11:13:32AM +0800, xuejiufei wrote:
>>>> From: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
>>>>
>>>> A null pointer dereference can occur when blkcg is removed manually
>>>> with writeback IOs inflight. This is caused by the following case:
>>>>
>>>> Writeback kworker submit the bio and set bio->bi_cg_private to tg
>>>> in blk_throtl_assoc_bio.
>>>> Then we remove the block cgroup manually, the blkg and tg would be
>>>> freed if there is no request inflight.
>>>> When the submitted bio come back, blk_throtl_bio_endio() fetch the tg
>>>> which was already freed.
>>>>
>>>> Fix this by increasing the refcount of blkg in funcion
>>>> blk_throtl_assoc_bio() so that the blkg will not be freed until the
>>>> bio_endio called.
>>>>
>>>> Signed-off-by: Jiufei Xue <jiufei.xjf@alibaba-inc.com>
>>>> ---
>>>> block/blk-throttle.c | 12 ++++++++++--
>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>>>> index 17816a0..d80c3f0 100644
>>>> --- a/block/blk-throttle.c
>>>> +++ b/block/blk-throttle.c
>>>> @@ -2112,8 +2112,12 @@ static inline void throtl_update_latency_buckets(struct throtl_data *td)
>>>> static void blk_throtl_assoc_bio(struct throtl_grp *tg, struct bio *bio)
>>>> {
>>>> #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
>>>> - if (bio->bi_css)
>>>> + if (bio->bi_css) {
>>>> + if (bio->bi_cg_private)
>>>> + blkg_put(tg_to_blkg(bio->bi_cg_private));
>>>> bio->bi_cg_private = tg;
>>>> + blkg_get(tg_to_blkg(tg));
>>>> + }
>>>> blk_stat_set_issue(&bio->bi_issue_stat, bio_sectors(bio));
>>>> #endif
>>>> }
>>>> @@ -2283,8 +2287,10 @@ void blk_throtl_bio_endio(struct bio *bio)
>>>>
>>>> start_time = blk_stat_time(&bio->bi_issue_stat) >> 10;
>>>> finish_time = __blk_stat_time(finish_time_ns) >> 10;
>>>> - if (!start_time || finish_time <= start_time)
>>>> + if (!start_time || finish_time <= start_time) {
>>>> + blkg_put(tg_to_blkg(tg));
>>>> return;
>>>> + }
>>>>
>>>> lat = finish_time - start_time;
>>>> /* this is only for bio based driver */
>>>> @@ -2314,6 +2320,8 @@ void blk_throtl_bio_endio(struct bio *bio)
>>>> tg->bio_cnt /= 2;
>>>> tg->bad_bio_cnt /= 2;
>>>> }
>>>> +
>>>> + blkg_put(tg_to_blkg(tg));
>>>> }
>>>> #endif
>>>
>>> Reviewed-by: Shaohua Li <shli@fb.com>
>>
>> I was going to queue this up for 4.15, but was wondering if there's a
>> strong reason to include it for 4.14 instead?
>
> Either is ok to me, this isn't easy to trigger, but on the other hand it's
> quite safe.
OK, let's just stick with 4.15 then.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-10 19:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <9fdd7f4b-91b5-7612-bf91-7b18defffb58@gmail.com>
2017-10-10 3:13 ` [PATCH] blk-throttle: fix null pointer dereference while throttling writeback IOs xuejiufei
2017-10-10 18:13 ` Shaohua Li
2017-10-10 18:48 ` Jens Axboe
2017-10-10 19:05 ` Shaohua Li
2017-10-10 19:09 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).