[PATCH net] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[PATCH net] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Wenbo Sun]

hsr_set_rx_mode() syncs unicast and multicast addresses to slave A and
slave B devices via dev_uc_sync_multiple() and dev_mc_sync_multiple(),
and hsr_change_rx_flags() propagates IFF_ALLMULTI via
dev_set_allmulti().

However, when an HSR device is deleted via hsr_dellink(),
hsr_del_ports() removes all ports from hsr->ports before
unregister_netdevice_queue() invokes hsr_dev_close(). Similarly, when a
slave device is unregistered while the HSR master is UP,
hsr_netdev_notify() calls hsr_del_port() directly. Because
hsr_del_port() does not unsync unicast/multicast addresses or restore
the allmulti count, the slave devices permanently retain synced hardware
addresses and a leaked allmulti reference count after port deletion.

Fix this by calling dev_uc_unsync() and dev_mc_unsync(), and
decrementing allmulti via dev_set_allmulti(port->dev, -1) if
IFF_ALLMULTI is set on the master device, when detaching slave A or
slave B in hsr_del_port().

This issue was identified with the assistance of an LLM-based code
review tool, confirmed by reproducing the leaked allmulti reference
count and synced multicast address in a User-Mode Linux (UML)
environment, and verified in UML that both states are properly cleaned
up upon HSR port deletion.

Fixes: 36b20fcdd966 ("net: hsr: Add support for MC filtering at the slave device")
Signed-off-by: Wenbo Sun <sunwenbo@cmss.chinamobile.com>
---
 net/hsr/hsr_slave.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index a546f70f9cc8..bab0c27dd06a 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -245,8 +245,14 @@ void hsr_del_port(struct hsr_port *port)
 		netdev_rx_handler_unregister(port->dev);
 		if (!port->hsr->fwd_offloaded || port->type == HSR_PT_INTERLINK)
 			dev_set_promiscuity(port->dev, -1);
-		if (port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B)
+		if (port->type == HSR_PT_SLAVE_A ||
+		    port->type == HSR_PT_SLAVE_B) {
 			vlan_vids_del_by_dev(port->dev, master->dev);
+			dev_uc_unsync(port->dev, master->dev);
+			dev_mc_unsync(port->dev, master->dev);
+			if (master->dev->flags & IFF_ALLMULTI)
+				dev_set_allmulti(port->dev, -1);
+		}
 		netdev_upper_dev_unlink(port->dev, master->dev);
 		if (hsr->prot_version == PRP_V1 &&
 		    port->type == HSR_PT_SLAVE_B) {
-- 
2.33.0

Re: [PATCH net] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Hangbin Liu]

