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

[Felix Maurer]

On Tue, Mar 24, 2026 at 03:35:01PM +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 ensuring the cleanup path targets the previously
> programmed slave device when the second slave fails to add the VID.

The cross-dev cleanup is already complex enough and I don't think this
makes it much better. How about having a central cleanup at the end that
cleans whatever ports have the VID added?

> Signed-off-by: Luka Gejak <luka.gejak@xxxxxxxxx>
> ---
>  net/hsr/hsr_device.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 5c3eca2235ce..6185f9f2630f 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -534,6 +534,8 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>  {
>  	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;

No matter if my previous comment, the is_slave_{a,b}_added bools are not
necessary with your changes but just duplicate state. Testing
slave_{a,b}_dev == NULL serves the same purpose.

>  	struct hsr_port *port;
>  	struct hsr_priv *hsr;
>  	int ret = 0;
> @@ -552,11 +554,12 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>  				/* clean up Slave-B */
>  				netdev_err(dev, "add vid failed for Slave-A\n");
>  				if (is_slave_b_added)
> -					vlan_vid_del(port->dev, proto, vid);
> +					vlan_vid_del(slave_b_dev, proto, vid);
>  				return ret;
>  			}
>
>  			is_slave_a_added = true;
> +			slave_a_dev = port->dev;
>  			break;
>
>  		case HSR_PT_SLAVE_B:
> @@ -564,11 +567,12 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>  				/* 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);
> +					vlan_vid_del(slave_a_dev, proto, vid);
>  				return ret;
>  			}
>
>  			is_slave_b_added = true;
> +			slave_b_dev = port->dev;
>  			break;
>  		default:
>  			break;
> --
> 2.53.0
>