[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