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