[PATCH 0/2] Disable forwarding of LRO skbs [2nd try]

[PATCH 0/2] Disable forwarding of LRO skbs [2nd try]

[Ben Hutchings]

[Re-sent with the correct Message-ID, previously eaten by mutt.]

Large Receive Offload (LRO) destroys packet headers that should be
preserved when forwarding.  Currently it also triggers a BUG() or WARN()
in skb_gso_segment().  We should disable it wherever forwarding is
enabled, and discard LRO skbs with a warning if it is turned back on.

Since the previous version of these patches, I have:
- Tightened the test for LRO'd skbs, so it should not catch skbs from
  VM guests that want GSO
- Made the test an inline function, with the warning a separate extern
  function
- Rebased against net-next-2.6

Ben.

Ben Hutchings (2):
  net: Disable LRO on devices that are forwarding
  net: Discard and warn about LRO'd skbs received for forwarding

 include/linux/netdevice.h |    1 +
 include/linux/skbuff.h    |   14 ++++++++++++++
 net/bridge/br_forward.c   |    2 +-
 net/bridge/br_if.c        |    1 +
 net/core/dev.c            |   24 ++++++++++++++++++++++++
 net/core/skbuff.c         |    8 ++++++++
 net/ipv4/devinet.c        |   21 ++++++++++++++++-----
 net/ipv4/ip_forward.c     |    3 +++
 net/ipv6/addrconf.c       |    6 ++++++
 net/ipv6/ip6_output.c     |    3 +++
 10 files changed, 77 insertions(+), 6 deletions(-)

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

[PATCH 1/2] net: Disable LRO on devices that are forwarding

[Ben Hutchings]

Large Receive Offload (LRO) is only appropriate for packets that are
destined for the host, and should be disabled if received packets may be
forwarded.  It can also confuse the GSO on output.

Add dev_disable_lro() function which uses the appropriate ethtool ops to
disable LRO if enabled.

Add calls to dev_disable_lro() in br_add_if() and functions that enable
IPv4 and IPv6 forwarding.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/netdevice.h |    1 +
 net/bridge/br_if.c        |    1 +
 net/core/dev.c            |   24 ++++++++++++++++++++++++
 net/ipv4/devinet.c        |   21 ++++++++++++++++-----
 net/ipv6/addrconf.c       |    6 ++++++
 5 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 45dce2b..1304ad2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -886,6 +886,7 @@ extern struct net_device	*__dev_get_by_name(struct net *net, const char *name);
 extern int		dev_alloc_name(struct net_device *dev, const char *name);
 extern int		dev_open(struct net_device *dev);
 extern int		dev_close(struct net_device *dev);
+extern void		dev_disable_lro(struct net_device *dev);
 extern int		dev_queue_xmit(struct sk_buff *skb);
 extern int		register_netdevice(struct net_device *dev);
 extern void		unregister_netdevice(struct net_device *dev);
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 143c954..832a561 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -387,6 +387,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 		goto err2;
 
 	rcu_assign_pointer(dev->br_port, p);
+	dev_disable_lro(dev);
 	dev_set_promiscuity(dev, 1);
 
 	list_add_rcu(&p->list, &br->port_list);
diff --git a/net/core/dev.c b/net/core/dev.c
index a495f71..f6944ec 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -90,6 +90,7 @@
 #include <linux/if_ether.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/notifier.h>
 #include <linux/skbuff.h>
 #include <net/net_namespace.h>
@@ -1123,6 +1124,29 @@ int dev_close(struct net_device *dev)
 }
 
 
