1. Patchew
  2. linux
  3. [PATCH net-next v2 0/4] net: hsr: address functional and concurrency bugs
  4. View patch

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

luka.gejak@linux.dev posted 4 patches 1 week ago
There is a newer version of this series
[PATCH net-next v2 2/4] net: hsr: fix VLAN add unwind on slave errors
Posted by luka.gejak@linux.dev 1 week ago 
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 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@linux.dev>
---
 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;
 
 		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;
@@ -576,6 +575,15 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
 	}
 
 	return 0;
+
+unwind:
+	if (slave_a_dev)
+		vlan_vid_del(slave_a_dev, proto, vid);
+
+	if (slave_b_dev)
+		vlan_vid_del(slave_b_dev, proto, vid);
+
+	return ret;
 }
 
 static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,
-- 
2.53.0
Re: [PATCH net-next v2 2/4] net: hsr: fix VLAN add unwind on slave errors
Posted by Felix Maurer 6 days, 8 hours ago 
Thank you for updating this patch!

On Thu, Mar 26, 2026 at 04:47:13PM +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 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@linux.dev>
> ---
>  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
Re: [PATCH net-next v2 2/4] net: hsr: fix VLAN add unwind on slave errors
Posted by Luka Gejak 6 days, 4 hours ago 
On March 27, 2026 12:52:02 PM GMT+01:00, Felix Maurer <fmaurer@redhat.com> wrote:
>Thank you for updating this patch!
>
>On Thu, Mar 26, 2026 at 04:47:13PM +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 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@linux.dev>
>> ---
>>  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
>

Hi Felix,
Thanks for taking a look at v2.
>nit: superflous empty line (it's inconsistent with the other case 
>blocks)
Good catch. I'll drop the extra newline in v3.
>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?
Very well. I will take your preference into consideration and will 
make mentioned changes in v3. However I am not currently home and will
send v3 once I come home in 1-2 days.
Best regards,
Luka Gejak
Re: [PATCH net-next v2 2/4] net: hsr: fix VLAN add unwind on slave errors
Posted by Felix Maurer 6 days, 1 hour ago 
On Fri, Mar 27, 2026 at 05:03:18PM +0100, Luka Gejak wrote:
> On March 27, 2026 12:52:02 PM GMT+01:00, Felix Maurer <fmaurer@redhat.com> wrote:
[...]
> >nit: superflous empty line (it's inconsistent with the other case
> >blocks)
> Good catch. I'll drop the extra newline in v3.
> >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?
> Very well. I will take your preference into consideration and will
> make mentioned changes in v3. However I am not currently home and will
> send v3 once I come home in 1-2 days.

Alright, thank you! No hurry, take your time. Just fyi, I'll be out of
office next week, but I'll definitely review v3 as soon as I'm back (if
nobody else did it in the meantime).

Thanks,
   Felix

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.