netdev - [PATCH v2 net] net: don't omit syncing RX filters to devices that are down

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20230410195220.1335670-1-vladimir.oltean@nxp.com>
Date:   Mon, 10 Apr 2023 22:52:20 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        David Ahern <dsahern@...nel.org>,
        Ido Schimmel <idosch@...dia.com>
Subject: [PATCH v2 net] net: don't omit syncing RX filters to devices that are down

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@...dia.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@....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