+/**
+ *	dev_disable_lro - disable Large Receive Offload on a device
+ *	@dev: device
+ *
+ *	Disable Large Receive Offload (LRO) on a net device.  Must be
+ *	called under RTNL.  This is needed if received packets may be
+ *	forwarded to another interface.
+ */
+void dev_disable_lro(struct net_device *dev)
+{
+	if (dev->ethtool_ops && dev->ethtool_ops->get_flags &&
+	    dev->ethtool_ops->set_flags) {
+		u32 flags = dev->ethtool_ops->get_flags(dev);
+		if (flags & ETH_FLAG_LRO) {
+			flags &= ~ETH_FLAG_LRO;
+			dev->ethtool_ops->set_flags(dev, flags);
+		}
+	}
+	WARN_ON(dev->features & NETIF_F_LRO);
+}
+EXPORT_SYMBOL(dev_disable_lro);
+
+
 static int dev_boot_phase = 1;
 
 /*
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index f8c0b0a..9de2514 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -168,6 +168,8 @@ static struct in_device *inetdev_init(struct net_device *dev)
 	in_dev->dev = dev;
 	if ((in_dev->arp_parms = neigh_parms_alloc(dev, &arp_tbl)) == NULL)
 		goto out_kfree;
+	if (IPV4_DEVCONF(in_dev->cnf, FORWARDING))
+		dev_disable_lro(dev);
 	/* Reference in_dev->dev */
 	dev_hold(dev);
 	/* Account for reference dev->ip_ptr (below) */
@@ -1241,6 +1243,8 @@ static void inet_forward_change(struct net *net)
 	read_lock(&dev_base_lock);
 	for_each_netdev(net, dev) {
 		struct in_device *in_dev;
+		if (on)
+			dev_disable_lro(dev);
 		rcu_read_lock();
 		in_dev = __in_dev_get_rcu(dev);
 		if (in_dev)
@@ -1248,8 +1252,6 @@ static void inet_forward_change(struct net *net)
 		rcu_read_unlock();
 	}
 	read_unlock(&dev_base_lock);
-
-	rt_cache_flush(0);
 }
 
 static int devinet_conf_proc(ctl_table *ctl, int write,
@@ -1335,10 +1337,19 @@ static int devinet_sysctl_forward(ctl_table *ctl, int write,
 	if (write && *valp != val) {
 		struct net *net = ctl->extra2;
 
-		if (valp == &IPV4_DEVCONF_ALL(net, FORWARDING))
-			inet_forward_change(net);
-		else if (valp != &IPV4_DEVCONF_DFLT(net, FORWARDING))
+		if (valp != &IPV4_DEVCONF_DFLT(net, FORWARDING)) {
+			rtnl_lock();
+			if (valp == &IPV4_DEVCONF_ALL(net, FORWARDING)) {
+				inet_forward_change(net);
+			} else if (*valp) {
+				struct ipv4_devconf *cnf = ctl->extra1;
+				struct in_device *idev =
+					container_of(cnf, struct in_device, cnf);
+				dev_disable_lro(idev->dev);
+			}
+			rtnl_unlock();
 			rt_cache_flush(0);
+		}
 	}
 
 	return ret;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 9be6be3..84127d8 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -348,6 +348,8 @@ static struct inet6_dev * ipv6_add_dev(struct net_device *dev)
 		kfree(ndev);
 		return NULL;
 	}
+	if (ndev->cnf.forwarding)
+		dev_disable_lro(dev);
 	/* We refer to the device */
 	dev_hold(dev);
 
@@ -442,6 +444,8 @@ static void dev_forward_change(struct inet6_dev *idev)
 	if (!idev)
 		return;
 	dev = idev->dev;
+	if (idev->cnf.forwarding)
+		dev_disable_lro(dev);
 	if (dev && (dev->flags & IFF_MULTICAST)) {
 		if (idev->cnf.forwarding)
 			ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
@@ -487,12 +491,14 @@ static void addrconf_fixup_forwarding(struct ctl_table *table, int *p, int old)
 	if (p == &net->ipv6.devconf_dflt->forwarding)
 		return;
 
+	rtnl_lock();
 	if (p == &net->ipv6.devconf_all->forwarding) {
 		__s32 newf = net->ipv6.devconf_all->forwarding;
 		net->ipv6.devconf_dflt->forwarding = newf;
 		addrconf_forward_change(net, newf);
 	} else if ((!*p) ^ (!old))
 		dev_forward_change((struct inet6_dev *)table->extra1);
+	rtnl_unlock();
 
 	if (*p)
 		rt6_purge_dflt_routers(net);

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 1/2] net: Disable LRO on devices that are forwarding

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 19 Jun 2008 19:07:36 +0100

> Large Receive Offload (LRO) is only appropriate for packets that are
> destined for the host, and should be disabled if received packets may be
> forwarded.  It can also confuse the GSO on output.
> 
> Add dev_disable_lro() function which uses the appropriate ethtool ops to
> disable LRO if enabled.
> 
> Add calls to dev_disable_lro() in br_add_if() and functions that enable
> IPv4 and IPv6 forwarding.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Looks good, applied to net-next-2.6

Re: [PATCH 1/2] net: Disable LRO on devices that are forwarding

[Ben Hutchings]

Ben Hutchings wrote:
[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a495f71..f6944ec 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[...]
> @@ -1123,6 +1124,29 @@ int dev_close(struct net_device *dev)
>  }
>  
>  
> +/**
> + *	dev_disable_lro - disable Large Receive Offload on a device
> + *	@dev: device
> + *
> + *	Disable Large Receive Offload (LRO) on a net device.  Must be
> + *	called under RTNL.  This is needed if received packets may be
> + *	forwarded to another interface.
> + */
> +void dev_disable_lro(struct net_device *dev)
> +{
> +	if (dev->ethtool_ops && dev->ethtool_ops->get_flags &&
> +	    dev->ethtool_ops->set_flags) {
> +		u32 flags = dev->ethtool_ops->get_flags(dev);
> +		if (flags & ETH_FLAG_LRO) {
> +			flags &= ~ETH_FLAG_LRO;
> +			dev->ethtool_ops->set_flags(dev, flags);
> +		}
> +	}
> +	WARN_ON(dev->features & NETIF_F_LRO);
> +}
> +EXPORT_SYMBOL(dev_disable_lro);
[...]

Of course, this doesn't work where the device that's about to start
forwarding is a VLAN device, bonding device or other device depending on
the physical device which is doing LRO.

It might be worth separating out the offload control ops from ethtool ops
since they could usefully be applied to other device types... particularly
here.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 1/2] net: Disable LRO on devices that are forwarding

