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>] [day] [month] [year] [list]

Message-ID: <fbeb7dc0-8091-51a2-78e1-5f563a3ed53d@huawei.com>
Date:   Wed, 10 Feb 2021 14:39:53 +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>, <kashyap.desai@...adcom.com>,
        <linuxarm@...wei.com>, <pragalla@...eaurora.org>
Subject: Re: [RFC PATCH v2 2/2] blk-mq: Lockout tagset iter when freeing rqs

+

On 04/01/2021 18:37, John Garry wrote:

Hi Bart,

> 
>> Right, what I proposed is unrelated to the use-after-free triggered by
>> disabling I/O scheduling.
>>
>> Regarding the races triggered by disabling I/O scheduling: can these be
>> fixed by quiescing all request queues associated with a tag set before
>> changing the I/O scheduler instead of only the request queue for which 
>> the
>> I/O scheduler is changed? I think we already do this before updating the
>> number of hardware queues.
> 
> Maybe quiescing all request queues could solve the issue in 
> blk_mq_queue_tag_busy_iter(), as that issue involves interaction of 2x 
> request queues.
> 
> But the blk_mq_tagset_busy_iter() issue, above, it is related to only a 
> single request queue, so not sure how it could help.
> 
> But let me consider this more.

I have looked at this proposal again, that being to quiesce all (other) 
request queues prior to freeing the IO scheduler tags+requests. I tried 
this and it seems to work (changes not shown), in terms of solving the 
issue of blk_mq_queue_tag_busy_iter() and freeing the requests+tags 
racing. However, I am still not sure if it is acceptable to quiesce all 
request queues like this just when changing IO scheduler, even if it is 
done elsewhere.

In addition, this still leaves the blk_mq_tagset_busy_iter() issue; that 
being that freeing requests+tags can race against that same function. 
There is nothing to stop blk_mq_tagset_busy_iter() taking a reference to 
a request and that request being freed in parallel when switching IO 
scheduler, however unlikely.

Solving that becomes trickier. As Ming pointed out, that function can be 
called from softirq and even hard irq context - like 
ufshcd_tmv_handler() -> blk_mq_tagset_busy_iter() -  so there is a 
locking context issue.

Thanks,
John