netdev - Re: [PATCH net] net: usb: lan78xx: limit size of local TSO packets

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]

Date:   Thu, 16 Jan 2020 14:26:31 +0000
From:   James Hughes <james.hughes@...pberrypi.org>
To:     Eric Dumazet <edumazet@...gle.com>
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

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;
+
+ 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