[Patrick McHardy]

Ben Hutchings wrote:
> Ben Hutchings wrote:
> [...]
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index a495f71..f6944ec 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
> [...]
>> @@ -1123,6 +1124,29 @@ int dev_close(struct net_device *dev)
>>  }
>>  
>>  
>> +/**
>> + *	dev_disable_lro - disable Large Receive Offload on a device
>> + *	@dev: device
>> + *
>> + *	Disable Large Receive Offload (LRO) on a net device.  Must be
>> + *	called under RTNL.  This is needed if received packets may be
>> + *	forwarded to another interface.
>> + */
>> +void dev_disable_lro(struct net_device *dev)
>> +{
>> +	if (dev->ethtool_ops && dev->ethtool_ops->get_flags &&
>> +	    dev->ethtool_ops->set_flags) {
>> +		u32 flags = dev->ethtool_ops->get_flags(dev);
>> +		if (flags & ETH_FLAG_LRO) {
>> +			flags &= ~ETH_FLAG_LRO;
>> +			dev->ethtool_ops->set_flags(dev, flags);
>> +		}
>> +	}
>> +	WARN_ON(dev->features & NETIF_F_LRO);
>> +}
>> +EXPORT_SYMBOL(dev_disable_lro);
> [...]
> 
> Of course, this doesn't work where the device that's about to start
> forwarding is a VLAN device, bonding device or other device depending on
> the physical device which is doing LRO.
> 
> It might be worth separating out the offload control ops from ethtool ops
> since they could usefully be applied to other device types... particularly
> here.


You can propagate it to the real device. I have a patch queued
to add ethtool support to vlan, I can send it to Dave this
weekend. I guess for bonding etc. that would also be useful
for disabling offloading/checksumming/... globally or just
querying the state.

