* [PATCH] block, bfq: fix use after free in bfq_bfqq_expire
@ 2019-04-10 8:26 Paolo Valente
2019-04-10 8:34 ` Paolo Valente
0 siblings, 1 reply; 2+ messages in thread
From: Paolo Valente @ 2019-04-10 8:26 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie,
bfq-iosched, oleksandr, Paolo Valente, Dmitrii Tcvetkov,
Douglas Anderson
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] block, bfq: fix use after free in bfq_bfqq_expire
2019-04-10 8:26 [PATCH] block, bfq: fix use after free in bfq_bfqq_expire Paolo Valente
@ 2019-04-10 8:34 ` Paolo Valente
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Valente @ 2019-04-10 8:34 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, kernel list, Ulf Hansson, Linus Walleij, Mark Brown,
'Paolo Valente' via bfq-iosched, Oleksandr Natalenko,
Dmitrii Tcvetkov, Douglas Anderson
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-04-10 8:34 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-10 8:26 [PATCH] block, bfq: fix use after free in bfq_bfqq_expire Paolo Valente
2019-04-10 8:34 ` Paolo Valente
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).