Re: NVMe induced NULL deref in bt_iter()

Sagi Grimberg
2017-07-02T15:08:48Z

>> Hmm,
>>
>> In the above flow we should not be freeing the tag_set, not on admin as
>> well. The target keep removing namespaces and finally removes the
>> subsystem which generates a error recovery flow. What we at least try
>> to do is:
>>
>> 1. mark rdma queues as not live
>> 2. stop all the sw queues (admin and io)
>> 3. fail inflight I/Os
>> 4. restart all sw queues (to fast fail until we recover)
>>
>> We shouldn't be freeing the tagsets (although we might update them
>> when we recover and cpu map changed - which I don't think is happening).
>>
>> However, I do see a difference between bt_tags_for_each
>> and blk_mq_flush_busy_ctxs (checks tags->rqs not being NULL).
>>
>> Unrelated to this I think we should quiesce/unquiesce the admin_q
>> instead of stop/start because it respects the submission path rcu [1].
>>
>> It might hide the issue, but given that we never free the tagset its
>> seems like it's not in nvme-rdma (max, can you see if this makes the
>> issue go away?)
>
> Yes, this fixes the null deref issue.
> I run some additional login/logout tests that passed too.
> This fix is important also for stable kernel (with needed backports to
> blk_mq_quiesce_queue/blk_mq_unquiesce_queue functions).
> You can add my:
> Tested-by: Max Gurtovoy <maxg@mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>

Thanks for clarifying Max.

However I still think its not the root cause (unless I don't understand
it).

As I said, we do not free the tagset so I'm not sure why we get to
a NULL deref in the sbitmap code. Jens, can you explain why
changing blk_mq_stop_hw_queues to blk_mq_quiesce_queue makes the issue
go away? I know that quiesce respects the rcu grace, but I still do not
understand why without it we get a NULL sb->map.

> Let me know if you want me to push this fix to the mailing list to save
> time (can we make it to 4.12 ?)

I can send patches, we need it in pci, fc and loop too..

I don't think its a 4.12 material as we are way too late to this sort of
fix.