From: Luka Gejak <luka.gejak@linux.dev>

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.

Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
---
 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;
 	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

On Tue, Mar 24, 2026 at 03:35:01PM +0100, luka.gejak@linux.dev wrote:
> From: Luka Gejak <luka.gejak@linux.dev>
>
> 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@linux.dev>
> ---
>  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
>