On Mon, Jul 03, 2017 at 03:46:34PM +0300, Max Gurtovoy wrote:

On 7/3/2017 3:03 PM, Ming Lei wrote:

quoted

On Mon, Jul 03, 2017 at 01:07:44PM +0300, Sagi Grimberg wrote:

quoted

Hi Ming,

quoted

Yeah, the above change is correct, for any canceling requests in this
way we should use blk_mq_quiesce_queue().

I still don't understand why should blk_mq_flush_busy_ctxs hit a NULL
deref if we don't touch the tagset...

Looks no one mentioned the steps for reproduction, then it isn't easy
to understand the related use case, could anyone share the steps for
reproduction?

Hi Ming,
I create 500 ns per 1 subsystem (using with CX4 target and C-IB initiator
but also saw it in CX5 vs. CX5 setup).
The null deref happens when I remove all configuration in the target (1 port
1 subsystem and 500 namespaces and nvmet modules unload) during traffic to 1
nvme device/ns from the intiator.
I get Null deref in blk_mq_flush_busy_ctxs function that calls
sbitmap_for_each_set in the initiator. seems like the "struct sbitmap_word
*word = &sb->map[i];" is null. It's actually might be not null in the
beginning of the func and become null during running the while loop there.

So looks it is still a normal release in initiator.

Per my experience, without quiescing queue before
blk_mq_tagset_busy_iter() for canceling requests, request double free
can be caused: one submitted req in .queue_rq can completed in
blk_mq_end_request(), meantime it can be completed in
nvme_cancel_request(). That is why we have to quiescing queue
first before canceling request in this way. Except for NVMe, looks
NBD and mtip32xx need fix too.

This way might cause blk_cleanup_queue() to complete early, then NULL
deref can be triggered in blk_mq_flush_busy_ctxs(). But in my previous
debug in PCI NVMe, this wasn't seen yet.

It should have been verified if the above is true by adding some debug
message inside blk_cleanup_queue(). 


Thanks,
Ming