netdev - Re: [PATCH net 1/2] net: ethernet: cortina: Drop software checksumming

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: <CANn89iJo8ER1kZYB7La1jx5p00FrHxzSLnSsWcMNdj8-iG9_Rw@mail.gmail.com>
Date: Fri, 15 Dec 2023 10:32:30 +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 1/2] net: ethernet: cortina: Drop software checksumming

On Fri, Dec 15, 2023 at 9:49 AM Linus Walleij <linus.walleij@...aro.org> wrote:
>
> The recent change to allow large frames without hardware checksumming
> slotted in software checksumming in the driver if hardware could not
> do it.
>
> This will however upset TSO (TCP Segment Offloading). Typical
> error dumps includes this:
>
> skb len=2961 headroom=222 headlen=66 tailroom=0
> (...)
> WARNING: CPU: 0 PID: 956 at net/core/dev.c:3259 skb_warn_bad_offload+0x7c/0x108
> gemini-ethernet-port: caps=(0x0000010000154813, 0x00002007ffdd7889)
>
> And the packets do not go through.
>
> After investigating I drilled it down to the introduction of the
> software checksumming in the driver.
>
> Since the segmenting of packets will be done by the hardware this
> makes a bit of sense since in that case the hardware also needs to
> be keeping track of the checksumming.
>
> That begs the question why large TCP or UDP packets also have to
> bypass the checksumming (like e.g. ICMP does). If the hardware is
> splitting it into smaller packets per-MTU setting, and checksumming
> them, why is this happening then? I don't know. I know it is needed,
> from tests: the OpenWrt webserver uhttpd starts sending big skb:s (up
> to 2047 bytes, the max MTU) and above 1514 bytes it starts to fail
> and hang unless the bypass bit is set: the frames are not getting
> through.
>
> Keeping the size check but removing the software checksum makes things
> work again. This was probably dubious to introduce in the first place.
>
> Fixes: d4d0c5b4d279 ("net: ethernet: cortina: Handle large frames")
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 78287cfcbf63..255fcffc1579 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1145,7 +1145,6 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>         dma_addr_t mapping;
>         unsigned short mtu;
>         void *buffer;
> -       int ret;
>
>         mtu  = ETH_HLEN;
>         mtu += netdev->mtu;
> @@ -1166,14 +1165,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>                  * checksum buffer is only 1518 bytes, so when the frames get
>                  * bigger they get truncated, or the last few bytes get
>                  * overwritten by the FCS.
> -                *
> -                * Just use software checksumming and bypass on bigger frames.
>                  */
> -               if (skb->ip_summed == CHECKSUM_PARTIAL) {
> -                       ret = skb_checksum_help(skb);
> -                       if (ret)
> -                               return ret;
> -               }

If the hardware does not support checksumming for frames bigger than
ETH_FRAME_LEN,
then an appropriate mitigation would be to have an ndo_features_check() ?

Depending on packet being gso or not, you would have to check skb->len
or shinfo->gso_size

The ndo_features_check could then take a more appropriate action
(forcing GSO, and/or forcing software checksumming)

This driver claims to support TSO, but I do not see it using
shinfo->gso_size, something must be very wrong...

I would simply remove this TSO part, before the driver really supports
TSO properly.

diff --git a/drivers/net/ethernet/cortina/gemini.c
b/drivers/net/ethernet/cortina/gemini.c
index 78287cfcbf6388f01bfab417c264f41f3a1a16f2..829cb69982fe1caf99b56363e9e0565fbaecc82e
100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -79,8 +79,7 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 #define GMAC0_IRQ4_8 (GMAC0_MIB_INT_BIT | GMAC0_RX_OVERRUN_INT_BIT)

 #define GMAC_OFFLOAD_FEATURES (NETIF_F_SG | NETIF_F_IP_CSUM | \
-               NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | \
-               NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6)
+               NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM )

 /**
  * struct gmac_queue_page - page buffer per-page info