From: Simon Horman <[email protected]> Sent: Wednesday, July 8, 2026 7:03 PM
>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? That;s the only missing path that really matters > >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() */ > ... >} With un-attached netdev i believe there in rather no chance to race even if mac addr refresh would be conducted during pci error recovery > >[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); Both above notes relate to the already existing findings which i believe should not block this series since it touches different side of the driver. Addresing these notes would require service task refactor