On Wed, Nov 11, 2026 at 02:01:43PM +0800, Wenbo Sun wrote:
> hsr_set_rx_mode() syncs unicast and multicast addresses to slave A and
> slave B devices via dev_uc_sync_multiple() and dev_mc_sync_multiple(),
> and hsr_change_rx_flags() propagates IFF_ALLMULTI via
> dev_set_allmulti().
> 
> However, when an HSR device is deleted via hsr_dellink(),
> hsr_del_ports() removes all ports from hsr->ports before
> unregister_netdevice_queue() invokes hsr_dev_close(). Similarly, when a
> slave device is unregistered while the HSR master is UP,
> hsr_netdev_notify() calls hsr_del_port() directly. Because
> hsr_del_port() does not unsync unicast/multicast addresses or restore
> the allmulti count, the slave devices permanently retain synced hardware
> addresses and a leaked allmulti reference count after port deletion.
> 
> Fix this by calling dev_uc_unsync() and dev_mc_unsync(), and
> decrementing allmulti via dev_set_allmulti(port->dev, -1) if
> IFF_ALLMULTI is set on the master device, when detaching slave A or
> slave B in hsr_del_port().
> 
> This issue was identified with the assistance of an LLM-based code
> review tool, confirmed by reproducing the leaked allmulti reference
> count and synced multicast address in a User-Mode Linux (UML)
> environment, and verified in UML that both states are properly cleaned
> up upon HSR port deletion.
> 
> Fixes: 36b20fcdd966 ("net: hsr: Add support for MC filtering at the slave device")
> Signed-off-by: Wenbo Sun <sunwenbo@cmss.chinamobile.com>
> ---
>  net/hsr/hsr_slave.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index a546f70f9cc8..bab0c27dd06a 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -245,8 +245,14 @@ void hsr_del_port(struct hsr_port *port)
>  		netdev_rx_handler_unregister(port->dev);
>  		if (!port->hsr->fwd_offloaded || port->type == HSR_PT_INTERLINK)
>  			dev_set_promiscuity(port->dev, -1);
> -		if (port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B)
> +		if (port->type == HSR_PT_SLAVE_A ||
> +		    port->type == HSR_PT_SLAVE_B) {
>  			vlan_vids_del_by_dev(port->dev, master->dev);
> +			dev_uc_unsync(port->dev, master->dev);
> +			dev_mc_unsync(port->dev, master->dev);
> +			if (master->dev->flags & IFF_ALLMULTI)
> +				dev_set_allmulti(port->dev, -1);
> +		}
>  		netdev_upper_dev_unlink(port->dev, master->dev);
>  		if (hsr->prot_version == PRP_V1 &&
>  		    port->type == HSR_PT_SLAVE_B) {
> -- 
> 2.33.0
> 
> 
> 

Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

Re: [PATCH net] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Hangbin Liu]

On Wed, Nov 11, 2026 at 02:01:43PM +0800, Wenbo Sun wrote:
> hsr_set_rx_mode() syncs unicast and multicast addresses to slave A and
> slave B devices via dev_uc_sync_multiple() and dev_mc_sync_multiple(),
> and hsr_change_rx_flags() propagates IFF_ALLMULTI via
> dev_set_allmulti().
> 
> However, when an HSR device is deleted via hsr_dellink(),
> hsr_del_ports() removes all ports from hsr->ports before
> unregister_netdevice_queue() invokes hsr_dev_close(). Similarly, when a
> slave device is unregistered while the HSR master is UP,
> hsr_netdev_notify() calls hsr_del_port() directly. Because
> hsr_del_port() does not unsync unicast/multicast addresses or restore
> the allmulti count, the slave devices permanently retain synced hardware
> addresses and a leaked allmulti reference count after port deletion.
> 
> Fix this by calling dev_uc_unsync() and dev_mc_unsync(), and
> decrementing allmulti via dev_set_allmulti(port->dev, -1) if
> IFF_ALLMULTI is set on the master device, when detaching slave A or
> slave B in hsr_del_port().
> 
> This issue was identified with the assistance of an LLM-based code
> review tool, confirmed by reproducing the leaked allmulti reference
> count and synced multicast address in a User-Mode Linux (UML)
> environment, and verified in UML that both states are properly cleaned
> up upon HSR port deletion.
> 
> Fixes: 36b20fcdd966 ("net: hsr: Add support for MC filtering at the slave device")
> Signed-off-by: Wenbo Sun <sunwenbo@cmss.chinamobile.com>
> ---
>  net/hsr/hsr_slave.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index a546f70f9cc8..bab0c27dd06a 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -245,8 +245,14 @@ void hsr_del_port(struct hsr_port *port)
>  		netdev_rx_handler_unregister(port->dev);
>  		if (!port->hsr->fwd_offloaded || port->type == HSR_PT_INTERLINK)
>  			dev_set_promiscuity(port->dev, -1);
> -		if (port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B)
> +		if (port->type == HSR_PT_SLAVE_A ||
> +		    port->type == HSR_PT_SLAVE_B) {
>  			vlan_vids_del_by_dev(port->dev, master->dev);
> +			dev_uc_unsync(port->dev, master->dev);
> +			dev_mc_unsync(port->dev, master->dev);
> +			if (master->dev->flags & IFF_ALLMULTI)
> +				dev_set_allmulti(port->dev, -1);
> +		}
>  		netdev_upper_dev_unlink(port->dev, master->dev);
>  		if (hsr->prot_version == PRP_V1 &&
>  		    port->type == HSR_PT_SLAVE_B) {
> -- 
> 2.33.0
> 

Sorry. I just got a question.

hsr_del_port() is called via dev unregister or hsr_dellink() -> hsr_del_ports().
The hsr_dellink() also calls unregister devices.

The unregister process will stop a device if it's up. And hsr_dev_close() has
called dev_uc_unsync() and dev_mc_unsync().

Could you share more details on how you performed the testing?

Thanks
Hangbin

Re: [PATCH net] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Wenbo Sun]

