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.
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.
Also ibmvnic_reset() checks for adapter->state without holding the lock
which might be racy, too.
Best regards
Uwe
[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()).
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
