Re: [PATCH net v2 4/5] net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] bcmgenet_set_mac_addr()
  (drivers/net/ethernet/broadcom/genet/bcmgenet.c:3628-3644) now hand-…

--

net: bcmgenet: validate Ethernet address in bcmgenet_set_mac_addr

This adds an is_valid_ether_addr() check to bcmgenet_set_mac_addr() so
that an invalid MAC address (all zeroes, or a multicast address) is
rejected with -EADDRNOTAVAIL instead of being programmed while the
interface is down.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index bfb74dca55d6f..e9d9d3d7b6369 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3634,6 +3634,9 @@ static int bcmgenet_set_mac_addr(struct net_device *dev, void *p)
>  	if (netif_running(dev))
>  		return -EBUSY;
>  
> +	if (!is_valid_ether_addr(addr->sa_data))
> +		return -EADDRNOTAVAIL;
> +
>  	eth_hw_addr_set(dev, addr->sa_data);
>  
>  	return 0;

[Severity: Low]
This isn't a bug, but with this hunk bcmgenet_set_mac_addr() now open-codes
exactly what the core helpers already do.

net/ethernet/eth.c:

	int eth_prepare_mac_addr_change(struct net_device *dev, void *p)
	{
		struct sockaddr *addr = p;

		if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
			return -EBUSY;
		if (!is_valid_ether_addr(addr->sa_data))
			return -EADDRNOTAVAIL;
		return 0;
	}

	void eth_commit_mac_addr_change(struct net_device *dev, void *p)
	{
		struct sockaddr *addr = p;

		eth_hw_addr_set(dev, addr->sa_data);
	}

The resulting body of bcmgenet_set_mac_addr() is then equivalent to
eth_mac_addr(). Would calling eth_prepare_mac_addr_change() followed by
eth_commit_mac_addr_change() (keeping the driver-specific comment about the
UniMAC RX/TX enable bits) be preferable to duplicating the validation here?

One small difference: the driver copy tests netif_running() unconditionally
and so does not honour the IFF_LIVE_ADDR_CHANGE exemption that the core
helper checks. bcmgenet does not set that private flag today, so there is no
behavioural change now, but the duplicated checks will not track future
changes to the core validation rules.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921220021.281418-1-florian.fainelli%40broadcom.com