netdev - Re: [PATCH net v3 3/3] net: ethernet: cortina: Bypass checksumming engine of alien ethertypes

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

Date: Thu, 21 Dec 2023 19:04:57 +0100
From: Simon Horman <horms@...nel.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Hans Ulli Kroll <ulli.kroll@...glemail.com>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net v3 3/3] net: ethernet: cortina: Bypass checksumming
 engine of alien ethertypes

On Thu, Dec 21, 2023 at 09:50:41AM +0100, Eric Dumazet wrote:
> On Thu, Dec 21, 2023 at 1:02 AM Linus Walleij <linus.walleij@...aro.org> wrote:
> >
> > We had workarounds were the ethernet checksumming engine would be bypassed
> > for larger frames, this fixed devices using DSA, but regressed devices
> > where the ethernet was connected directly to a PHY.
> >
> > The devices with a PHY connected directly can't handle large frames
> > either way, with or without bypass. Looking at the size of the frame
> > is probably just wrong.
> >
> > Rework the workaround such that we just bypass the checksumming engine if
> > the ethertype inside the actual frame is something else than 0x0800
> > (IPv4) or 0x86dd (IPv6). These are the only frames the checksumming engine
> > can actually handle. VLAN framing (0x8100) also works fine.
> >
> > We can't inspect skb->protocol because DSA frames will sometimes have a
> > custom ethertype despite skb->protocol is e.g. 0x0800.
> >
> > After this both devices with direct ethernet attached such as D-Link
> > DNS-313 and devices with a DSA switch with a custom ethertype such as
> > D-Link DIR-685 work fine.
> >
> > Fixes: d4d0c5b4d279 ("net: ethernet: cortina: Handle large frames")
> > Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> > ---
> >  drivers/net/ethernet/cortina/gemini.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> > index ecc247acac39..6d153eba8e81 100644
> > --- a/drivers/net/ethernet/cortina/gemini.c
> > +++ b/drivers/net/ethernet/cortina/gemini.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/of_net.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/etherdevice.h>
> > +#include <linux/if_ether.h>
> >  #include <linux/if_vlan.h>
> >  #include <linux/skbuff.h>
> >  #include <linux/phy.h>
> > @@ -1143,6 +1144,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
> >         skb_frag_t *skb_frag;
> >         dma_addr_t mapping;
> >         unsigned short mtu;
> > +       u16 ethertype;
> >         void *buffer;
> >
> >         mtu  = ETH_HLEN;
> > @@ -1158,7 +1160,24 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
> >                 word3 |= mtu;
> >         }
> >
> > -       if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +       /* Dig out the the ethertype actually in the buffer and not what the
> > +        * protocol claims to be. This is the raw data that the checksumming
> > +        * offload engine will have to deal with.
> > +        */
> > +       ethertype = skb_eth_raw_ethertype(skb);
> > +       /* This is the only VLAN type supported by this hardware so check for
> > +        * that: the checksumming engine can handle IP and IPv6 inside 802.1Q.
> > +        */
> > +       if (ethertype == ETH_P_8021Q)
> > +               ethertype = vlan_get_protocol(skb);
> 
> You meant : ethertype = __vlan_get_protocol(skb, ethertype, NULL);
> 
> Otherwise skb->protocol could be something unexpected, according to
> your comments.

Also, __vlan_get_protocol() deals with big endian Ether Types,
so I think you need something like:

	ethertype = be16_to_cpu(__vlan_get_protocol(skb, cpu_to_be16(ethertype),
						    NULL));

Which is a bit ugly, so perhaps it is nicer to switch
things around so ethertype is __be16. And perhaps the
return value of skb_eth_raw_ethertype is __be16 too.

> 
> > +
> > +       if (ethertype != ETH_P_IP && ethertype != ETH_P_IPV6) {
> > +               /* Hardware offloaded checksumming isn't working on non-IP frames.
> > +                * This happens for example on some DSA switches using a custom
> > +                * ethertype. Just bypass the engine for those.
> > +                */
> > +               word1 |= TSS_BYPASS_BIT;
> > +       } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >                 int tcp = 0;
> >
> >                 /* We do not switch off the checksumming on non TCP/UDP
> >
> > --
> > 2.34.1
> >
>