netdev - Re: [PATCH net v3 1/3] net: ethernet: cortina: Drop software checksum and TSO

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: <CANn89iKycKiDXFeJZykA9ERR2exWpkCJtmYFdf+sVViuz==PMQ@mail.gmail.com>
Date: Thu, 21 Dec 2023 09:47:11 +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 1/3] net: ethernet: cortina: Drop software checksum
 and TSO

On Thu, Dec 21, 2023 at 1:02 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.
>
> Drop the size check and the offloading features for now: this
> needs to be fixed up properly.
>
> Suggested-by: Eric Dumazet <edumazet@...gle.com>
> 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, 2 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 78287cfcbf63..ecc247acac39 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
> @@ -1145,7 +1144,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;

I suggested removing all this business with mtu.

A driver is not supposed to double check the upper layer is giving a
too big packet.

I think this code was 'trying' to implement TSO in a very wrong way.

If a patch removes TSO, it should also remove this dead code...

> @@ -1160,22 +1158,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>                 word3 |= mtu;
>         }
>
> -       if (skb->len >= ETH_FRAME_LEN) {
> -               /* Hardware offloaded checksumming isn't working on frames
> -                * bigger than 1514 bytes. A hypothesis about this is that the
> -                * 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;
> -               }
> -               word1 |= TSS_BYPASS_BIT;
> -       } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +       if (skb->ip_summed == CHECKSUM_PARTIAL) {
>                 int tcp = 0;
>
>                 /* We do not switch off the checksumming on non TCP/UDP
>
> --
> 2.34.1
>