Commit eed47d19 authored by Paolo Valente's avatar Paolo Valente Committed by Jens Axboe
Browse files

block, bfq: fix use after free in bfq_bfqq_expire 



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 9dee8b3b ("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 9dee8b3b
("block, bfq: fix queue removal from weights tree"), 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.

Fixes: 9dee8b3b ("block, bfq: fix queue removal from weights tree")
Reported-by: default avatarDmitrii Tcvetkov <demfloro@demfloro.ru>
Reported-by: default avatarDouglas Anderson <dianders@chromium.org>
Tested-by: default avatarDmitrii Tcvetkov <demfloro@demfloro.ru>
Tested-by: default avatarDouglas Anderson <dianders@chromium.org>
Signed-off-by: default avatarPaolo Valente <paolo.valente@linaro.org>
Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
parent 3ec482d1

block/bfq-iosched.c

+7 −8
Original line number Diff line number Diff line
@@ -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 entities 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;

block/bfq-iosched.h

+1 −1
Original line number Diff line number Diff line
@@ -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);

block/bfq-wf2q.c

+15 −2
Original line number Diff line number Diff line
@@ -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,

CRA Git | Maintained and supported by SUSTech CRA and CCSE