Re: [Intel-wired-lan] [PATCH iwl-next v1 5/6] ixgbe: E610: add MAC address runtime refresh

[Simon Horman <horms@kernel.org>]

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://netdev-ai.bots.linux.dev/sashiko/
---
ixgbe: E610: add MAC address runtime refresh

This change polls RAR0 each service-task cycle on E610 and updates
netdev->dev_addr, hw->mac.addr and mac_table[0].addr when firmware has
altered the MAC.  A refresh is also invoked from ixgbe_reset() so the
address survives RAR0 clearing during init_hw().

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index df698ddd08be..eb05c2f982e8 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -6464,6 +6464,36 @@ void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
>  	}
>  }
>
> +static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	int err;
> +
> +	if (hw->mac.type != ixgbe_mac_e610)
> +		return;
> +
> +	/* fetch address stored currently in RAR0 in case the addr has been
> +	 * altered by FW; if so, use it as the default one
> +	 */
> +	err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);
> +	if (err) {
> +		e_dev_warn("Cannot get MAC address\n");
> +		return;
> +	}

[High]
Can this overwrite hw->mac.addr with transient/garbage RAR0 contents
before validation?  ixgbe_get_mac_addr_generic() unconditionally copies
the raw IXGBE_RAH(0)/IXGBE_RAL(0) values into the buffer passed by the
caller and always returns 0.  Here the caller passes hw->mac.addr
itself as the destination:

    err = hw->mac.ops.get_mac_addr(hw, hw->mac.addr);

so any transient RAR0 value (all zeros while firmware is rewriting the
register, garbage during the PCI slot-reset/resume window, a
multicast/zero pattern in flight) silently clobbers hw->mac.addr.

The is_valid_ether_addr() check below prevents publishing to netdev,
but it does not restore the previous hw->mac.addr.

Consider reading into a local u8 buf[ETH_ALEN], validating that, and
only then copying into hw->mac.addr.

Compounding that, ixgbe_get_mac_addr_generic()'s own comment states "A
reset of the adapter must be performed prior to calling this function
in order for the MAC address to have been loaded from the EEPROM into
RAR0", but in ixgbe_reset() the refresh runs before init_hw().  Can
this ordering yield a stale/invalid RAR0 read on the reset path?

The corruption is then observable: after the refresh returns,
ixgbe_reset() proceeds through init_hw() and ixgbe_mac_set_default_filter()
memcpy()s hw->mac.addr into mac_table[0].addr and programs it via
set_rar() — a corrupted hw->mac.addr can therefore end up written to
hardware.

> +
> +	if (ether_addr_equal(netdev->dev_addr, hw->mac.addr) ||
> +	    !is_valid_ether_addr(hw->mac.addr))
> +		return;
> +
> +	ASSERT_RTNL();
> +
> +	eth_hw_addr_set(netdev, hw->mac.addr);
> +	ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
> +
> +	call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);
> +}

[Medium]
Does this leave hw->mac.perm_addr, netdev->perm_addr and
netdev->addr_assign_type inconsistent with the newly published
address?  Every other MAC-handling site in ixgbe (probe,
ixgbe_reset_hw_e610, and the equivalent flows on 82598/82599/X540/X550)
writes RAR0 into hw->mac.perm_addr.  After firmware alters the MAC at
runtime, perm_addr will keep reporting the pre-change value, so
consumers such as ethtool -P, bonding and netlink perm-address
attributes will report the stale address while NET_ADDR_PERM is still
advertised for what is now effectively a runtime-assigned MAC.

[Medium]
This unconditionally treats RAR0 (owned by firmware/MGMT) as
authoritative and, on the next service-task tick, silently overwrites
any MAC previously configured via "ip link set dev X address ...":

    eth_hw_addr_set(netdev, hw->mac.addr);
    ether_addr_copy(adapter->mac_table[0].addr, hw->mac.addr);
    call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);

No log line, no arbitration with an admin-set address, no opt-out.
NETDEV_CHANGEADDR is also emitted from a polling context, which means
userspace (udev, NetworkManager, bridging, bonding) can receive
spontaneous address-change events on E610 with no correlating
administrative action.  Should the commit message describe this uAPI
behavior change ("ip link" no longer produces a persistent MAC on
E610), and should there be some form of arbitration or logging when
firmware overrides an admin-configured address?

> +
>  void ixgbe_reset(struct ixgbe_adapter *adapter)
>  {
>  	struct ixgbe_hw *hw = &adapter->hw;
> @@ -6481,6 +6511,8 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
>  			     IXGBE_FLAG2_SFP_NEEDS_RESET);
>  	adapter->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
>
> +	ixgbe_mac_addr_refresh(adapter);
> +

[High]
Is ixgbe_reset() always called with RTNL held?  ixgbe_mac_addr_refresh()
uses ASSERT_RTNL() and then dispatches eth_hw_addr_set() and
call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev), both of which
require RTNL.

The follow-up commit "ixgbe: take rtnl lock before ixgbe_reset() is
called" only converts the ixgbe_resume() path.  The PCI error-recovery
callback ixgbe_io_slot_reset() still calls ixgbe_reset() without
acquiring rtnl_lock(), and .slot_reset callbacks are not guaranteed to
run under RTNL.

On that path, if RAR0 differs from netdev->dev_addr on an E610,
ASSERT_RTNL() will WARN and the subsequent netdev address publication
and notifier-chain traversal race with concurrent RTNL-protected
users (SIOCSIFHWADDR, notifier chain (un)registration, etc).  Should
ixgbe_io_slot_reset() also take rtnl_lock() around ixgbe_reset(), or
should the refresh be moved to a caller-side location that guarantees
RTNL?

>  	err = hw->mac.ops.init_hw(hw);
>  	switch (err) {
>  	case 0:
> @@ -8651,6 +8683,11 @@ static void ixgbe_service_task(struct work_struct *work)
>  			ixgbe_handle_fw_event(adapter);
>  		ixgbe_check_media_subtask(adapter);
>  	}
> +
> +	rtnl_lock();
> +	ixgbe_mac_addr_refresh(adapter);
> +	rtnl_unlock();
> +

[Medium]
Would it make sense to hoist the E610 gating out of
ixgbe_mac_addr_refresh() and skip the rtnl_lock()/rtnl_unlock()
entirely for non-E610 devices?

The type check lives inside the helper:

    static void ixgbe_mac_addr_refresh(struct ixgbe_adapter *adapter)
    {
        ...
        if (hw->mac.type != ixgbe_mac_e610)
            return;
        ...
    }

so 82598, 82599, X540 and X550 adapters (which comprise most of the
ixgbe device family) acquire the global rtnl_mutex every service-task
cycle only to return immediately.  Something like:

    if (adapter->hw.mac.type == ixgbe_mac_e610) {
        rtnl_lock();
        ixgbe_mac_addr_refresh(adapter);
        rtnl_unlock();
    }

would avoid the rtnl_mutex contention on non-E610 hardware.

>  	ixgbe_reset_subtask(adapter);
>  	ixgbe_phy_interrupt_subtask(adapter);
>  	ixgbe_sfp_detection_subtask(adapter);