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

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 3/29/26 5:07 PM, Luka Gejak wrote:
> 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.
> 

These points makes sense. I guess I didn't notice the that 
eth_hw_addr_set() due to my interface being set to promiscuous mode at 
hsr_slave.c.

Let me research a bit more about the impact here and will get back to 
you either with a new patch or an reply here.

> 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.
> 

I used `unsigned char` because it was already used around HSR code 
already, so I chose consistency. If there is a preference for u8 over 
unsigned char I could change it.

> Please address these points and submit a v4.
> 

Well, this is almost a year old so I am sending a fix instead. Is this 
some kind of automated bot going through all the patches in netdev 
mailing list? I am a bit confused about receiving a patch review a year 
later.

> Best regards,
> Luka Gejak
>