linux-kernel - Re: [PATCH v2 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477

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: <20240619215433.6xpadwyvudtybd72@skbuf>
Date: Thu, 20 Jun 2024 00:54:33 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Lukasz Majewski <lukma@...x.de>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Oleksij Rempel <o.rempel@...gutronix.de>, Tristram.Ha@...rochip.com,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Simon Horman <horms@...nel.org>,
	Dan Carpenter <dan.carpenter@...aro.org>,
	"Ricardo B. Marliere" <ricardo@...liere.net>,
	Casper Andersson <casper.casan@...il.com>,
	linux-kernel@...r.kernel.org,
	Woojung Huh <woojung.huh@...rochip.com>,
	UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v2 net-next] net: dsa: Allow only up to two HSR HW
 offloaded ports for KSZ9477

On Wed, Jun 19, 2024 at 06:48:14PM +0300, Vladimir Oltean wrote:
> You would have to make hsr_portdev_setup() call something else rather
> than netdev_upper_dev_link(), because that eats the "void *upper_info"
> argument when calling __netdev_upper_dev_link(). Possibly create a new
> netdev_upper_dev_link_info() ("link upper dev with extra info")
> function, which only HSR calls with a new info structure. Then, the DSA
> core has access to that and implicitly to the port type, and from there
> on, you can apply the proper restriction.

Yet another comment. TL;DR: I think we should also make HSR use
netdev_master_upper_dev_link() anyway (as a separate change), and thus,
no new API is needed, since that function is able to pass a void
*upper_info already.

Explanation: "master" uppers are called like that because any lower
interface can have only at most one. Bridge, team, bond, etc are all
"master" uppers. So an interface cannot have 2 bridge uppers, or a team
and a bond upper at the same time, etc. Compare this to regular upper
interfaces like VLANs. You can have as many VLAN upper interfaces as you
want (including if you already have a master upper interface).

The point is that, AFAIU, the "master" upper restriction comes from the
use of an rx_handler. You can't chain RX handlers, and a lower netdev
can have a single one, so if the upper netdev uses an RX handler, it'd
better make sure that no one else does, or it does but in a coordinated
manner.

Upper drivers like macvlan do use RX handlers, and are not "master"
uppers nonetheless. This is not a contradiction in terms, because they
take care to set up the RX handler of the lower interface only once.

HSR is not as careful, and uses an rx_handler very plainly, but also
does not mark itself as a "master" upper. An attempt to put a physical
interface under a HSR upper, and then under a second one (without
removing it from the first one), would fail the second time around due
to hsr_portdev_setup() -> netdev_rx_handler_register() ->
netdev_is_rx_handler_busy() -> -EBUSY.

Nor does that configuration appear to make too much sense to me, so you
could mark HSR as master, in order for that second attempt to fail one
step earlier: hsr_portdev_setup() -> netdev_master_upper_dev_link() ->
__netdev_master_upper_dev_get() -> -EBUSY.

With that in place, you also have free access now to the "upper_info"
parameter, for what was discussed earlier to be propagated down.