Hi Hangbin,

Thank you very much for the Reviewed-by and for raising this question!

On Thu, Nov 12, 2026 at 09:05:41AM +0800, Hangbin Liu wrote:
> Sorry. I just got a question.
>
> hsr_del_port() is called via dev unregister or hsr_dellink() -> hsr_del_ports().
> The hsr_dellink() also calls unregister devices.
>
> The unregister process will stop a device if it's up. And hsr_dev_close() has
> called dev_uc_unsync() and dev_mc_unsync().
>
> Could you share more details on how you performed the testing?

Here is why hsr_dev_close() does not clean them up in those paths, and
how the test reproduces it:

1. Call order in hsr_dellink() (net/hsr/hsr_netlink.c):
   hsr_dellink() calls hsr_del_ports(hsr) *before*
   unregister_netdevice_queue(dev, head). Inside hsr_del_ports(), every
   port is removed from hsr->ports via list_del_rcu(&port->port_list)
   and freed. When unregister_netdevice_many_notify() later invokes
   netif_close_many() -> hsr_dev_close(), hsr_for_each_port_rtnl(hsr,
   port) sees an already-empty list, so dev_uc_unsync() and
   dev_mc_unsync() are never called on the slaves.
2. Similarly, when a slave is unregistered directly while hsr0 is UP,
   hsr_netdev_notify() (NETDEV_UNREGISTER) calls hsr_del_port(port)
   before any close/dellink takes place.
3. Also, hsr_dev_close() never calls dev_set_allmulti(port->dev, -1)
   (and dev_close() preserves IFF_ALLMULTI), so the allmulti refcount
   leaks even if hsr0 is brought down before deletion.

Here are the exact test steps I ran in UML (using veth pairs):

  ip link add veth0 type veth peer name veth0p
  ip link add veth1 type veth peer name veth1p
  ip link set veth0 up && ip link set veth0p up
  ip link set veth1 up && ip link set veth1p up
  # Create hsr0 with slave1=veth0, slave2=veth1 (RTM_NEWLINK)
  ip link set hsr0 up
  # Set IFF_ALLMULTI (SIOCSIFFLAGS) and add 01:00:5e:00:12:34 (SIOCADDMULTI)
  # Verify IFLA_ALLMULTI on veth0 == 1 and /proc/net/dev_mcast has 01005e001234
  ip link del hsr0
  # Re-check IFLA_ALLMULTI on veth0 and /proc/net/dev_mcast

Observed results on unpatched kernel:
  [BEFORE DEL] veth0 allmulti_refcnt=1, synced_mcast(01005e001234)=1
  veth0: left promiscuous mode
  veth1: left promiscuous mode
  [AFTER DEL]  veth0 allmulti_refcnt=1, synced_mcast(01005e001234)=1

Observed results with this patch:
  [BEFORE DEL] veth0 allmulti_refcnt=1, synced_mcast(01005e001234)=1
  veth0: left promiscuous mode
  veth0: left allmulticast mode
  veth1: left promiscuous mode
  veth1: left allmulticast mode
  [AFTER DEL]  veth0 allmulti_refcnt=0, synced_mcast(01005e001234)=0

I will also send a v2 with an updated commit message noting explicitly
that hsr_dev_close() sees an empty port list (along with your
Reviewed-by). Please let me know if you have any further questions.

