path: root/0005-BFQ-bugfix.patch

From 2da7edbb8f54b447c0b7dfa029ee4df580a6030e Mon Sep 17 00:00:00 2001
From: Paolo Valente <paolo.valente@linaro.org>
Date: Thu, 16 Mar 2017 14:36:41 +0100
Subject: [PATCH] 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 <paolo.valente@linaro.org>
---
 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);
 }
 
 /*