Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net] net: usb: lan78xx: limit size of local TSO packets
@ 2020-01-13 17:27 Eric Dumazet
  2020-01-14 19:05 ` David Miller
  2020-01-16 14:26 ` James Hughes
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-01-13 17:27 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Eric Dumazet, Eric Dumazet, RENARD Pierre-Francois,
	Stefan Wahren, Woojung Huh, Microchip Linux Driver Support

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@google.com>
Reported-by: RENARD Pierre-Francois <pfrenard@gmail.com>
Tested-by: RENARD Pierre-Francois <pfrenard@gmail.com>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Woojung Huh <woojung.huh@microchip.com>
Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: usb: lan78xx: limit size of local TSO packets
  2020-01-13 17:27 [PATCH net] net: usb: lan78xx: limit size of local TSO packets Eric Dumazet
@ 2020-01-14 19:05 ` David Miller
  2020-01-16 14:26 ` James Hughes
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2020-01-14 19:05 UTC (permalink / raw)
  To: edumazet
  Cc: netdev, eric.dumazet, pfrenard, stefan.wahren, woojung.huh,
	UNGLinuxDriver

From: Eric Dumazet <edumazet@google.com>
Date: Mon, 13 Jan 2020 09:27:11 -0800

> 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@google.com>
> Reported-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> Tested-by: RENARD Pierre-Francois <pfrenard@gmail.com>

Applied and queued up for -stable, thanks Eric.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: usb: lan78xx: limit size of local TSO packets
  2020-01-13 17:27 [PATCH net] net: usb: lan78xx: limit size of local TSO packets Eric Dumazet
  2020-01-14 19:05 ` David Miller
@ 2020-01-16 14:26 ` James Hughes
  2020-01-16 16:05   ` Eric Dumazet
  1 sibling, 1 reply; 6+ messages in thread
From: James Hughes @ 2020-01-16 14:26 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, RENARD Pierre-Francois, Stefan Wahren, Woojung Huh,
	Microchip Linux Driver Support

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@raspberrypi.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@raspberrypi.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;
+
+ 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@google.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@google.com>
> Reported-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> Tested-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Woojung Huh <woojung.huh@microchip.com>
> Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: usb: lan78xx: limit size of local TSO packets
  2020-01-16 14:26 ` James Hughes
@ 2020-01-16 16:05   ` Eric Dumazet
  2020-01-16 16:07     ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2020-01-16 16:05 UTC (permalink / raw)
  To: James Hughes
  Cc: netdev, RENARD Pierre-Francois, Stefan Wahren, Woojung Huh,
	Microchip Linux Driver Support

On Thu, Jan 16, 2020 at 6:26 AM James Hughes
<james.hughes@raspberrypi.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@raspberrypi.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@raspberrypi.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)
    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@google.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@google.com>
> > Reported-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> > Tested-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> > Cc: Stefan Wahren <stefan.wahren@i2se.com>
> > Cc: Woojung Huh <woojung.huh@microchip.com>
> > Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: usb: lan78xx: limit size of local TSO packets
  2020-01-16 16:05   ` Eric Dumazet
@ 2020-01-16 16:07     ` Eric Dumazet
  2020-01-16 21:28       ` James Hughes
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2020-01-16 16:07 UTC (permalink / raw)
  To: James Hughes
  Cc: netdev, RENARD Pierre-Francois, Stefan Wahren, Woojung Huh,
	Microchip Linux Driver Support

On Thu, Jan 16, 2020 at 8:05 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jan 16, 2020 at 6:26 AM James Hughes
> <james.hughes@raspberrypi.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@raspberrypi.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@raspberrypi.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@google.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@google.com>
> > > Reported-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> > > Tested-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> > > Cc: Stefan Wahren <stefan.wahren@i2se.com>
> > > Cc: Woojung Huh <woojung.huh@microchip.com>
> > > Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] net: usb: lan78xx: limit size of local TSO packets
  2020-01-16 16:07     ` Eric Dumazet
@ 2020-01-16 21:28       ` James Hughes
  0 siblings, 0 replies; 6+ messages in thread
From: James Hughes @ 2020-01-16 21:28 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, RENARD Pierre-Francois, Stefan Wahren, Woojung Huh,
	Microchip Linux Driver Support

On Thu, 16 Jan 2020 at 16:07, Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jan 16, 2020 at 8:05 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Jan 16, 2020 at 6:26 AM James Hughes
> > <james.hughes@raspberrypi.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@raspberrypi.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@raspberrypi.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;
> >

Got it, thanks! Will redo the patch as indicated and submit correctly tomorrow.

James



> >  +
> > > + 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@google.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@google.com>
> > > > Reported-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> > > > Tested-by: RENARD Pierre-Francois <pfrenard@gmail.com>
> > > > Cc: Stefan Wahren <stefan.wahren@i2se.com>
> > > > Cc: Woojung Huh <woojung.huh@microchip.com>
> > > > Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.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
> > > >

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 17:27 [PATCH net] net: usb: lan78xx: limit size of local TSO packets Eric Dumazet
2020-01-14 19:05 ` David Miller
2020-01-16 14:26 ` James Hughes
2020-01-16 16:05   ` Eric Dumazet
2020-01-16 16:07     ` Eric Dumazet
2020-01-16 21:28       ` James Hughes

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git