linux-kernel - Re: [PATCH 7/8] nvme: use blk_mq_queue_tag_busy

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]

Date:   Fri, 15 Mar 2019 17:49:20 +0100
From:   Hannes Reinecke <hare@...e.de>
To:     James Smart <james.smart@...adcom.com>,
        James Smart <jsmart2021@...il.com>,
        Jianchao Wang <jianchao.w.wang@...cle.com>, axboe@...nel.dk
Cc:     linux-block@...r.kernel.org, sagi@...mberg.me,
        josef@...icpanda.com, linux-nvme@...ts.infradead.org,
        linux-kernel@...r.kernel.org, keith.busch@...el.com,
        jthumshirn@...e.de, hch@....de, bvanassche@....org
Subject: Re: [PATCH 7/8] nvme: use blk_mq_queue_tag_busy_iter

On 3/15/19 5:39 PM, James Smart wrote:
> 
> 
> On 3/15/2019 9:33 AM, James Smart wrote:
>> On 3/15/2019 1:57 AM, Jianchao Wang wrote:
>>> blk_mq_tagset_busy_iter is not safe that it could get stale request
>>> in tags->rqs[]. Use blk_mq_queue_tag_busy_iter here. A new helper
>>> interface nvme_iterate_inflight_rqs is introduced to iterate
>>> all of the ns under a ctrl.
>>>
>>> Signed-off-by: Jianchao Wang <jianchao.w.wang@...cle.com>
>>> ---
>>>   drivers/nvme/host/core.c   | 12 ++++++++++++
>>>   drivers/nvme/host/fc.c     | 12 ++++++------
>>>   drivers/nvme/host/nvme.h   |  2 ++
>>>   drivers/nvme/host/pci.c    |  5 +++--
>>>   drivers/nvme/host/rdma.c   |  6 +++---
>>>   drivers/nvme/host/tcp.c    |  5 +++--
>>>   drivers/nvme/target/loop.c |  6 +++---
>>>   7 files changed, 32 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 6a9dd68..5760fad 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -3816,6 +3816,18 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
>>>   }
>>>   EXPORT_SYMBOL_GPL(nvme_start_queues);
>>>   +void nvme_iterate_inflight_rqs(struct nvme_ctrl *ctrl,
>>> +        busy_iter_fn *fn, void *data)
>>> +{
>>> +    struct nvme_ns *ns;
>>> +
>>> +    down_read(&ctrl->namespaces_rwsem);
>>> +    list_for_each_entry(ns, &ctrl->namespaces, list)
>>> +        blk_mq_queue_tag_busy_iter(ns->queue, fn, data, true);
>>> +    up_read(&ctrl->namespaces_rwsem);
>>> +}
>>> +EXPORT_SYMBOL_GPL(nvme_iterate_inflight_rqs);
>>
>> Hmm... so this aborts ios for namespaces. How about ios that aren't 
>> for a namespace but rather for the controller itself.  For example, 
>> anything on the admin queue ?   Rather critical for those ios be 
>> terminated as well.
> NVM - didn't look at which tag set.  but it does highlight - doesn't 
> cover anything issued by core/transport on the io queues.
> 
Actually, I would consider that an error.
_All_ commands on io queues (internal or block-layer generated) whould 
be tracked sbitmap and hence the tag iterator.
That's what we have the 'private' tags for, and why the iter callback 
has this ominous 'bool' argument...

Cheers,

Hannes