* [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 related [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 related [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, other threads:[~2020-01-16 21:29 UTC | newest] 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
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.