Netdev List
 help / color / mirror / Atom feed
* [PATCH v2 net] net: don't omit syncing RX filters to devices that are down
@ 2023-04-10 19:52 Vladimir Oltean
  2023-04-10 23:52 ` Vladimir Oltean
  0 siblings, 1 reply; 2+ messages in thread
From: Vladimir Oltean @ 2023-04-10 19:52 UTC (permalink / raw)
  To: netdev
  Cc: Jakub Kicinski, David S. Miller, Eric Dumazet, Paolo Abeni,
	David Ahern, Ido Schimmel

During NETDEV_DOWN netdev events, ipv4 devinet calls ip_mc_down(),
and ipv6 calls addrconf_ifdown(), and both of these eventually result in
calls to dev_mc_del(), either through igmp_group_dropped() or
igmp6_group_dropped(). During the handling of that notifier, the IFF_UP
dev->flags bit of the device is unset.

The problem is that, although dev_mc_del() does call __dev_set_rx_mode(),
this will not propagate all the way to the ndo_set_rx_mode() of the
device, because of the IFF_UP check removed by this change.

DSA does some processing in its dsa_slave_set_rx_mode(), and assumes
that all addresses that were synced by higher layers are also unsynced
by the time the device driver is unregistered.

That unsatisfied assumption triggers the WARN_ON(!list_empty(&dp->mdbs))
call from dsa_switch_release_ports(), and we leak memory corresponding
to the multicast addresses that were never unsynced.

Minimal reproducer:
ip link set swp0 up
ip link set swp0 down
echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/unbind

There are 2 possible ways to solve the issue.

One would be to change devinet and addrconf to react to the earlier
NETDEV_GOING_DOWN event (which is emitted while dev->flags still has
IFF_UP set). That would work, but it would require paying attention in
the future to other call paths that would also potentially need the same
change.

Alternatively, we could remove the check/optimization and thus make
dev_mc_del() always propagate down to the ndo_set_rx_mode() of the
device. This would implicitly solve the IGMP/IGMP6 code paths with DSA,
as well as any other potential issues of this kind with address deletion
not being synced prior to device removal.

Fixes: 5e8a1e03aa4d ("net: dsa: install secondary unicast and multicast addresses as host FDB/MDB")
Link: https://lore.kernel.org/netdev/ZDP2bxXGbHX8C4BC@shredder/
Suggested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1 is in the Link. A different approach was used here.

 net/core/dev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 480600a075ce..a83f725c76d8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8448,10 +8448,6 @@ void __dev_set_rx_mode(struct net_device *dev)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 
-	/* dev_open will call this function so the list will stay sane. */
-	if (!(dev->flags&IFF_UP))
-		return;
-
 	if (!netif_device_present(dev))
 		return;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-04-10 23:53 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-10 19:52 [PATCH v2 net] net: don't omit syncing RX filters to devices that are down Vladimir Oltean
2023-04-10 23:52 ` Vladimir Oltean

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox