Message-ID: <CANn89iJ6pWQxmWqgTBtdCo4xbdbjR31cFZS0yVnoEiTKZ3XT6Q@mail.gmail.com>
Date:   Thu, 16 Jan 2020 08:07:43 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     James Hughes <james.hughes@...pberrypi.org>
Cc:     netdev <netdev@...r.kernel.org>,
        RENARD Pierre-Francois <pfrenard@...il.com>,
        Stefan Wahren <stefan.wahren@...e.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net] net: usb: lan78xx: limit size of local TSO packets

On Thu, Jan 16, 2020 at 8:05 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Thu, Jan 16, 2020 at 6:26 AM James Hughes
> <james.hughes@...pberrypi.org> wrote:
> >
> > Following on from Eric comment below, is this patch suitable for the
> > forwarded packets case? I'm not familiar enough to be sure, and not
> > aware of any way to test it. I've testing ethernet functionality on a
> > Pi3B+ and see no regressions.
> >
> > Content of format patch of the commit, if it seems OK I'll submit a
> > proper patch email.
> >
> > From b7f06bf6298904b106c38b4bac06a6fcebeee47e Mon Sep 17 00:00:00 2001
> > From: James Hughes <james.hughes@...pberrypi.org>
> > Date: Wed, 15 Jan 2020 13:41:54 +0000
> > Subject: [PATCH] net: usb: lan78xx: Add .ndo_features_check
> >
> > As reported by Eric Dumazet, the driver does not handle skb's
> > over a certain size in all possible cases. Most cases have been fixed,
> > this patch should ensure that forwarded TSO packets that are greater than
> > MAX_SINGLE_PACKET_SIZE - header size are software segmented and handled
> > correctly.
> >
> > Signed-off-by: James Hughes <james.hughes@...pberrypi.org>
> > ---
> >  drivers/net/usb/lan78xx.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index bc572b921fe1..3c721dc1fc10 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/mdio.h>
> >  #include <linux/phy.h>
> >  #include <net/ip6_checksum.h>
> > +#include <net/vxlan.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/irq.h>
> > @@ -3733,6 +3734,19 @@ static void lan78xx_tx_timeout(struct net_device *net)
> >   tasklet_schedule(&dev->bh);
> >  }
> >
> > +static netdev_features_t lan78xx_features_check(struct sk_buff *skb,
> > + struct net_device *netdev,
> > + netdev_features_t features)
> > +{
> > + if (skb_shinfo(skb)->gso_size > MAX_SINGLE_PACKET_SIZE - MAX_HEADER)
> > + features &= ~NETIF_F_TSO;
> >
>
> Here gso_size is the payload size of each segment (typically around 1428 bytes)
>
> What you need here is testing the whole packet length (no need to care
> about MAX_HEADER btw)
>
> Also prefer ~NETIF_F_GSO_MASK (otherwise IPv6 will still be broken)
>
> if (skb->len > MAX_SINGLE_PACKET_SIZE)

And it seems you need to account for TX_OVERHEAD (8 additional bytes)
that are added later in the ndo_start_xmit()

if (skb->len + TX_OVERHEAD > MAX_SINGLE_PACKET_SIZE)
....

>     features &= ~NETIF_F_GSO_MASK;
>
>  +
> > + features = vlan_features_check(skb, features);
> > + features = vxlan_features_check(skb, features);
> > +
> > + return features;
> > +}
> > +
> >  static const struct net_device_ops lan78xx_netdev_ops = {
> >   .ndo_open = lan78xx_open,
> >   .ndo_stop = lan78xx_stop,
> > @@ -3746,6 +3760,7 @@ static const struct net_device_ops lan78xx_netdev_ops = {
> >   .ndo_set_features = lan78xx_set_features,
> >   .ndo_vlan_rx_add_vid = lan78xx_vlan_rx_add_vid,
> >   .ndo_vlan_rx_kill_vid = lan78xx_vlan_rx_kill_vid,
> > + .ndo_features_check = lan78xx_features_check,
> >  };
> >
> >  static void lan78xx_stat_monitor(struct timer_list *t)
> > --
> > 2.17.1
> >
> >
> > On Mon, 13 Jan 2020 at 17:27, Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > lan78xx_tx_bh() makes sure to not exceed MAX_SINGLE_PACKET_SIZE
> > > bytes in the aggregated packets it builds, but does
> > > nothing to prevent large GSO packets being submitted.
> > >
> > > Pierre-Francois reported various hangs when/if TSO is enabled.
> > >
> > > For localy generated packets, we can use netif_set_gso_max_size()
> > > to limit the size of TSO packets.
> > >
> > > Note that forwarded packets could still hit the issue,
> > > so a complete fix might require implementing .ndo_features_check
> > > for this driver, forcing a software segmentation if the size
> > > of the TSO packet exceeds MAX_SINGLE_PACKET_SIZE.
> > >
> > > Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > Reported-by: RENARD Pierre-Francois <pfrenard@...il.com>
> > > Tested-by: RENARD Pierre-Francois <pfrenard@...il.com>
> > > Cc: Stefan Wahren <stefan.wahren@...e.com>
> > > Cc: Woojung Huh <woojung.huh@...rochip.com>
> > > Cc: Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>
> > > ---
> > >  drivers/net/usb/lan78xx.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > > index fb4781080d6dec2af22f41c5e064350ea74130b3..75bdfae5f3e20afef3d2880171c7c6e8511546c5 100644
> > > --- a/drivers/net/usb/lan78xx.c
> > > +++ b/drivers/net/usb/lan78xx.c
> > > @@ -3750,6 +3750,7 @@ static int lan78xx_probe(struct usb_interface *intf,
> > >
> > >         /* MTU range: 68 - 9000 */
> > >         netdev->max_mtu = MAX_SINGLE_PACKET_SIZE;
> > > +       netif_set_gso_max_size(netdev, MAX_SINGLE_PACKET_SIZE - MAX_HEADER);
> > >
> > >         dev->ep_blkin = (intf->cur_altsetting)->endpoint + 0;
> > >         dev->ep_blkout = (intf->cur_altsetting)->endpoint + 1;
> > > --
> > > 2.25.0.rc1.283.g88dfdc4193-goog
> > >
> >
> >
> > --
> > James Hughes
> > Principal Software Engineer,
> > Raspberry Pi (Trading) Ltd

Powered by blists - more mailing lists