netdev - Re: [PATCH net v5 1/2] 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]

Date: Fri, 5 Jan 2024 15:40:01 +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>, Vladimir Oltean <olteanv@...il.com>, 
	Household Cang <canghousehold@....com>, Romain Gantois <romain.gantois@...tlin.com>, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net v5 1/2] net: ethernet: cortina: Drop software checksum
 and TSO

On Tue, Jan 2, 2024 at 9:34 PM 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 | 35 ++++-------------------------------
>  1 file changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 78287cfcbf63..5e399c6e095b 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
> @@ -1143,39 +1142,13 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>         struct gmac_txdesc *txd;
>         skb_frag_t *skb_frag;
>         dma_addr_t mapping;
> -       unsigned short mtu;
>         void *buffer;
> -       int ret;
> -
> -       mtu  = ETH_HLEN;
> -       mtu += netdev->mtu;
> -       if (skb->protocol == htons(ETH_P_8021Q))
> -               mtu += VLAN_HLEN;
>
> +       /* TODO: implement proper TSO using MTU in word3 */

I would not use MTU in this comment, but gso_size (or flow MSS).

>         word1 = skb->len;
> -       word3 = SOF_BIT;
> -
> -       if (word1 > mtu) {
> -               word1 |= TSS_MTU_ENABLE_BIT;
> -               word3 |= mtu;
> -       }
> +       word3 = SOF_BIT | skb->len;

Probably word3 could be left with SOF_BIT ?
I am guessing the 'length' would only be used by the NIC if TSO is requested.

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