On 4/2/21 4:59 PM, Khazhy Kumykov wrote:
> On Sun, Mar 28, 2021 at 7:00 PM Bart Van Assche <bvanassche@xxxxxxx> wrote:
>> @@ -209,7 +209,12 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>>
>> if (!reserved)
>> bitnr += tags->nr_reserved_tags;
>> - rq = tags->rqs[bitnr];
>> + /*
>> + * See also the percpu_ref_tryget() and blk_queue_exit() calls in
>> + * blk_mq_queue_tag_busy_iter().
>> + */
>> + rq = rcu_dereference_check(tags->rqs[bitnr],
>> + !percpu_ref_is_zero(&hctx->queue->q_usage_counter));
>
> do we need to worry about rq->q != hctx->queue here? i.e., could we
> run into use-after-free on rq->q == hctx->queue check below, since
> rq->q->q_usage_counter might not be raised? Once we verify rq->q ==
> hctx->queue, i agree q_usage_counter is sufficient then
That's a great question. I will change the second
rcu_dereference_check() argument into 'true' and elaborate the comment
above rcu_dereference_check().
>> -static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>> +static bool __bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr,
>> + void *data)
>> {
>> struct bt_tags_iter_data *iter_data = data;
>> struct blk_mq_tags *tags = iter_data->tags;
>> @@ -275,7 +286,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>> if (iter_data->flags & BT_TAG_ITER_STATIC_RQS)
>> rq = tags->static_rqs[bitnr];
>> else
>> - rq = tags->rqs[bitnr];
>> + rq = rcu_dereference_check(tags->rqs[bitnr], true);
>
> lockdep_is_held(&tags->iter_rwsem) ?
I will change the second rcu_dereference_check() argument into the
following:
rcu_read_lock_held() || lockdep_is_held(&tags->iter_rwsem)
>> + /*
>> + * Freeing tags happens with the request queue frozen so the
>> + * srcu dereference below is protected by the request queue
>
> s/srcu/rcu
Thanks, will fix.
Bart.
