diff mbox series

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

Message ID 20200131092409.10867-4-paolo.valente@linaro.org (mailing list archive)
State New, archived
Headers show
Series block, bfq: series of fixes, and not only, for some recently reported issues | expand

Commit Message

Paolo Valente Jan. 31, 2020, 9:24 a.m. UTC 
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@redhat.com>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-cgroup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Oleksandr Natalenko Jan. 31, 2020, 10:20 a.m. UTC | #1 
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@redhat.com>
> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
> ---
>  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?

> +
>  	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);
>  }
> 
>  /**
Paolo Valente Jan. 31, 2020, 10:41 a.m. UTC | #2 
[RESENDING, BECAUSE BOUNCED OFF]

> Il giorno 31 gen 2020, alle ore 11:20, Oleksandr Natalenko <oleksandr@natalenko.name> 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@redhat.com>
>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>> ---
>> 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)
diff mbox series

Patch

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++;
+
 	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);
 }
 
 /**