netdev - Re: [PATCH net-next v3 2/5] net: ethernet: cortina: Use TSO also on common TCP

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: <CANn89iJ9t5MYEBh_ztib22Ozz4t50ADekbqShpqQSJb9r5x52g@mail.gmail.com>
Date: Tue, 14 May 2024 11:16:04 +0200
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>, Andrew Lunn <andrew@...n.ch>, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/5] net: ethernet: cortina: Use TSO also on
 common TCP

On Mon, May 13, 2024 at 3:38 PM Linus Walleij <linus.walleij@...aro.org> wrote:
>
> It is possible to push the segment offloader to also
> process non-segmented frames: just pass the skb->len
> or desired MSS to the offloader and it will handle them.
>
> This is especially good if the user sets up the MTU
> and the frames get big, because the checksumming engine
> cannot handle any frames bigger than 1518 bytes, so
> segmenting them all to be at max that will be helpful
> for the hardware, which only need to quirk odd frames
> such as big UDP ping packets.
>
> The vendor driver always uses the TSO like this, and
> the driver seems more stable after this, so apparently
> the hardware may have been engineered to always use
> the TSO on anything it can handle.

We do not copy what vendor drivers do.

Please send the first patch as a standalone one, instead of sending a
series with controversial changes.

>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index b2ac9dfe1aae..3ba579550cdd 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1148,6 +1148,7 @@ 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;
> +       bool tcp = false;
>         void *buffer;
>         u16 mss;
>         int ret;
> @@ -1155,6 +1156,13 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>         word1 = skb->len;
>         word3 = SOF_BIT;
>
> +       /* Determine if we are doing TCP */
> +       if (skb->protocol == htons(ETH_P_IP))
> +               tcp = (ip_hdr(skb)->protocol == IPPROTO_TCP);
> +       else
> +               /* IPv6 */
> +               tcp = (ipv6_hdr(skb)->nexthdr == IPPROTO_TCP);
> +
>         mss = skb_shinfo(skb)->gso_size;
>         if (mss) {
>                 /* This means we are dealing with TCP and skb->len is the
> @@ -1167,6 +1175,20 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>                            mss, skb->len);
>                 word1 |= TSS_MTU_ENABLE_BIT;
>                 word3 |= mss;
> +       } else if (tcp) {

Please do not do this.

Let the packet be dropped as it should.

Can you share how this path is hit and how you tested it ?


> +               /* Even if we are not using TSO, use the segment offloader
> +                * for transferring the TCP frame: the TSO engine will deal
> +                * with chopping up frames that exceed ETH_DATA_LEN which
> +                * the checksumming engine cannot handle (see below) into
> +                * manageable chunks. It flawlessly deals with quite big
> +                * frames and frames containing custom DSA EtherTypes.
> +                */
> +               mss = netdev->mtu + skb_tcp_all_headers(skb);

This is broken anyway.   if netdev->mtu is 1500, and
skb_tcp_all_headers(skb) is 94,
we would ask the NIC to send packets of 1594 bytes...

> +               mss = min(mss, skb->len);
> +               netdev_dbg(netdev, "botched TSO len %04x mtu %04x mss %04x\n",
> +                          skb->len, netdev->mtu, mss);
> +               word1 |= TSS_MTU_ENABLE_BIT;
> +               word3 |= mss;
>         } else 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
> @@ -1185,21 +1207,16 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
>         }
>
>         if (skb->ip_summed == CHECKSUM_PARTIAL) {
> -               int tcp = 0;
> -
>                 /* We do not switch off the checksumming on non TCP/UDP
>                  * frames: as is shown from tests, the checksumming engine
>                  * is smart enough to see that a frame is not actually TCP
>                  * or UDP and then just pass it through without any changes
>                  * to the frame.
>                  */
> -               if (skb->protocol == htons(ETH_P_IP)) {
> +               if (skb->protocol == htons(ETH_P_IP))
>                         word1 |= TSS_IP_CHKSUM_BIT;
> -                       tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
> -               } else { /* IPv6 */
> +               else
>                         word1 |= TSS_IPV6_ENABLE_BIT;
> -                       tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
> -               }
>
>                 word1 |= tcp ? TSS_TCP_CHKSUM_BIT : TSS_UDP_CHKSUM_BIT;
>         }
>
> --
> 2.45.0
>