Thanks again,
Wenbo

Re: [PATCH net] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Hangbin Liu]

On Thu, Nov 12, 2026 at 02:44:59PM +0800, Wenbo Sun wrote:
> Hi Hangbin,
> 
> Thank you very much for the Reviewed-by and for raising this question!
> 
> On Thu, Nov 12, 2026 at 09:05:41AM +0800, Hangbin Liu wrote:
> > Sorry. I just got a question.
> >
> > hsr_del_port() is called via dev unregister or hsr_dellink() -> hsr_del_ports().
> > The hsr_dellink() also calls unregister devices.
> >
> > The unregister process will stop a device if it's up. And hsr_dev_close() has
> > called dev_uc_unsync() and dev_mc_unsync().
> >
> > Could you share more details on how you performed the testing?
> 
> Here is why hsr_dev_close() does not clean them up in those paths, and
> how the test reproduces it:
> 
> 1. Call order in hsr_dellink() (net/hsr/hsr_netlink.c):
>    hsr_dellink() calls hsr_del_ports(hsr) *before*
>    unregister_netdevice_queue(dev, head). Inside hsr_del_ports(), every
>    port is removed from hsr->ports via list_del_rcu(&port->port_list)
>    and freed. When unregister_netdevice_many_notify() later invokes
>    netif_close_many() -> hsr_dev_close(), hsr_for_each_port_rtnl(hsr,
>    port) sees an already-empty list, so dev_uc_unsync() and
>    dev_mc_unsync() are never called on the slaves.

Oh, since dev_uc_unsync() and dev_mc_unsync() is never called by
hsr_dev_close(), should we remove them from there?

On the other hand, the comment of dev_uc_unsync() said it is intended
to be called from the dev->stop.

So we need to chose one place for these undoes. I personally agree to
add it in hsr_dellink() as all the hsr info is remove in hsr_dellink.

> 2. Similarly, when a slave is unregistered directly while hsr0 is UP,
>    hsr_netdev_notify() (NETDEV_UNREGISTER) calls hsr_del_port(port)
>    before any close/dellink takes place.
> 3. Also, hsr_dev_close() never calls dev_set_allmulti(port->dev, -1)
>    (and dev_close() preserves IFF_ALLMULTI), so the allmulti refcount
>    leaks even if hsr0 is brought down before deletion.

Yes, I see the allmulti is not called in hsr_dev_close.

> 
> Here are the exact test steps I ran in UML (using veth pairs):
> 
>   ip link add veth0 type veth peer name veth0p
>   ip link add veth1 type veth peer name veth1p
>   ip link set veth0 up && ip link set veth0p up
>   ip link set veth1 up && ip link set veth1p up
>   # Create hsr0 with slave1=veth0, slave2=veth1 (RTM_NEWLINK)
>   ip link set hsr0 up
>   # Set IFF_ALLMULTI (SIOCSIFFLAGS) and add 01:00:5e:00:12:34 (SIOCADDMULTI)
>   # Verify IFLA_ALLMULTI on veth0 == 1 and /proc/net/dev_mcast has 01005e001234
>   ip link del hsr0
>   # Re-check IFLA_ALLMULTI on veth0 and /proc/net/dev_mcast
> 
> Observed results on unpatched kernel:
>   [BEFORE DEL] veth0 allmulti_refcnt=1, synced_mcast(01005e001234)=1
>   veth0: left promiscuous mode
>   veth1: left promiscuous mode
>   [AFTER DEL]  veth0 allmulti_refcnt=1, synced_mcast(01005e001234)=1
> 
> Observed results with this patch:
>   [BEFORE DEL] veth0 allmulti_refcnt=1, synced_mcast(01005e001234)=1
>   veth0: left promiscuous mode
>   veth0: left allmulticast mode
>   veth1: left promiscuous mode
>   veth1: left allmulticast mode
>   [AFTER DEL]  veth0 allmulti_refcnt=0, synced_mcast(01005e001234)=0

