linux-kernel - Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW offloading 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: <20230912215523.as4puqamj65dikip@skbuf>
Date:   Wed, 13 Sep 2023 00:55:23 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Lukasz Majewski <lukma@...x.de>
Cc:     Tristram.Ha@...rochip.com, Eric Dumazet <edumazet@...gle.com>,
        Andrew Lunn <andrew@...n.ch>, davem@...emloft.net,
        Woojung Huh <woojung.huh@...rochip.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com,
        Oleksij Rempel <linux@...pel-privat.de>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [[RFC PATCH v4 net-next] 0/2] net: dsa: hsr: Enable HSR HW
 offloading for KSZ9477

On Tue, Sep 12, 2023 at 05:06:41PM +0200, Lukasz Majewski wrote:
> Are we debating about some possible impact on patches which were posted
> and (in a near future?) would be reposted?

We are discussing the ways in which a multi-purpose register should be
programmed. Not "the impact on patches" per se, because Oleksij will
have to adapt no matter what you do, but rather the options that remain
available to him, after the first feature that makes use of the
multi-purpose register makes its way to mainline.

> > > Considering the above - the HSR implementation is safe (to the
> > > extend to the whole DSA subsystem current operation). Am I correct?
> > >  
> > 
> > If we exclude the aforementioned bug (which won't be a bug forever),
> > there still exists the case where the MAC address of a DSA user port
> > can be changed. The HSR driver has a NETDEV_CHANGEADDR handler for
> > this as well, and updates its own MAC address to follow the port. If
> > that is allowed to happen after the offload, currently it will break
> > the offload.
> 
> But then we can have struct ksz_device extended with bitmask -
> hw_mac_addr_ports, which could be set to ports (WoL or HSR) when
> REG_MAC_ADDR_0 is written.
> 
> If WoL would like to alter it after it was written by HSR, then the
> error is presented (printed) to the user and we return.
> 
> The same would be with HSR altering the WoL's MAC in-device setup.
> 
> 
> The HSR or WoL can be added without issues (the first one which is
> accepted).
> 
> Then the second feature would need to implement this check.

This is more or less a rehash of what I proposed as option 2, except for
the fact that you suggest a port mask and I suggest a proper refcount_t.
And the reason why I suggest that is to allow the "WoL+HSR on the same
port" to work. With your proposal, both the HSR and WoL code paths would
set the same bit in hw_mac_addr_ports, which would become problematic
when the time comes to unset it. Not so much when every port calls
refcount_inc() per feature. With WoL+HSR on the same port, the MAC
address would have a refcount of 2, and you could call port_hsr_leave()
and that refcount would just drop to 1 instead of letting go.

There are probably hundreds of implementations of this idea in the
kernel, but the one that comes to my mind is ocelot_mirror_get() +
ocelot_mirror_put(). Again, I need to mention that I know that port
mirroring != HSR - I'm just talking about the technique.

There is one more thing that your reply to my observation fails to
address. Even with this refcount thing, you will still need to add code
to dsa_slave_set_mac_address() which notifies the ksz driver, so that
the driver can refuse MAC address changes, which would break the
offloads. Ack?

In principle it sounds like a plan. It just needs to be implemented.