Re: [PATCH 1/2] net: Disable LRO on devices that are forwarding

[Andi Kleen]

Ben Hutchings <bhutchings@solarflare.com> writes:

> Large Receive Offload (LRO) is only appropriate for packets that are
> destined for the host, and should be disabled if received packets may be
> forwarded.  It can also confuse the GSO on output.
>
> Add dev_disable_lro() function which uses the appropriate ethtool ops to
> disable LRO if enabled.
>
> Add calls to dev_disable_lro() in br_add_if() and functions that enable
> IPv4 and IPv6 forwarding.

It might be a good idea to add a printk so that the user knows why
the device suddenly goes slower.

-Andi

[PATCH 2/2] net: Discard and warn about LRO'd skbs received for forwarding

[Ben Hutchings]

Add skb_warn_if_lro() to test whether an skb was received with LRO and
warn if so.

Change br_forward(), ip_forward() and ip6_forward() to call it) and
discard the skb if it returns true.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/skbuff.h  |   14 ++++++++++++++
 net/bridge/br_forward.c |    2 +-
 net/core/skbuff.c       |    8 ++++++++
 net/ipv4/ip_forward.c   |    3 +++
 net/ipv6/ip6_output.c   |    3 +++
 5 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 299ec4b..2220b9e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1702,6 +1702,20 @@ static inline int skb_is_gso_v6(const struct sk_buff *skb)
 	return skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6;
 }
 
+extern void __skb_warn_lro_forwarding(const struct sk_buff *skb);
+
+static inline bool skb_warn_if_lro(const struct sk_buff *skb)
+{
+	/* LRO sets gso_size but not gso_type, whereas if GSO is really
+	 * wanted then gso_type will be set. */
+	struct skb_shared_info *shinfo = skb_shinfo(skb);
+	if (shinfo->gso_size != 0 && unlikely(shinfo->gso_type == 0)) {
+		__skb_warn_lro_forwarding(skb);
+		return true;
+	}
+	return false;
+}
+
 static inline void skb_forward_csum(struct sk_buff *skb)
 {
 	/* Unfortunately we don't support this one.  Any brave souls? */
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 5126457..bdd9cce 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -89,7 +89,7 @@ void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
 /* called with rcu_read_lock */
 void br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
 {
-	if (should_deliver(to, skb)) {
+	if (!skb_warn_if_lro(skb) && should_deliver(to, skb)) {
 		__br_forward(to, skb);
 		return;
 	}
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3e18f85..2df012b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2583,6 +2583,13 @@ bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off)
 	return true;
 }
 
+void __skb_warn_lro_forwarding(const struct sk_buff *skb)
+{
+	if (net_ratelimit())
+		pr_warning("%s: received packets cannot be forwarded"
+			   " while LRO is enabled\n", skb->dev->name);
+}
+
 EXPORT_SYMBOL(___pskb_trim);
 EXPORT_SYMBOL(__kfree_skb);
 EXPORT_SYMBOL(kfree_skb);
@@ -2616,6 +2623,7 @@ EXPORT_SYMBOL(skb_seq_read);
 EXPORT_SYMBOL(skb_abort_seq_read);
 EXPORT_SYMBOL(skb_find_text);
 EXPORT_SYMBOL(skb_append_datato_frags);
+EXPORT_SYMBOL(__skb_warn_lro_forwarding);
 
 EXPORT_SYMBOL_GPL(skb_to_sgvec);
 EXPORT_SYMBOL_GPL(skb_cow_data);
diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 37d36a3..da14725 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -56,6 +56,9 @@ int ip_forward(struct sk_buff *skb)
 	struct rtable *rt;	/* Route we use */
 	struct ip_options * opt	= &(IPCB(skb)->opt);
 
+	if (skb_warn_if_lro(skb))
+		goto drop;
+
 	if (!xfrm4_policy_check(NULL, XFRM_POLICY_FWD, skb))
 		goto drop;
 
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 40a2813..fd7cd1b 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -407,6 +407,9 @@ int ip6_forward(struct sk_buff *skb)
 	if (ipv6_devconf.forwarding == 0)
 		goto error;
 
+	if (skb_warn_if_lro(skb))
+		goto drop;
+
 	if (!xfrm6_policy_check(NULL, XFRM_POLICY_FWD, skb)) {
 		IP6_INC_STATS(ip6_dst_idev(dst), IPSTATS_MIB_INDISCARDS);
 		goto drop;

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 2/2] net: Discard and warn about LRO'd skbs received for forwarding

[Stephen Hemminger]

On Thu, 19 Jun 2008 19:08:34 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> Add skb_warn_if_lro() to test whether an skb was received with LRO and
> warn if so.
> 
> Change br_forward(), ip_forward() and ip6_forward() to call it) and
> discard the skb if it returns true.

