netdev - Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

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]

Date: Wed, 20 Dec 2023 00:46:16 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Romain Gantois <romain.gantois@...tlin.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Miquel Raynal <miquel.raynal@...tlin.com>,
	Sylvain Girard <sylvain.girard@...com>,
	Pascal EBERHARD <pascal.eberhard@...com>,
	Richard Tresidder <rtresidd@...ctromag.com.au>,
	netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, stable@...r.kernel.org
Subject: Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

On Tue, Dec 19, 2023 at 05:29:32PM +0100, Maxime Chevallier wrote:
> Hi Linus,
> 
> On Tue, 19 Dec 2023 15:19:45 +0100
> Linus Walleij <linus.walleij@...aro.org> wrote:
> 
> > On Tue, Dec 19, 2023 at 2:07 PM Maxime Chevallier
> > <maxime.chevallier@...tlin.com> wrote:
> > 
> > > So it looks like an acceptable solution would be something along the
> > > lines of what Linus is suggesting here :
> > >
> > > https://lore.kernel.org/netdev/20231216-new-gemini-ethernet-regression-v2-2-64c269413dfa@linaro.org/
> > >
> > > If so, maybe it's worth adding a new helper for that check ?  
> > 
> > Yeah it's a bit annoying when skb->protocol is not == ethertype of buffer.
> > 
> > I can certainly add a helper such as skb_eth_raw_ethertype()
> > to <linux/if_ether.h> that will inspect the actual ethertype in
> > skb->data.
> > 
> > It's the most straight-forward approach.
> 
> Agreed :)

If you rewrite that patch to use skb_vlan_eth_hdr() to get a struct
vlan_ethhdr pointer through which h_vlan_proto and h_vlan_encapsulated_proto
are accessible, I don't see much value in writing that helper. It is
going to beg the question how generic should it be - should it also
treat ETH_P_8021AD, should it treat nested VLANs?

At the end of the day, you are trying to cover in software the cases for
which the hardware engine can perform TX checksum offloading. That is
going to be hardware specific.

> > We could also add something like bool custom_ethertype; to
> > struct sk_buff and set that to true if the tagger adds a custom
> > ethertype. But I don't know how the network developers feel about
> > that.
> 
> I don't think this would be OK, first because sk_buff is pretty
> sensitive when it comes to cache alignment, adding things for this kind
> of use-cases isn't necessarily a good idea. Moreover, populating this
> flag isn't going to be straightforward as well. I guess some ethertype
> would be compatible with checksum engines, while other wouldn't, so
> probably what 'custom_ethertype' means will depend on the MAC driver.
> 
> From my point of view the first approach would indeed be better.

I guess we should first try to answer the questions "what does
skb->protocol represent?" and "does DSA use it correctly?" before
even thinking about adding yet another fuzzy layer on top it.