[PATCH 1/2] nvme-pci: Fix multiple races in nvme_setup_io_queues()

[Casey Chen]

On 6/22/21 8:06 AM, Keith Busch wrote:
> On Mon, Jun 21, 2021 at 05:27:09PM -0700, Casey Chen wrote:
>> +	/*
>> +	 * Free IRQ resources as soon as NVMEQ_ENABLED bit transitions
>> +	 * from set to unset. If there is a window to it is truely freed,
>> +	 * pci_free_irq_vectors() jumping into this window will crash.
>> +	 * And take lock to avoid racing with pci_free_irq_vectors() in
>> +	 * nvme_dev_disable() path.
>> +	 */
>> +	mutex_lock(&dev->shutdown_lock)
> Sorry, I wasn't clear in previous review. All of the shutdown_locks
> taken after initialization need to by mutex_trylock()'s. If you have to
> wait to get the lock, the device is not going to be suitable for
> continuing initialization after the lock is available.
>
> And looking at this again, if trylock is successful, I think you need to
> verify controller state is suitable for continuing the initialization.
I assume you mean all the mutex_lock() except the one in 
nvme_dev_disable(), right ? I agree with replacing all mutex_lock() with 
mutex_trylock() except the one in nvme_dev_disable(). But I doubt about 
checking controller state after lock being acquired. Two reasons:

1. What state can use to check ? Suppose there is a state set on 
surprise removal, what if the state check happen before it is set while 
surprise removal is on its way ?

2. Even we don't check controller state after acquiring lock, the 
initialization still fail fairly soon if surprise removal happens. We 
don't sacrifice any performance.

Casey