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

Date: Thu, 21 Dec 2023 09:50:41 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: 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 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.

> +
> +       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
>