netdev - Re: [PATCH net v6] net: ethernet: cortina: Drop TSO support

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: <CANn89iK7peY2MbquAmU3QN0hCXYMNoMv672ayjneioT=ts7HNA@mail.gmail.com>
Date: Sat, 6 Jan 2024 10:40:21 +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 v6] net: ethernet: cortina: Drop TSO support

On Sat, Jan 6, 2024 at 1:12 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.
>
> The TSO implementation is bogus: a TSO enabled driver must propagate
> the skb_shinfo(skb)->gso_size value to the TSO engine on the NIC.
>
> Drop the size check and TSO offloading features for now: this
> needs to be fixed up properly.
>
> After this ethernet works fine on Gemini devices with a direct connected
> PHY such as D-Link DNS-313.
>
> Also tested to still be working with a DSA switch using the Gemini
> ethernet as conduit interface.
>
> Link: https://lore.kernel.org/netdev/CANn89iJLfxng1sYL5Zk0mknXpyYQPCp83m3KgD2KJ2_hKCpEUg@mail.gmail.com/
> Suggested-by: Eric Dumazet <edumazet@...gle.com>
> Fixes: d4d0c5b4d279 ("net: ethernet: cortina: Handle large frames")
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> This fix was developed on top of the earlier fixes.
>
> Finding the right solution is hard because the Gemini checksumming
> engine is completely undocumented in the datasheets.
> ---
> Changes in v6:
> - Keep the software checksum on larger frames, just drop the
>   TSO support which is bogus anyway.
> - Drop the heuristics in the second patch. Just dropping TSO
>   makes everything work right.
> - Drop adding the length in word3.
> - Link to v5: https://lore.kernel.org/r/20240102-new-gemini-ethernet-regression-v5-0-cf61ab3aa8cd@linaro.org
>
> Changes in v5:
> - Drop the patch re-implementing eth_header_parse_protocol()
> - Link to v4: https://lore.kernel.org/r/20231222-new-gemini-ethernet-regression-v4-0-a36e71b0f32b@linaro.org
>
> Changes in v4:
> - Properly drop all MTU/TSO muckery in the TX function, the
>   whole approach is bogus.
> - Make the raw etherype retrieveal return __be16, it is the
>   callers job to deal with endianness (as per the pattern
>   from if_vlan.h)
> - Use __vlan_get_protocol() instead of vlan_get_protocol()
> - Only actively bypass the TSS if the frame is over a certain
>   size.
> - Drop comment that no longer applies.
> - Link to v3: https://lore.kernel.org/r/20231221-new-gemini-ethernet-regression-v3-0-a96b4374bfe8@linaro.org
>
> Changes in v3:
> - Fix a whitespace bug in the first patch.
> - Add generic accessors to obtain the raw ethertype of an
>   ethernet frame. VLAN already have the right accessors.
> - Link to v2: https://lore.kernel.org/r/20231216-new-gemini-ethernet-regression-v2-0-64c269413dfa@linaro.org
>
> Changes in v2:
> - Drop the TSO and length checks altogether, this was never
>   working properly.
> - Plan to make a proper TSO implementation in the next kernel
>   cycle.
> - Link to v1: https://lore.kernel.org/r/20231215-new-gemini-ethernet-regression-v1-0-93033544be23@linaro.org
> ---
>  drivers/net/ethernet/cortina/gemini.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 78287cfcbf63..705c3eb19cd3 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,23 +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 */

Okay, but you still kept this wrong comment.

MTU refers to the device MTU, which is very often bigger than the MSS
of the flow.

Hopefully the comment will be removed soon when TSO is properly implemented.

Reviewed-by: Eric Dumazet <edumazet@...gle.com>