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

Felix Maurer
2026-03-26T14:29:40Z

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
>