| 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
| ||
|
Message-ID: <CANn89iL1GK3cqY=bowYu0idtJ3o3FMJh5hkLAY9Lt4RE+Q560Q@mail.gmail.com>
Date: Thu, 9 May 2024 17:09:56 +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>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net: ethernet: cortina: Restore TSO support
On Thu, May 9, 2024 at 5:06 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Thu, May 9, 2024 at 4:49 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Thu, May 9, 2024 at 4:38 PM Linus Walleij <linus.walleij@...aro.org> wrote:
> > >
> > > On Thu, May 9, 2024 at 10:21 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > > > On Thu, May 9, 2024 at 9:48 AM Linus Walleij <linus.walleij@...aro.org> wrote:
> > > > >
> > > > > An earlier commit deleted the TSO support in the Cortina Gemini
> > > > > driver because the driver was confusing gso_size and MTU,
> > > > > probably because what the Linux kernel calls "gso_size" was
> > > > > called "MTU" in the datasheet.
> > > > >
> > > > > Restore the functionality properly reading the gso_size from
> > > > > the skbuff.
> > > > >
> > > > > Tested with iperf3, running a server on a different machine
> > > > > and client on the device with the cortina gemini ethernet:
> > > > >
> > > > > Connecting to host 192.168.1.2, port 5201
> > > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=1c8a
> > > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=1c8a
> > > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=27da
> > > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=0b92
> > > > > 60008000.ethernet-port eth0: segment offloading mss = 05a8 len=2bda
> > > > > (...)
> > > > >
> > > > > It also performs well: ~268 MBit/s.
> > > >
> > > > This does not look very good to me ?
> > >
> > > Oh it's pretty typical. This is an ARMv4 router from 2007, end-of-lifed
> > > in 2015, and it is not meant to be stressed by the software like
> > > this, the idea is that packets get routed by the DSA switch
> > > (RTL8366RB).
> > >
> > > > What number do you have when/if TSO is turned off ?
> > >
> > > Around 187 MBit/s.
> > >
> > > > > + /* Translate to link layer size */
> > > > > + mss += ETH_HLEN;
> > > > > + if (skb->protocol == htons(ETH_P_8021Q))
> > > > > + mss += VLAN_HLEN;
> > > >
> > > > Are you sure this is needed at all ?
> > > > Why not include IP and TCP header sizes as well, if the datasheet
> > > > mentions 'link layer size' ?
> > >
> > > Actually that code is just reusing the mss variable for
> > > skb->len in the case where TSO is not used, so I'll try to
> > > be more elaborate in the code :/
> > >
> > > I guess I actually need to account for it if ->gso_size expand
> > > to the MTU of the interface if I bump it up. But I don't
> > > know if the the TSO code actually does this or if it is
> > > more conservative?
> > >
> > > > To double check, please disable GRO on the receive side and verify the
> > > > packet sizes with tcpdump.
> > > >
> > > > Typically, for MTU=1500, IPv4, and TCP timestamp enabled,
> > > > skb_shinfo(skb)->gso_size is 1448
> > > >
> > > > (Because 20 (ipv4 header) + 32 (tcp header with TS option) + 1448 = 1500)
> > >
> > > I disabled all segment offloading on the receiving side:
> > > ethtool -K enp2s0 gro off gso off tso off
> >
> > >
> > > The iperf3 -c generates segmens like in the commit message:
> > > gemini-ethernet-port 60008000.ethernet-port eth0: segment offloading
> > > mss = 05a8 len=2bda
> > > gemini-ethernet-port 60008000.ethernet-port eth0: segment offloading
> > > mss = 05a8 len=27da
> > > gemini-ethernet-port 60008000.ethernet-port eth0: segment offloading
> > > mss = 05a8 len=0b92
> > >
> > > And 05a8 is 1448 so it is expected.
> > >
> > > tcpdump -e -X enp2s0 gives this on a single segment in a segmented
> > > iperf3 -c transfer:
> > >
> > > 16:24:09.182095 14:d6:4d:a8:3c:4f (oui Unknown) > fc:34:97:01:a0:c6
> > > (oui Unknown), ethertype IPv4 (0x0800), length 1448: OpenWrt.lan.56624
> > > > Fecusia.targus-getdata1: Flags [.], seq 18664:20046, ack 1, win
> > > 4198, options [nop,nop,TS val 2770370491 ecr 3490176978], length 1382
> > > 0x0000: 4500 059a 8ff6 4000 4006 218d c0a8 0188 E.....@.@.......
> > > 0x0010: c0a8 0102 dd30 1451 a701 4f9d e809 8788 .....0.Q..O.....
> > > 0x0020: 8010 1066 0b60 0000 0101 080a a520 7fbb ...f.`..........
> > > (...)
> > > 0x0580: de60 2081 5678 4f8b 31b1 6f85 87fe ae63 .`..VxO.1.o....c
> > > 0x0590: e2ca 8281 fa72 16aa 52e2 .....r..R.
> > >
> > > As can be seen in the header, it is indeed 1448 bytes when arriving
> > > as well, so it seems to work!
> >
> > Not really.
> >
> > Try to disable TSO, and look at the resulting incoming packets, how
> > they are different.
> >
> > If skb_shinfo(skb)->gso_size is 1448, you should receive something like
> >
> > seq 18664:20112 .... length 1448 (this is the payload len at this stage)
> >
> > If you receive instead ".... length 1382" this means you gave to the
> > NIC a 'link layer MSS' too small by 66 bytes.
>
> I would use something like this (modulo GMAC_OFFLOAD_FEATURES changes)
>
> diff --git a/drivers/net/ethernet/cortina/gemini.c
> b/drivers/net/ethernet/cortina/gemini.c
> index 2f98f644b9d7b5e48c4983dd2450a8c10fe04008..88cddd73851215666c26a10da8223c0fa0ac5473
> 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1143,13 +1143,21 @@ static int gmac_map_tx_bufs(struct net_device
> *netdev, struct sk_buff *skb,
> skb_frag_t *skb_frag;
> dma_addr_t mapping;
> void *buffer;
> + u32 mss;
> int ret;
>
> - /* TODO: implement proper TSO using MTU in word3 */
> - word1 = skb->len;
> + word1 = mss = skb->len;
> word3 = SOF_BIT;
>
> - if (skb->len >= ETH_FRAME_LEN) {
> + if (skb_is_gso(skb)) {
> + mss = skb_shinfo(skb)->gso_size + skb_tcp_all_headers(skb);
> + /* skb->len will be all segments in this case */
> + netdev_dbg(netdev, "segment offloading mss = %04x len=%04x\n",
> + mss, skb->len);
> + word1 |= TSS_MTU_ENABLE_BIT;
> + word3 |= mss;
> + }
> + if (mss >= ETH_FRAME_LEN) {
Also this last check makes little sense, because normal MTU/MSS would
trigger this condition.
Forcing software checksumming would make TSO useless.
I suspect this code is there because of the old buggy TSO
implementation of this driver.
Powered by blists - more mailing lists