Thanks, this reproducer is more detailed. Would you please
add this info in the --- part of next patch?
> 
> I will also send a v2 with an updated commit message noting explicitly
> that hsr_dev_close() sees an empty port list (along with your
> Reviewed-by). Please let me know if you have any further questions.

Please drop my reviewed by tag if the code changed.

Thanks
Hangbin

[PATCH net v2] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Wenbo Sun]

hsr_set_rx_mode() syncs unicast and multicast addresses to slave A and
slave B devices via dev_uc_sync_multiple() and dev_mc_sync_multiple(),
and hsr_change_rx_flags() propagates IFF_ALLMULTI via
dev_set_allmulti().

When an UP HSR device is deleted via hsr_dellink(), hsr_del_ports()
removes all ports from hsr->ports before unregister_netdevice_queue()
leads to hsr_dev_close(). Consequently, hsr_dev_close() iterates over an
already-empty port list and cannot execute dev_uc_unsync() or
dev_mc_unsync() on the detached slaves. Likewise, when a slave device is
unregistered while the HSR master is UP, hsr_netdev_notify() calls
hsr_del_port() directly without running hsr_dev_close(). In addition,
hsr_dev_close() does not release IFF_ALLMULTI on slave devices either.
As a result, detached slave devices can retain synced unicast/multicast
addresses and an elevated allmulti reference count.

Clean up these states in hsr_del_port() when detaching slave A or slave B
by calling dev_uc_unsync(), dev_mc_unsync(), and, when IFF_ALLMULTI is
set on the master device, dev_set_allmulti(port->dev, -1).

This issue was identified with the assistance of an LLM-based code
review tool, confirmed by reproducing the retained allmulti reference
count and synced multicast address in a User-Mode Linux (UML)
environment upon deleting an active HSR interface, and verified in UML
that both states are properly cleared after HSR port deletion.

Fixes: 36b20fcdd966 ("net: hsr: Add support for MC filtering at the slave device")
Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>
Signed-off-by: Wenbo Sun <sunwenbo@cmss.chinamobile.com>
---
Changes in v2:
- Clarify the commit message to explain explicitly why hsr_dev_close()
  iterates over an already-empty hsr->ports list on hsr_dellink() and
  does not release IFF_ALLMULTI, answering Hangbin Liu's question.
- Add Reviewed-by from Hangbin Liu.
- Link to v1: https://lore.kernel.org/r/20261111060143.183670-1-sunwenbo@cmss.chinamobile.com

 net/hsr/hsr_slave.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index a546f70f9cc8..bab0c27dd06a 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -245,8 +245,14 @@ void hsr_del_port(struct hsr_port *port)
 		netdev_rx_handler_unregister(port->dev);
 		if (!port->hsr->fwd_offloaded || port->type == HSR_PT_INTERLINK)
 			dev_set_promiscuity(port->dev, -1);
-		if (port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B)
+		if (port->type == HSR_PT_SLAVE_A ||
+		    port->type == HSR_PT_SLAVE_B) {
 			vlan_vids_del_by_dev(port->dev, master->dev);
+			dev_uc_unsync(port->dev, master->dev);
+			dev_mc_unsync(port->dev, master->dev);
+			if (master->dev->flags & IFF_ALLMULTI)
+				dev_set_allmulti(port->dev, -1);
+		}
 		netdev_upper_dev_unlink(port->dev, master->dev);
 		if (hsr->prot_version == PRP_V1 &&
 		    port->type == HSR_PT_SLAVE_B) {
-- 
2.33.0

Re: [PATCH net v2] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Hangbin Liu]

