This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ixgbe: take rtnl lock before ixgbe_reset() is called
Previous commit introduced ixgbe_mac_addr_refresh which touches netdev
struct by updating mac addr. It should operate after taking rtnl lock.
One of the callers is ixgbe_reset(). Most of scenarios when ixgbe_reset()
is called met taking lock requirement, but there is a ixgbe_resume() path
which calls ixgbe_reset() -> ixgbe_mac_addr_refresh() without taking
the lock. So there is a risk of race.
Move rtnl_lock() before ixgbe_reset() is called.
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index eb05c2f982e8e..aa5a61f95d883 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7535,11 +7535,11 @@ static int ixgbe_resume(struct device *dev_d)
>
> device_wakeup_disable(dev_d);
>
> + rtnl_lock();
> ixgbe_reset(adapter);
[Severity: High]
This patch fixes the missing rtnl_lock() in ixgbe_resume(), but is this fix
incomplete?
It looks like there are still other paths that call ixgbe_reset() without
acquiring rtnl_lock(), which would trigger the same race condition and
ASSERT_RTNL() warning on the MAC address update.
For example, the PCI error recovery handler calls it directly:
ixgbe_io_slot_reset() {
...
ixgbe_reset(adapter);
...
}
And the service task can also call it without rtnl_lock():
ixgbe_handle_fw_event() {
...
ixgbe_down(adapter); /* calls ixgbe_reset() */
...
}
[Severity: High]
This isn't a bug introduced by this patch, but does ixgbe_reset() also modify
the unicast address list without the required lock?
ixgbe_reset() directly calls __dev_uc_unsync(netdev, NULL) which iterates
over and modifies the dev->uc address list. However, __dev_uc_unsync() is a
lockless helper that expects the caller to hold netif_addr_lock_bh(netdev).
Since the callers of ixgbe_reset() only hold rtnl_lock() (or no lock),
could this cause list corruption or use-after-free if the device resets
concurrently with unicast address modifications?
[Severity: High]
This is a pre-existing issue, but does ixgbe_reset() use an ad-hoc
synchronization mechanism instead of standard primitives?
Inside ixgbe_reset():
/* lock SFP init bit to prevent race conditions with the watchdog */
while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
usleep_range(1000, 2000);
Reimplementing a lock with test_and_set_bit() and usleep_range() lacks
deadlock detection (lockdep visibility), priority inheritance, and fairness.
Should this use a proper mutex or spinlock instead of spinning on a bit flag?
>
> IXGBE_WRITE_REG(&adapter->hw, IXGBE_WUS, ~0);
>
> - rtnl_lock();
> err = ixgbe_init_interrupt_scheme(adapter);
