Re: [PATCH net-next v2 2/4] net: hsr: fix VLAN add unwind on slave errors

[Felix Maurer]

Thank you for updating this patch!

On Thu, Mar 26, 2026 at 04:47:13PM +0100, luka.gejak@xxxxxxxxx wrote:
> From: Luka Gejak <luka.gejak@xxxxxxxxx>
>
> When vlan_vid_add() fails for a secondary slave, the error path calls
> vlan_vid_del() on the failing port instead of the peer slave that had
> already succeeded. This results in asymmetric VLAN state across the HSR
> pair.
>
> Fix this by switching to a centralized unwind path that removes the VID
> from any slave device that was already programmed.
>
> Signed-off-by: Luka Gejak <luka.gejak@xxxxxxxxx>
> ---
>  net/hsr/hsr_device.c | 46 ++++++++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 5c3eca2235ce..75c491279df8 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -532,8 +532,8 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)
>  static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>  				   __be16 proto, u16 vid)
>  {
> -	bool is_slave_a_added = false;
> -	bool is_slave_b_added = false;
> +	struct net_device *slave_a_dev = NULL;
> +	struct net_device *slave_b_dev = NULL;
>  	struct hsr_port *port;
>  	struct hsr_priv *hsr;
>  	int ret = 0;
> @@ -546,29 +546,28 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>  			continue;
>
>  		ret = vlan_vid_add(port->dev, proto, vid);
> -		switch (port->type) {
> -		case HSR_PT_SLAVE_A:
> -			if (ret) {
> -				/* clean up Slave-B */
> +		if (ret) {
> +			switch (port->type) {
> +			case HSR_PT_SLAVE_A:
>  				netdev_err(dev, "add vid failed for Slave-A\n");
> -				if (is_slave_b_added)
> -					vlan_vid_del(port->dev, proto, vid);
> -				return ret;
> +				break;
> +			case HSR_PT_SLAVE_B:
> +				netdev_err(dev, "add vid failed for Slave-B\n");
> +				break;
> +			default:
> +				break;
>  			}
>
> -			is_slave_a_added = true;
> +			goto unwind;
> +		}
> +
> +		switch (port->type) {
> +		case HSR_PT_SLAVE_A:
> +			slave_a_dev = port->dev;
>  			break;
>

nit: superflous empty line (it's inconsistent with the other case
blocks)

>  		case HSR_PT_SLAVE_B:
> -			if (ret) {
> -				/* clean up Slave-A */
> -				netdev_err(dev, "add vid failed for Slave-B\n");
> -				if (is_slave_a_added)
> -					vlan_vid_del(port->dev, proto, vid);
> -				return ret;
> -			}
> -
> -			is_slave_b_added = true;
> +			slave_b_dev = port->dev;
>  			break;
>  		default:
>  			break;

I think this would look cleaner with the good and the bad paths in the
same switch-case, but that may be a matter of (my) taste. What do you
think?

Thanks,
   Felix