Re: [PATCH BUGFIX 3/6] block, bfq: get extra ref to prevent a queue from being freed during a group move

From: Paolo Valente
Date: Fri Jan 31 2020 - 05:41:30 EST


[RESENDING, BECAUSE BOUNCED OFF]

> Il giorno 31 gen 2020, alle ore 11:20, Oleksandr Natalenko <oleksandr@xxxxxxxxxxxxxx> ha scritto:
>
> Hello.
>
> On 31.01.2020 10:24, Paolo Valente wrote:
>> 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 <cevich@xxxxxxxxxx>
>> Signed-off-by: Paolo Valente <paolo.valente@xxxxxxxxxx>
>> ---
>> 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

>> +
>> 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)