Re: ibmvnic: Race condition in remove callback

On 2021-01-17 02:12, Uwe Kleine-König wrote:

Hello,

while working on some cleanup I stumbled over a problem in the 
ibmvnic's
remove callback. Since commit

        7d7195a026ba ("ibmvnic: Do not process device remove during
device reset")

there is the following code in the remove callback:

        static int ibmvnic_remove(struct vio_dev *dev)
        {
                ...
                spin_lock_irqsave(&adapter->state_lock, flags);
                if (test_bit(0, &adapter->resetting)) {
                        spin_unlock_irqrestore(&adapter->state_lock, 
flags);
                        return -EBUSY;
                }

                adapter->state = VNIC_REMOVING;
                spin_unlock_irqrestore(&adapter->state_lock, flags);

                flush_work(&adapter->ibmvnic_reset);
                flush_delayed_work(&adapter->ibmvnic_delayed_reset);
                ...
        }

Unfortunately returning -EBUSY doesn't work as intended. That's because
the return value of this function is ignored[1] and the device is
considered unbound by the device core (shortly) after ibmvnic_remove()
returns.

Oh! I was not aware of this. In our code review, a question on whether 
or not device reset should have a higher precedence over device remove 
was raised before. So, now it is clear that this driver has to take care 
of remove over reset.

While looking into fixing that I noticed a worse problem:

If ibmvnic_reset() (e.g. called by the tx_timeout callback) calls
schedule_work(&adapter->ibmvnic_reset); just after the work queue is
flushed above the problem that 7d7195a026ba intends to fix will trigger
resulting in a use-after-free.

It was proposed that when coming into ibmvnic_remove() we lock down the 
workqueue to prevent future access, flush, cleanup, then unregister the 
device. Your thought on this?

Also ibmvnic_reset() checks for adapter->state without holding the lock
which might be racy, too.

Suka started addressing consistent locking with this patch series:
https://lists.openwall.net/netdev/2021/01/08/89

He is reworking this.

Best regards
Uwe

Thank you for taking the time to review this driver, Uwe. This is very 
helpful for us.

Best Regards,
Dany

[1] vio_bus_remove (in arch/powerpc/platforms/pseries/vio.c) records 
the
    return value and passes it on. But the driver core doesn't care for
    the return value (see __device_release_driver() in 
drivers/base/dd.c
    calling dev->bus->remove()).