On Thu, Nov 12, 2026 at 02:47:16PM +0800, Wenbo Sun wrote:
> hsr_set_rx_mode() syncs unicast and multicast addresses to slave A and
> slave B devices via dev_uc_sync_multiple() and dev_mc_sync_multiple(),
> and hsr_change_rx_flags() propagates IFF_ALLMULTI via
> dev_set_allmulti().
> 
> When an UP HSR device is deleted via hsr_dellink(), hsr_del_ports()
> removes all ports from hsr->ports before unregister_netdevice_queue()
> leads to hsr_dev_close(). Consequently, hsr_dev_close() iterates over an
> already-empty port list and cannot execute dev_uc_unsync() or
> dev_mc_unsync() on the detached slaves. Likewise, when a slave device is
> unregistered while the HSR master is UP, hsr_netdev_notify() calls
> hsr_del_port() directly without running hsr_dev_close(). In addition,
> hsr_dev_close() does not release IFF_ALLMULTI on slave devices either.
> As a result, detached slave devices can retain synced unicast/multicast
> addresses and an elevated allmulti reference count.
> 
> Clean up these states in hsr_del_port() when detaching slave A or slave B
> by calling dev_uc_unsync(), dev_mc_unsync(), and, when IFF_ALLMULTI is
> set on the master device, dev_set_allmulti(port->dev, -1).
> 
> This issue was identified with the assistance of an LLM-based code
> review tool, confirmed by reproducing the retained allmulti reference
> count and synced multicast address in a User-Mode Linux (UML)
> environment upon deleting an active HSR interface, and verified in UML
> that both states are properly cleared after HSR port deletion.
> 
> Fixes: 36b20fcdd966 ("net: hsr: Add support for MC filtering at the slave device")
> Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>
> Signed-off-by: Wenbo Sun <sunwenbo@cmss.chinamobile.com>
> ---
> Changes in v2:
> - Clarify the commit message to explain explicitly why hsr_dev_close()
>   iterates over an already-empty hsr->ports list on hsr_dellink() and
>   does not release IFF_ALLMULTI, answering Hangbin Liu's question.
> - Add Reviewed-by from Hangbin Liu.
> - Link to v1: https://lore.kernel.org/r/20261111060143.183670-1-sunwenbo@cmss.chinamobile.com
> 
>  net/hsr/hsr_slave.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index a546f70f9cc8..bab0c27dd06a 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -245,8 +245,14 @@ void hsr_del_port(struct hsr_port *port)
>  		netdev_rx_handler_unregister(port->dev);
>  		if (!port->hsr->fwd_offloaded || port->type == HSR_PT_INTERLINK)
>  			dev_set_promiscuity(port->dev, -1);
> -		if (port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B)
> +		if (port->type == HSR_PT_SLAVE_A ||
> +		    port->type == HSR_PT_SLAVE_B) {
>  			vlan_vids_del_by_dev(port->dev, master->dev);
> +			dev_uc_unsync(port->dev, master->dev);
> +			dev_mc_unsync(port->dev, master->dev);
> +			if (master->dev->flags & IFF_ALLMULTI)
> +				dev_set_allmulti(port->dev, -1);
> +		}
>  		netdev_upper_dev_unlink(port->dev, master->dev);
>  		if (hsr->prot_version == PRP_V1 &&
>  		    port->type == HSR_PT_SLAVE_B) {
> -- 
> 2.33.0
> 
> 
> 

Sorry, you post v2 version to fast. Please check if
dev_uc_unsync()/dev_mc_unsync() could be called from hsr_dev_close(). If not,
should we remove them from hsr_dev_close()?

Thanks
Hangbin

Re: [PATCH net v2] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Wenbo Sun]

Hi Hangbin,

I sincerely apologize for sending out v2 too quickly yesterday before
giving you enough time to review the previous discussion. I really appreciate
your patience and thoughtful guidance.

Regarding your question on whether dev_uc_unsync()/dev_mc_unsync() could be
called from hsr_dev_close(), and whether they should be removed:

I traced the function calls and verified the behavior in a User-Mode Linux
(UML) environment across different lifecycle events:

