From 2da7edbb8f54b447c0b7dfa029ee4df580a6030e Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Thu, 16 Mar 2017 14:36:41 +0100 Subject: [PATCH 1/2] BUGIFX: remove use of bfq queues after free bfq queues occasionally happened to be used after being freed, because they were accessed after some invocations of bfq_put_queue that could cause them to be freed. This commit refactors code, when needed, to avoid any occurrence of such a use-after-free of a bfq queue. This commit also adds comments to make references to bfq queues easier to follow. Signed-off-by: Paolo Valente --- block/bfq-cgroup.c | 1 - block/bfq-iosched.c | 36 ++++++++++++++++++--------- block/bfq-sched.c | 71 ++++++++++++++++++++++++++++++++++++----------------- 3 files changed, 73 insertions(+), 35 deletions(-) diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index de045cf..a66a723 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -771,7 +771,6 @@ static void bfq_pd_offline(struct blkg_policy_data *pd) __bfq_deactivate_entity(entity, false); bfq_put_async_queues(bfqd, bfqg); - BUG_ON(entity->tree); /* * @blkg is going offline and will be ignored by diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 21197f6..f00761b 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -1391,7 +1391,6 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, bfq_bfqq_expire(bfqd, bfqd->in_service_queue, false, BFQ_BFQQ_PREEMPTED); - BUG_ON(in_serv->entity.budget < 0); } } @@ -1560,8 +1559,10 @@ static void bfq_remove_request(struct request *rq) BUG_ON(bfqq->entity.budget < 0); if (bfq_bfqq_busy(bfqq) && bfqq != bfqd->in_service_queue) { + BUG_ON(bfqq->ref < 2); /* referred by rq and on tree */ bfq_del_bfqq_busy(bfqd, bfqq, false); - /* bfqq emptied. In normal operation, when + /* + * bfqq emptied. In normal operation, when * bfqq is empty, bfqq->entity.service and * bfqq->entity.budget must contain, * respectively, the service received and the @@ -1570,7 +1571,8 @@ static void bfq_remove_request(struct request *rq) * this last removal occurred while bfqq is * not in service. To avoid inconsistencies, * reset both bfqq->entity.service and - * bfqq->entity.budget. + * bfqq->entity.budget, if bfqq has still a + * process that may issue I/O requests to it. */ bfqq->entity.budget = bfqq->entity.service = 0; } @@ -2062,7 +2064,8 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic, new_bfqq->wr_coeff = bfqq->wr_coeff; new_bfqq->wr_cur_max_time = bfqq->wr_cur_max_time; new_bfqq->last_wr_start_finish = bfqq->last_wr_start_finish; - new_bfqq->wr_start_at_switch_to_srt = bfqq->wr_start_at_switch_to_srt; + new_bfqq->wr_start_at_switch_to_srt = + bfqq->wr_start_at_switch_to_srt; if (bfq_bfqq_busy(new_bfqq)) bfqd->wr_busy_queues++; new_bfqq->entity.prio_changed = 1; @@ -2105,6 +2108,7 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic, */ new_bfqq->bic = NULL; bfqq->bic = NULL; + /* release process reference to bfqq */ bfq_put_queue(bfqq); } @@ -3077,6 +3081,7 @@ static void bfq_bfqq_expire(struct bfq_data *bfqd, bool slow; unsigned long delta = 0; struct bfq_entity *entity = &bfqq->entity; + int ref; BUG_ON(bfqq != bfqd->in_service_queue); @@ -3184,12 +3189,15 @@ static void bfq_bfqq_expire(struct bfq_data *bfqd, __bfq_bfqq_recalc_budget(bfqd, bfqq, reason); BUG_ON(bfqq->next_rq == NULL && bfqq->entity.budget < bfqq->entity.service); + ref = bfqq->ref; __bfq_bfqq_expire(bfqd, bfqq); - BUG_ON(!bfq_bfqq_busy(bfqq) && reason == BFQ_BFQQ_BUDGET_EXHAUSTED && + BUG_ON(ref > 1 && + !bfq_bfqq_busy(bfqq) && reason == BFQ_BFQQ_BUDGET_EXHAUSTED && !bfq_class_idle(bfqq)); - if (!bfq_bfqq_busy(bfqq) && + /* mark bfqq as waiting a request only if a bic still points to it */ + if (ref > 1 && !bfq_bfqq_busy(bfqq) && reason != BFQ_BFQQ_BUDGET_TIMEOUT && reason != BFQ_BFQQ_BUDGET_EXHAUSTED) bfq_mark_bfqq_non_blocking_wait_rq(bfqq); @@ -3809,7 +3817,8 @@ static int bfq_dispatch_requests(struct request_queue *q, int force) * Task holds one reference to the queue, dropped when task exits. Each rq * in-flight on this queue also holds a reference, dropped when rq is freed. * - * Queue lock must be held here. + * Queue lock must be held here. Recall not to use bfqq after calling + * this function on it. */ static void bfq_put_queue(struct bfq_queue *bfqq) { @@ -3878,7 +3887,7 @@ static void bfq_exit_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq) bfq_put_cooperator(bfqq); - bfq_put_queue(bfqq); + bfq_put_queue(bfqq); /* release process reference */ } static void bfq_init_icq(struct io_cq *icq) @@ -3977,6 +3986,7 @@ static void bfq_check_ioprio_change(struct bfq_io_cq *bic, struct bio *bio) bfqq = bic_to_bfqq(bic, false); if (bfqq) { + /* release process reference on this queue */ bfq_put_queue(bfqq); bfqq = bfq_get_queue(bfqd, bio, BLK_RW_ASYNC, bic); bic_set_bfqq(bic, bfqq, false); @@ -4110,7 +4120,7 @@ static struct bfq_queue *bfq_get_queue(struct bfq_data *bfqd, } out: - bfqq->ref++; + bfqq->ref++; /* get a process reference to this queue */ bfq_log_bfqq(bfqd, bfqq, "get_queue, at end: %p, %d", bfqq, bfqq->ref); rcu_read_unlock(); return bfqq; @@ -4284,10 +4294,14 @@ static void bfq_insert_request(struct request_queue *q, struct request *rq) bfqq->allocated[rq_data_dir(rq)]--; new_bfqq->ref++; bfq_clear_bfqq_just_created(bfqq); - bfq_put_queue(bfqq); if (bic_to_bfqq(RQ_BIC(rq), 1) == bfqq) bfq_merge_bfqqs(bfqd, RQ_BIC(rq), bfqq, new_bfqq); + /* + * rq is about to be enqueued into new_bfqq, + * release rq reference on bfqq + */ + bfq_put_queue(bfqq); rq->elv.priv[1] = new_bfqq; bfqq = new_bfqq; } @@ -4708,7 +4722,7 @@ static void bfq_shutdown_timer_wq(struct bfq_data *bfqd) } static void __bfq_put_async_bfqq(struct bfq_data *bfqd, - struct bfq_queue **bfqq_ptr) + struct bfq_queue **bfqq_ptr) { struct bfq_group *root_group = bfqd->root_group; struct bfq_queue *bfqq = *bfqq_ptr; diff --git a/block/bfq-sched.c b/block/bfq-sched.c index 2e9dc59..70aac56 100644 --- a/block/bfq-sched.c +++ b/block/bfq-sched.c @@ -154,7 +154,13 @@ static bool bfq_update_next_in_service(struct bfq_sched_data *sd, #define for_each_entity(entity) \ for (; entity ; entity = entity->parent) -#define for_each_entity_safe(entity, parent) \ +/* + * For each iteration, compute parent in advance, so as to be safe if + * entity is deallocated during the iteration. Such a deallocation may + * happen as a consequence of a bfq_put_queue that frees the bfq_queue + * containing entity. + */ +#define for_each_entity_safe(entity, parent) \ for (; entity && ({ parent = entity->parent; 1; }); entity = parent) /* @@ -691,27 +697,31 @@ static void bfq_idle_insert(struct bfq_service_tree *st, } /** - * bfq_forget_entity - remove an entity from the wfq trees. + * bfq_forget_entity - do not consider entity any longer for scheduling * @st: the service tree. * @entity: the entity being removed. + * @is_in_service: true if entity is currently the in-service entity. * - * Update the device status and forget everything about @entity, putting - * the device reference to it, if it is a queue. Entities belonging to - * groups are not refcounted. + * Forget everything about @entity. In addition, if entity represents + * a queue, and the latter is not in service, then release the service + * reference to the queue (the one taken through bfq_get_entity). In + * fact, in this case, there is really no more service reference to + * the queue, as the latter is also outside any service tree. If, + * instead, the queue is in service, then __bfq_bfqd_reset_in_service + * will take care of putting the reference when the queue finally + * stops being served. */ static void bfq_forget_entity(struct bfq_service_tree *st, - struct bfq_entity *entity) + struct bfq_entity *entity, + bool is_in_service) { struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity); - struct bfq_sched_data *sd; - BUG_ON(!entity->on_st); entity->on_st = false; st->wsum -= entity->weight; - if (bfqq) { - sd = entity->sched_data; - bfq_log_bfqq(bfqq->bfqd, bfqq, "forget_entity: %p %d", + if (bfqq && !is_in_service) { + bfq_log_bfqq(bfqq->bfqd, bfqq, "forget_entity (before): %p %d", bfqq, bfqq->ref); bfq_put_queue(bfqq); } @@ -726,7 +736,8 @@ static void bfq_put_idle_entity(struct bfq_service_tree *st, struct bfq_entity *entity) { bfq_idle_extract(st, entity); - bfq_forget_entity(st, entity); + bfq_forget_entity(st, entity, + entity == entity->sched_data->in_service_entity); } /** @@ -1082,6 +1093,12 @@ static void __bfq_activate_entity(struct bfq_entity *entity, */ entity->start = min_vstart; st->wsum += entity->weight; + /* + * entity is about to be inserted into a service tree, + * and then set in service: get a reference to make + * sure entity does not disappear until it is no + * longer in service or scheduled for service. + */ bfq_get_entity(entity); BUG_ON(entity->on_st && bfqq); @@ -1264,27 +1281,27 @@ static bool __bfq_deactivate_entity(struct bfq_entity *entity, { struct bfq_sched_data *sd = entity->sched_data; struct bfq_service_tree *st = bfq_entity_service_tree(entity); - bool was_in_service = entity == sd->in_service_entity; + bool is_in_service = entity == sd->in_service_entity; if (!entity->on_st) { /* entity never activated, or already inactive */ BUG_ON(entity == entity->sched_data->in_service_entity); return false; } - BUG_ON(was_in_service && entity->tree && entity->tree != &st->active); + BUG_ON(is_in_service && entity->tree && entity->tree != &st->active); - if (was_in_service) + if (is_in_service) bfq_calc_finish(entity, entity->service); if (entity->tree == &st->active) bfq_active_extract(st, entity); - else if (!was_in_service && entity->tree == &st->idle) + else if (!is_in_service && entity->tree == &st->idle) bfq_idle_extract(st, entity); else if (entity->tree) BUG(); if (!ins_into_idle_tree || !bfq_gt(entity->finish, st->vtime)) - bfq_forget_entity(st, entity); + bfq_forget_entity(st, entity, is_in_service); else bfq_idle_insert(st, entity); @@ -1320,8 +1337,8 @@ static void bfq_deactivate_entity(struct bfq_entity *entity, if (!__bfq_deactivate_entity(entity, ins_into_idle_tree)) { /* - * Entity is not any tree any more, so, this - * deactivation is a no-op, and there is + * entity is not in any tree any more, so + * this deactivation is a no-op, and there is * nothing to change for upper-level entities * (in case of expiration, this can never * happen). @@ -1821,14 +1838,16 @@ static struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd) static void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) { - struct bfq_entity *entity = &bfqd->in_service_queue->entity; + struct bfq_queue *in_serv_bfqq = bfqd->in_service_queue; + struct bfq_entity *in_serv_entity = &in_serv_bfqq->entity; + struct bfq_entity *entity = in_serv_entity; if (bfqd->in_service_bic) { put_io_context(bfqd->in_service_bic->icq.ioc); bfqd->in_service_bic = NULL; } - bfq_clear_bfqq_wait_request(bfqd->in_service_queue); + bfq_clear_bfqq_wait_request(in_serv_bfqq); hrtimer_try_to_cancel(&bfqd->idle_slice_timer); bfqd->in_service_queue = NULL; @@ -1840,6 +1859,14 @@ static void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) */ for_each_entity(entity) entity->sched_data->in_service_entity = NULL; + + /* + * in_serv_entity is no longer in service, so, if it is in no + * service tree either, then release the service reference to + * the queue it represents (taken with bfq_get_entity). + */ + if (!in_serv_entity->on_st) + bfq_put_queue(in_serv_bfqq); } static void bfq_deactivate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq, @@ -1904,8 +1931,6 @@ static void bfq_del_bfqq_busy(struct bfq_data *bfqd, struct bfq_queue *bfqq, BUG_ON(bfqq->entity.budget < 0); bfq_deactivate_bfqq(bfqd, bfqq, true, expiration); - - BUG_ON(bfqq->entity.budget < 0); } /* From 8d4b2ded945bdb75fc5c197d95089ebfe5fa24e7 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Tue, 21 Mar 2017 09:56:39 -0400 Subject: [PATCH 2/2] BFQ-v8r9 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index f00761b..1e46d68 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5252,7 +5252,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r8"; + char msg[60] = "BFQ I/O-scheduler: v8r9"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index f433cfe..6faed4f 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r8 for 4.10.0: data structures and common functions prototypes. + * BFQ v8r9 for 4.10.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe