netdev - RE: Possible to use both dev_mc_sync and __dev_mc

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SA1PR15MB513713A75488DB88C7222C2DBD169@SA1PR15MB5137.namprd15.prod.outlook.com>
Date:   Mon, 21 Mar 2022 18:37:05 +0000
From:   Alexander Duyck <alexanderduyck@...com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...dia.com>,
        Florian Fainelli <f.fainelli@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: Possible to use both dev_mc_sync and __dev_mc_sync?

> -----Original Message-----
> From: Vladimir Oltean <olteanv@...il.com>
> Sent: Monday, March 21, 2022 9:32 AM
> To: Alexander Duyck <alexanderduyck@...com>; Jakub Kicinski
> <kuba@...nel.org>; Jiri Pirko <jiri@...dia.com>; Florian Fainelli
> <f.fainelli@...il.com>
> Cc: netdev@...r.kernel.org
> Subject: Possible to use both dev_mc_sync and __dev_mc_sync?
> 
> Hello,
> 
> Despite the similar names, the 2 functions above serve quite different
> purposes, and as it happens, DSA needs to use both of them, each for its
> own purpose.
> 
> static void dsa_slave_set_rx_mode(struct net_device *dev) {
> 	struct net_device *master = dsa_slave_to_master(dev);
> 	struct dsa_port *dp = dsa_slave_to_port(dev);
> 	struct dsa_switch *ds = dp->ds;
> 
> 	dev_mc_sync(master, dev); // DSA is a stacked device
> 	dev_uc_sync(master, dev);
> 	if (dsa_switch_supports_mc_filtering(ds))
> 		__dev_mc_sync(dev, dsa_slave_sync_mc,
> dsa_slave_unsync_mc); // DSA is also a hardware device
> 	if (dsa_switch_supports_uc_filtering(ds))
> 		__dev_uc_sync(dev, dsa_slave_sync_uc,
> dsa_slave_unsync_uc); }
> 
> What I'm noticing is that some addresses, for example 33:33:00:00:00:01
> (added by addrconf.c as in6addr_linklocal_allnodes) are synced to the master
> via dev_mc_sync(), but not to hardware by __dev_mc_sync().
> 
> Superficially, this is because dev_mc_sync() -> __hw_addr_sync_one() will
> increase ha->sync_cnt to a non-zero value. Then, when __dev_mc_sync()
> -> __hw_addr_sync_dev() checks ha->sync_cnt, it sees that it has been
> "already synced" (not really), so it doesn't call the "sync" method
> (dsa_slave_sync_mc) for this ha.
> 
> However I don't understand the deep reasons and I am confused by the
> members of struct netdev_hw_addr (synced vs sync_cnt vs refcount).
> I can't tell if this was supposed to work, given that "sync address to another
> device" is conceptually a different kind of sync than "sync address to
> hardware", so I'm a bit surprised that they share the same variables.
> 
> Any ideas?

I hadn't intended it to work this way. The expectation was that __dev_mc_sync would be used by hardware devices whereas dev_mc_sync was used by stacked devices such as vlan or macvlan.

Probably the easiest way to address it is to split things up so that you are using __dev_mc_sync if the switch supports mc filtering and have your dsa_slave_sync/unsync_mc call also push it down to the lower device, and then call dev_mc_sync after that so that if it hasn't already been pushed to the lower device it gets pushed. The assumption is that the lower device and the hardware would be synced in the same way. If we can't go that route we may have to look at implementing a different setup in terms of the reference counting such as what is done in __hw_addr_sync_multiple.