1. Can they be called from hsr_dev_close()?
   Yes. When an administrator administratively brings down the HSR interface
   without deleting it (e.g. running "ip link set hsr0 down"):
   - hsr_dellink() is NOT called, and ports remain in hsr->ports.
   - dev_close() triggers ndo_stop (hsr_dev_close()), where hsr_for_each_port_rtnl()
     successfully iterates over SLAVE_A and SLAVE_B.
   - Both dev_uc_unsync() and dev_mc_unsync() ARE actively called here to unsync
     addresses, ensuring physical slaves stop filtering for HSR multicast while
     hsr0 is DOWN (and re-synced by hsr_set_rx_mode() once brought back UP).
   Therefore, they are NOT dead code in hsr_dev_close() and should NOT be removed.

2. Why do we still need the cleanup in hsr_del_port()?
   Because in port removal/deletion paths, hsr_dev_close() cannot perform this cleanup:
   - Master deletion ("ip link del hsr0"): In hsr_dellink(), hsr_del_ports()
     detaches all ports from hsr->ports BEFORE calling unregister_netdevice_queue().
     When unregister_netdevice_many_notify() subsequently invokes hsr_dev_close(),
     hsr->ports is already empty, so the unsync loop runs 0 times.
   - Slave unregistration ("ip link del veth0" while hsr0 is UP): hsr_netdev_notify()
     calls hsr_del_port() directly without calling hsr_dev_close().
   - In addition, hsr_change_rx_flags() propagates IFF_ALLMULTI via
     dev_set_allmulti(slave, 1), but neither hsr_dev_close() nor the original
     hsr_del_port() ever restored it.

   Adding dev_uc_unsync(), dev_mc_unsync(), and dev_set_allmulti(port->dev, -1)
   in hsr_del_port() ensures that whenever a slave port is detached, its synced
   state and allmulti count are cleanly restored (mirroring the cleanup in
   macvlan_uninit() and team_port_del()).

Per your comment on v1, I have dropped your Reviewed-by tag and will incorporate
the detailed reproducer and lifecycle trace steps into the commit message notes
(under the --- section).

Because v2 was sent yesterday at 14:47:16 (+0800), I will wait until the 24-hour
cooldown period has elapsed (after 14:47:16 today) before submitting [PATCH net v3],
giving us full time to discuss.

Please let me know if this clarifies the call flow and whether you are comfortable
with this plan.

Thanks again for your time!

Best regards,
Wenbo Sun

Re: [PATCH net v2] net: hsr: clean up synced addresses and allmulti in hsr_del_port()

[Hangbin Liu]

Hi Wenbo,
On Fri, Nov 13, 2026 at 02:06:23PM +0800, Wenbo Sun wrote:
> Hi Hangbin,
> 
> I sincerely apologize for sending out v2 too quickly yesterday before
> giving you enough time to review the previous discussion. I really appreciate
> your patience and thoughtful guidance.
> 
> Regarding your question on whether dev_uc_unsync()/dev_mc_unsync() could be
> called from hsr_dev_close(), and whether they should be removed:
> 
> I traced the function calls and verified the behavior in a User-Mode Linux
> (UML) environment across different lifecycle events:
> 
> 1. Can they be called from hsr_dev_close()?
>    Yes. When an administrator administratively brings down the HSR interface
>    without deleting it (e.g. running "ip link set hsr0 down"):
>    - hsr_dellink() is NOT called, and ports remain in hsr->ports.
>    - dev_close() triggers ndo_stop (hsr_dev_close()), where hsr_for_each_port_rtnl()
>      successfully iterates over SLAVE_A and SLAVE_B.
>    - Both dev_uc_unsync() and dev_mc_unsync() ARE actively called here to unsync
>      addresses, ensuring physical slaves stop filtering for HSR multicast while
>      hsr0 is DOWN (and re-synced by hsr_set_rx_mode() once brought back UP).
>    Therefore, they are NOT dead code in hsr_dev_close() and should NOT be removed.

Thanks, this looks good to me. We don’t need to clear the IFF_ALLMULTI flag
because the interface is only being brought down, we still need this flag
after it comes back up.

When the interface is brought up, `hsr_set_rx_mode` is also called to ensure
`dev_mc/uc_sync_multiple` runs. So the patch looks good to me.

Thanks
Hangbin