Actually shouldn't ip forwarding work because of refragmentation?

RE: [PATCH 2/2] net: Discard and warn about LRO'd skbs received for forwarding

[Brandeburg, Jesse]

Stephen Hemminger wrote:
> On Thu, 19 Jun 2008 19:08:34 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
>> Add skb_warn_if_lro() to test whether an skb was received with LRO
>> and warn if so. 
>> 
>> Change br_forward(), ip_forward() and ip6_forward() to call it) and
>> discard the skb if it returns true.
> 
> Actually shouldn't ip forwarding work because of refragmentation?

Yes, it should, but in practice it runs out of fragment handling space
and throughput drops to 10kB/s or something horrible like that, AFAIR.

Re: [PATCH 2/2] net: Discard and warn about LRO'd skbs received for forwarding

[Ben Hutchings]

Stephen Hemminger wrote:
> On Thu, 19 Jun 2008 19:08:34 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > Add skb_warn_if_lro() to test whether an skb was received with LRO and
> > warn if so.
> > 
> > Change br_forward(), ip_forward() and ip6_forward() to call it) and
> > discard the skb if it returns true.
> 
> Actually shouldn't ip forwarding work because of refragmentation?

At the moment, LRO'd skbs have gso_size != 0 and ip_summed ==
CHECKSUM_UNNECESSARY (typically), and this results in taking the
GSO path on output and hitting a BUG() or WARN() in skb_segment_gso().
If you want to avoid this without dropping packets, you can avoid the
condition by either:

- setting gso_size = 0 - should result in fragmentation
- setting ip_summed = CHECKSUM_NONE - should result in GSO
  (maybe skb_forward_csum() could do that)

But I didn't try either of these because I think it's fundamentally
dodgy to apply LRO to forwarded packets.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 2/2] net: Discard and warn about LRO'd skbs received for forwarding

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 19 Jun 2008 19:08:34 +0100

> Add skb_warn_if_lro() to test whether an skb was received with LRO and
> warn if so.
> 
> Change br_forward(), ip_forward() and ip6_forward() to call it) and
> discard the skb if it returns true.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Also applied to net-next-2.6

Thanks again Ben.

Re: [PATCH 0/2] Disable forwarding of LRO skbs [2nd try]

[Stephen Hemminger]

On Thu, 19 Jun 2008 19:44:56 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> [Re-sent with the correct Message-ID, previously eaten by mutt.]
> 
> Large Receive Offload (LRO) destroys packet headers that should be
> preserved when forwarding.  Currently it also triggers a BUG() or WARN()
> in skb_gso_segment().  We should disable it wherever forwarding is
> enabled, and discard LRO skbs with a warning if it is turned back on.
> 
> Since the previous version of these patches, I have:
> - Tightened the test for LRO'd skbs, so it should not catch skbs from
>   VM guests that want GSO
> - Made the test an inline function, with the warning a separate extern
>   function
> - Rebased against net-next-2.6
> 

I like that it covers both cases and looks clean. If anyone ever gets
around to rewriting the network device driver chapter in the linux
kernel book(s) this should be covered in more detail.