Re: [PATCH v3 net-next] net: hsr: sync hw addr of slave2 according to slave1 hw addr on PRP

["Luka Gejak" <luka.gejak@linux.dev>]

On Wed Apr 9, 2025 at 12:19 PM CEST, Fernando Fernandez Mancera wrote:
> In order to work properly PRP requires slave1 and slave2 to share the
> same MAC address. To ease the configuration process on userspace tools,
> sync the slave2 MAC address with slave1. In addition, when deleting the
> port from the list, restore the original MAC address.
>
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
> v3: restore the original MAC address on slave removal.
> ---
>  net/hsr/hsr_device.c | 5 +++++
>  net/hsr/hsr_main.c   | 9 +++++++++
>  net/hsr/hsr_main.h   | 1 +
>  net/hsr/hsr_slave.c  | 2 ++
>  4 files changed, 17 insertions(+)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 439cfb7ad5d1..e8922e8d9398 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -761,6 +761,11 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
>  	if (res)
>  		goto err_unregister;
>  
> +	if (protocol_version == PRP_V1) {
> +		eth_hw_addr_set(slave[1], slave[0]->dev_addr);
> +		call_netdevice_notifiers(NETDEV_CHANGEADDR, slave[1]);
> +	}
> +
>  	if (interlink) {
>  		res = hsr_add_port(hsr, interlink, HSR_PT_INTERLINK, extack);
>  		if (res)
> diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
> index d7ae32473c41..192893c3f2ec 100644
> --- a/net/hsr/hsr_main.c
> +++ b/net/hsr/hsr_main.c
> @@ -78,6 +78,15 @@ static int hsr_netdev_notify(struct notifier_block *nb, unsigned long event,
>  			eth_hw_addr_set(master->dev, dev->dev_addr);
>  			call_netdevice_notifiers(NETDEV_CHANGEADDR,
>  						 master->dev);
> +
> +			if (hsr->prot_version == PRP_V1) {
> +				port = hsr_port_get_hsr(hsr, HSR_PT_SLAVE_B);
> +				if (port) {
> +					eth_hw_addr_set(port->dev, dev->dev_addr);
> +					call_netdevice_notifiers(NETDEV_CHANGEADDR,
> +								 port->dev);
> +				}
> +			}
>  		}
>  
>  		/* Make sure we recognize frames from ourselves in hsr_rcv() */
> diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
> index 1bc47b17a296..135ec5fce019 100644
> --- a/net/hsr/hsr_main.h
> +++ b/net/hsr/hsr_main.h
> @@ -155,6 +155,7 @@ struct hsr_port {
>  	struct hsr_priv		*hsr;
>  	enum hsr_port_type	type;
>  	struct rcu_head		rcu;
> +	unsigned char		original_macaddress[ETH_ALEN];
>  };
>  
>  struct hsr_frame_info;
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index 2a802a5de2ac..b87b6a6fe070 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -196,6 +196,7 @@ int hsr_add_port(struct hsr_priv *hsr, struct net_device *dev,
>  	port->hsr = hsr;
>  	port->dev = dev;
>  	port->type = type;
> +	ether_addr_copy(port->original_macaddress, dev->dev_addr);
>  
>  	if (type != HSR_PT_MASTER) {
>  		res = hsr_portdev_setup(hsr, dev, port, extack);
> @@ -232,6 +233,7 @@ void hsr_del_port(struct hsr_port *port)
>  		if (!port->hsr->fwd_offloaded)
>  			dev_set_promiscuity(port->dev, -1);
>  		netdev_upper_dev_unlink(port->dev, master->dev);
> +		eth_hw_addr_set(port->dev, port->original_macaddress);
>  	}
>  
>  	kfree_rcu(port, rcu);

Hi Fernando,

Thanks for the patch. The logic to sync the slave2 MAC address with 
slave1 for PRP is sound, but there are a few implementation issues here 
that will cause hardware filtering problems and style violations.

1. Hardware MAC Filtering
In your current implementation:

  if (protocol_version == PRP_V1) {

      eth_hw_addr_set(slave[1], slave[0]->dev_addr);

      call_netdevice_notifiers(NETDEV_CHANGEADDR, slave[1]);

  }

eth_hw_addr_set() is appropriate for virtual devices (like the hsr_dev 
itself), but slave[1] is a physical net_device. By using this helper, 
you are bypassing dev->netdev_ops->ndo_set_mac_address. This means the 
physical NIC's hardware MAC filter will not be programmed, and it will 
silently drop unicast traffic destined for the new PRP MAC address 
unless the interface happens to be in promiscuous mode. You should use 
dev_set_mac_address() here instead. Note that unlike eth_hw_addr_set(), 
dev_set_mac_address() can fail and returns an error code, so you will 
need to check its return value and handle the error path accordingly.

2. Teardown Notifiers
In hsr_del_port():

  netdev_upper_dev_unlink(port->dev, master->dev);
  eth_hw_addr_set(port->dev, port->original_macaddress);

You have the same issue with eth_hw_addr_set() here. Additionally, 
unlike the setup paths, this version omits call_netdevice_notifiers
(NETDEV_CHANGEADDR, port->dev). Switching to dev_set_mac_address() for 
the restoration will handle both the hardware programming and the  
notifier chain automatically.

3. Coding Style
In hsr_main.h:

  unsigned char original_macaddress[ETH_ALEN];

Please use u8 instead of unsigned char for byte arrays to align with 
standard netdev coding styles.

Please address these points and submit a v4.

Best regards,
Luka Gejak