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()).
