linux-kernel - Re: [RFC PATCH v2 2/2] blk-mq: Lockout tagset iter when freeing rqs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7bdd562d-b258-43a2-0de0-966091086cff@huawei.com>
Date:   Wed, 23 Dec 2020 11:10:33 +0000
From:   John Garry <john.garry@...wei.com>
To:     Bart Van Assche <bvanassche@....org>, <axboe@...nel.dk>,
        <ming.lei@...hat.com>
CC:     <linux-block@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <hch@....de>, <hare@...e.de>, <ppvk@...eaurora.org>,
        <kashyap.desai@...adcom.com>, <linuxarm@...wei.com>
Subject: Re: [RFC PATCH v2 2/2] blk-mq: Lockout tagset iter when freeing rqs

On 22/12/2020 16:16, Bart Van Assche wrote:
> On 12/22/20 3:15 AM, John Garry wrote:
>> So then we could have something like this:
>>
>> ---8<---
>>
>>   -435,9 +444,13 @@ void blk_mq_queue_tag_busy_iter(struct 
>> request_queue *q, busy_iter_fn *fn,
>>      if (!blk_mq_hw_queue_mapped(hctx))
>>              continue;
>>
>> +    while (!atomic_inc_not_zero(&tags->iter_usage_counter));
>> +
>>      if (tags->nr_reserved_tags)
>>          bt_for_each(hctx, tags->breserved_tags, fn, priv, true);
>>      bt_for_each(hctx, tags->bitmap_tags, fn, priv, false);
>>
>> +    atomic_dec(&tags->iter_usage_counter);
>> }
>>
>> blk_queue_exit(q);
>>
>> --->8---
>>
>> And similar for blk_mq_tagset_busy_iter(). How about it?
> 

Hi Bart,

> Are there any blk_mq_tagset_busy_iter() calls that happen from a context 
> where the tag set can disappear while that function is in progress?
> 

So isn't the blk_mq_tag_set always a member of the host driver data for 
those cases, and, since blk_mq_tagset_busy_iter() is for iter'ing block 
driver tags and called from block driver or hctx_busy_show(), it would 
exist for the lifetime of the host device.

> Some blk_mq_tagset_busy_iter() calls happen from a context where it is 
> not allowed to sleep but also where it is guaranteed that the tag set 
> won't disappear, e.g. the call from inside sdk_mq_queue_rq().

You're talking about skd_mq_queue_rq() -> skd_in_flight() -> 
blk_mq_tagset_busy_iter(), right?

So I would expect any .queue_rq calls to complete before the associated 
request queue and tagset may be unregistered.

> 
> How about using a mutex inside blk_mq_queue_tag_busy_iter() instead? As 
> far as I can see all blk_mq_queue_tag_busy_iter() happen from a context 
> where it is allowed to sleep.

Well then it seems sensible to add might_sleep() also.

And we still have the blk_mq_queue_tag_busy_iter() problem. As Ming 
mentioned yesterday, we know contexts where from where it is called 
which may not sleep.

Thanks,
John