[RESENDING, BECAUSE BOUNCED OFF]

Il giorno 31 gen 2020, alle ore 11:20, Oleksandr Natalenko [off-list ref] ha scritto:

Hello.

On 31.01.2020 10:24, Paolo Valente wrote:

quoted

In bfq_bfqq_move(), the bfq_queue, say Q, to be moved to a new group
may happen to be deactivated in the scheduling data structures of the
source group (and then activated in the destination group). If Q is
referred only by the data structures in the source group when the
deactivation happens, then Q is freed upon the deactivation.
This commit addresses this issue by getting an extra reference before
the possible deactivation, and releasing this extra reference after Q
has been moved.
Tested-by: Chris Evich <redacted>
Signed-off-by: Paolo Valente <redacted>
---
block/bfq-cgroup.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index e1419edde2ec..8ab7f18ff8cb 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c

@@ -651,6 +651,12 @@ void bfq_bfqq_move(struct bfq_data *bfqd, struct

bfq_queue *bfqq,
		bfq_bfqq_expire(bfqd, bfqd->in_service_queue,
				false, BFQQE_PREEMPTED);
+	/*
+	 * get extra reference to prevent bfqq from being freed in
+	 * next possible deactivate
+	 */
+	bfqq->ref++;

Shouldn't this be hidden under some macro (bfq_get_queue_ref(), for instance) and also converted from int into refcount_t?

Yeah, that's in my (long) todo list.  Unfortunately, all BFQ code
handles that ref increment in that rustic way (inherited from CFQ).
It would take a little time to fix and check all occurrences.  This
fix is probably more critical, as this bug is crashing machines in some
configurations.  But I promise I won't forget your right suggestion.


Thanks,
Paolo

quoted

+
	if (bfq_bfqq_busy(bfqq))
		bfq_deactivate_bfqq(bfqd, bfqq, false, false);
	else if (entity->on_st)

@@ -670,6 +676,8 @@ void bfq_bfqq_move(struct bfq_data *bfqd, struct

bfq_queue *bfqq,
	if (!bfqd->in_service_queue && !bfqd->rq_in_driver)
		bfq_schedule_dispatch(bfqd);
+	/* release extra ref taken above */
+	bfq_put_queue(bfqq);
}
/**

-- 
 Oleksandr Natalenko (post-factum)