| Message ID | 20190410082646.14221-1-paolo.valente@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block, bfq: fix use after free in bfq_bfqq_expire | expand |
This patch causes some checkpatch complain, sorry. Sending a V2 right away. Paolo > Il giorno 10 apr 2019, alle ore 10:26, Paolo Valente <paolo.valente@linaro.org> ha scritto: > > The function bfq_bfqq_expire() invokes the function > __bfq_bfqq_expire(), and the latter may free the in-service bfq-queue. > If this happens, then no other instruction of bfq_bfqq_expire() must > be executed, or a use-after-free will occur. > > Basing on the assumption that __bfq_bfqq_expire() invokes > bfq_put_queue() on the in-service bfq-queue exactly once, the queue is > assumed to be freed if its refcounter is equal to one right before > invoking __bfq_bfqq_expire(). > > But, since commit 9dee8b3b057e1 ("block, bfq: fix queue removal from > weights tree") this assumption is false. __bfq_bfqq_expire() may also > invoke bfq_weights_tree_remove() and, since commit 9dee8b3b057e1, also > the latter function may invoke bfq_put_queue(). So __bfq_bfqq_expire() > may invoke bfq_put_queue() twice, and this is the actual case where > the in-service queue may happen to be freed. > > To address this issue, this commit moves the check on the refcounter > of the queue right around the last bfq_put_queue() that may be invoked > on the queue. > > Reported-by: Dmitrii Tcvetkov <demfloro@demfloro.ru> > Reported-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Dmitrii Tcvetkov <demfloro@demfloro.ru> > Tested-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > --- > block/bfq-iosched.c | 15 +++++++-------- > block/bfq-iosched.h | 2 +- > block/bfq-wf2q.c | 17 +++++++++++++++-- > 3 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index fac188dd78fa..30b88ec7ad26 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -2822,7 +2822,7 @@ static void bfq_dispatch_remove(struct request_queue *q, struct request *rq) > bfq_remove_request(q, rq); > } > > -static void __bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq) > +static bool __bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq) > { > /* > * If this bfqq is shared between multiple processes, check > @@ -2855,9 +2855,11 @@ static void __bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq) > /* > * All in-service entities must have been properly deactivated > * or requeued before executing the next function, which > - * resets all in-service entites as no more in service. > + * resets all in-service entites as no more in service. This > + * may cause bfqq to be freed. If this happens, the next > + * function returns true. > */ > - __bfq_bfqd_reset_in_service(bfqd); > + return __bfq_bfqd_reset_in_service(bfqd); > } > > /** > @@ -3262,7 +3264,6 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, > bool slow; > unsigned long delta = 0; > struct bfq_entity *entity = &bfqq->entity; > - int ref; > > /* > * Check whether the process is slow (see bfq_bfqq_is_slow). > @@ -3347,10 +3348,8 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, > * reason. > */ > __bfq_bfqq_recalc_budget(bfqd, bfqq, reason); > - ref = bfqq->ref; > - __bfq_bfqq_expire(bfqd, bfqq); > - > - if (ref == 1) /* bfqq is gone, no more actions on it */ > + if (__bfq_bfqq_expire(bfqd, bfqq)) > + /* bfqq is gone, no more actions on it */ > return; > > bfqq->injected_service = 0; > diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h > index 062e1c4787f4..86394e503ca9 100644 > --- a/block/bfq-iosched.h > +++ b/block/bfq-iosched.h > @@ -995,7 +995,7 @@ bool __bfq_deactivate_entity(struct bfq_entity *entity, > bool ins_into_idle_tree); > bool next_queue_may_preempt(struct bfq_data *bfqd); > struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd); > -void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd); > +bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd); > void bfq_deactivate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq, > bool ins_into_idle_tree, bool expiration); > void bfq_activate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq); > diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c > index a11bef75483d..ae4d000ac0af 100644 > --- a/block/bfq-wf2q.c > +++ b/block/bfq-wf2q.c > @@ -1605,7 +1605,8 @@ struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd) > return bfqq; > } > > -void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) > +/* returns true if the in-service queue gets freed */ > +bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) > { > struct bfq_queue *in_serv_bfqq = bfqd->in_service_queue; > struct bfq_entity *in_serv_entity = &in_serv_bfqq->entity; > @@ -1629,8 +1630,20 @@ void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) > * service tree either, then release the service reference to > * the queue it represents (taken with bfq_get_entity). > */ > - if (!in_serv_entity->on_st) > + if (!in_serv_entity->on_st) { > + /* > + * If no process is referencing in_serv_bfqq any > + * longer, then the service reference may be the only > + * reference to the queue. If this is the case, then > + * bfqq gets freed here. > + */ > + int ref = in_serv_bfqq->ref; > bfq_put_queue(in_serv_bfqq); > + if (ref == 1) > + return true; > + } > + > + return false; > } > > void bfq_deactivate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq, > -- > 2.20.1 >
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index fac188dd78fa..30b88ec7ad26 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -2822,7 +2822,7 @@ static void bfq_dispatch_remove(struct request_queue *q, struct request *rq) bfq_remove_request(q, rq); } -static void __bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq) +static bool __bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq) { /* * If this bfqq is shared between multiple processes, check @@ -2855,9 +2855,11 @@ static void __bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq) /* * All in-service entities must have been properly deactivated * or requeued before executing the next function, which - * resets all in-service entites as no more in service. + * resets all in-service entites as no more in service. This + * may cause bfqq to be freed. If this happens, the next + * function returns true. */ - __bfq_bfqd_reset_in_service(bfqd); + return __bfq_bfqd_reset_in_service(bfqd); } /** @@ -3262,7 +3264,6 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, bool slow; unsigned long delta = 0; struct bfq_entity *entity = &bfqq->entity; - int ref; /* * Check whether the process is slow (see bfq_bfqq_is_slow). @@ -3347,10 +3348,8 @@ void bfq_bfqq_expire(struct bfq_data *bfqd, * reason. */ __bfq_bfqq_recalc_budget(bfqd, bfqq, reason); - ref = bfqq->ref; - __bfq_bfqq_expire(bfqd, bfqq); - - if (ref == 1) /* bfqq is gone, no more actions on it */ + if (__bfq_bfqq_expire(bfqd, bfqq)) + /* bfqq is gone, no more actions on it */ return; bfqq->injected_service = 0; diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h index 062e1c4787f4..86394e503ca9 100644 --- a/block/bfq-iosched.h +++ b/block/bfq-iosched.h @@ -995,7 +995,7 @@ bool __bfq_deactivate_entity(struct bfq_entity *entity, bool ins_into_idle_tree); bool next_queue_may_preempt(struct bfq_data *bfqd); struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd); -void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd); +bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd); void bfq_deactivate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq, bool ins_into_idle_tree, bool expiration); void bfq_activate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq); diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c index a11bef75483d..ae4d000ac0af 100644 --- a/block/bfq-wf2q.c +++ b/block/bfq-wf2q.c @@ -1605,7 +1605,8 @@ struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd) return bfqq; } -void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) +/* returns true if the in-service queue gets freed */ +bool __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) { struct bfq_queue *in_serv_bfqq = bfqd->in_service_queue; struct bfq_entity *in_serv_entity = &in_serv_bfqq->entity; @@ -1629,8 +1630,20 @@ void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) * service tree either, then release the service reference to * the queue it represents (taken with bfq_get_entity). */ - if (!in_serv_entity->on_st) + if (!in_serv_entity->on_st) { + /* + * If no process is referencing in_serv_bfqq any + * longer, then the service reference may be the only + * reference to the queue. If this is the case, then + * bfqq gets freed here. + */ + int ref = in_serv_bfqq->ref; bfq_put_queue(in_serv_bfqq); + if (ref == 1) + return true; + } + + return false; } void bfq_deactivate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq,
The function bfq_bfqq_expire() invokes the function __bfq_bfqq_expire(), and the latter may free the in-service bfq-queue. If this happens, then no other instruction of bfq_bfqq_expire() must be executed, or a use-after-free will occur. Basing on the assumption that __bfq_bfqq_expire() invokes bfq_put_queue() on the in-service bfq-queue exactly once, the queue is assumed to be freed if its refcounter is equal to one right before invoking __bfq_bfqq_expire(). But, since commit 9dee8b3b057e1 ("block, bfq: fix queue removal from weights tree") this assumption is false. __bfq_bfqq_expire() may also invoke bfq_weights_tree_remove() and, since commit 9dee8b3b057e1, also the latter function may invoke bfq_put_queue(). So __bfq_bfqq_expire() may invoke bfq_put_queue() twice, and this is the actual case where the in-service queue may happen to be freed. To address this issue, this commit moves the check on the refcounter of the queue right around the last bfq_put_queue() that may be invoked on the queue. Reported-by: Dmitrii Tcvetkov <demfloro@demfloro.ru> Reported-by: Douglas Anderson <dianders@chromium.org> Tested-by: Dmitrii Tcvetkov <demfloro@demfloro.ru> Tested-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bfq-iosched.c | 15 +++++++-------- block/bfq-iosched.h | 2 +- block/bfq-wf2q.c | 17 +++++++++++++++-- 3 files changed, 23 insertions(+), 11 deletions(-) -- 2.20.1