| 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
| ||
|
Message-ID: <54CBEE24.8000603@redhat.com>
Date: Fri, 30 Jan 2015 12:48:36 -0800
From: Alexander Duyck <alexander.h.duyck@...hat.com>
To: Fan Du <fan.du@...el.com>, bhutchings@...arflare.com
CC: davem@...emloft.net, netdev@...r.kernel.org,
fengyuleidian0615@...il.com
Subject: Re: [PATCH] net: restore lro after device leave forwarding state
On 01/30/2015 04:33 AM, Fan Du wrote:
> Either detaching a device from bridge or switching a device
> out of FORWARDING state, the original lro feature should
> possibly be enabled for good reason, e.g. hw feature like
> receive side coalescing could come into play.
>
> BEFORE:
> echo 1 > /proc/sys/net/ipv4/conf/ens806f0/forwarding && ethtool -k ens806f0 | grep large
> large-receive-offload: off
>
> echo 0 > /proc/sys/net/ipv4/conf/ens806f0/forwarding && ethtool -k ens806f0 | grep large
> large-receive-offload: off
>
> AFTER:
> echo 1 > /proc/sys/net/ipv4/conf/ens806f0/forwarding && ethtool -k ens806f0 | grep large
> large-receive-offload: off
>
> echo 0 > /proc/sys/net/ipv4/conf/ens806f0/forwarding && ethtool -k ens806f0 | grep large
> large-receive-offload: on
>
> Signed-off-by: Fan Du <fan.du@...el.com>
> Fixes: 0187bdfb0567 ("net: Disable LRO on devices that are forwarding")
First off this isn't a "fix". This is going to likely break more than
it fixes. The main reason why LRO is disabled is because it can cause
more harm then it helps. Since GRO is available we should err on the
side of caution since enabling LRO/RSC can have undesirable side effects
in a number of cases.
As far as the rest of the patch I have serious misgivings as this is
going to be switching on LRO in multiple cases so if the user disables
it they will find it was re-enabled when they likely weren't expecting
it. LRO/RSC has a history of causing issues in a number of different
cases. I'd say if it is off leave it off. If the user really wants to
enable it they can do so via the ethtool interface that is already
provided in the kernel after they have removed the interface from the
bridge, or disabled IP routing.
Leaving it disabled would be consistent with how it is handled in
ixgbe_fix_features when Rx checksum offload is disabled. We just
disable the feature and expect the user to re-enable it when they
re-enable Rx checksum offload. The same logic should apply here. The
user put the hardware in this state, it is the responsibility of the
user to sort out the side effects of disabled features after they revert
to their original state.
> ---
> include/linux/netdevice.h | 1 +
> net/bridge/br_if.c | 1 +
> net/core/dev.c | 24 ++++++++++++++++++++++++
> net/ipv4/devinet.c | 4 ++++
> net/ipv6/addrconf.c | 2 ++
> 5 files changed, 32 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 642d426..904b1a4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2153,6 +2153,7 @@ int dev_alloc_name(struct net_device *dev, const char *name);
> int dev_open(struct net_device *dev);
> int dev_close(struct net_device *dev);
> void dev_disable_lro(struct net_device *dev);
> +void dev_enable_lro(struct net_device *dev);
> int dev_loopback_xmit(struct sk_buff *newskb);
> int dev_queue_xmit(struct sk_buff *skb);
> int dev_queue_xmit_accel(struct sk_buff *skb, void *accel_priv);
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 81e49fb..4236f3a 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -565,6 +565,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
> call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
>
> netdev_update_features(br->dev);
> + dev_enable_lro(dev);
>
> return 0;
> }
Removing an interface from a bridge should not be grounds for turning on
LRO/RSC.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1e325ad..938d7f6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1451,6 +1451,30 @@ void dev_disable_lro(struct net_device *dev)
> }
> EXPORT_SYMBOL(dev_disable_lro);
>
> +/**
> + * dev_enable_lro - enable Large Receive Offload on a device
> + * @dev: device
> + *
> + * Enable Large Receive Offload (LRO) on a net device. Must be
> + * called under RTNL. This is needed if device is not attached
> + * to a bridge, or user change the forwarding state.
> + */
> +void dev_enable_lro(struct net_device *dev)
> +{
> + struct net_device *lower_dev;
> + struct list_head *iter;
> +
> + dev->wanted_features |= NETIF_F_LRO;
> + netdev_update_features(dev);
> +
> + if (unlikely(!(dev->features & NETIF_F_LRO)))
> + netdev_WARN(dev, "failed to enable LRO!\n");
> +
> + netdev_for_each_lower_dev(dev, lower_dev, iter)
> + dev_enable_lro(lower_dev);
> +}
> +EXPORT_SYMBOL(dev_enable_lro);
> +
> static int call_netdevice_notifier(struct notifier_block *nb, unsigned long val,
> struct net_device *dev)
> {
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 214882e..3307196 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1956,6 +1956,8 @@ static void inet_forward_change(struct net *net)
> struct in_device *in_dev;
> if (on)
> dev_disable_lro(dev);
> + else
> + dev_enable_lro(dev);
> rcu_read_lock();
> in_dev = __in_dev_get_rcu(dev);
> if (in_dev) {
Disabling it due to a feature conflict makes sense. Enabling it after
disabling forwarding not so much. If the user disabled it prior to
enabling forwarding it should stay disabled, not be enabled.
> @@ -2047,6 +2049,8 @@ static int devinet_sysctl_forward(struct ctl_table *ctl, int write,
> container_of(cnf, struct in_device, cnf);
> if (*valp)
> dev_disable_lro(idev->dev);
> + else
> + dev_enable_lro(idev->dev);
> inet_netconf_notify_devconf(net,
> NETCONFA_FORWARDING,
> idev->dev->ifindex,
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index f7c8bbe..4c3b54c 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -669,6 +669,8 @@ static void dev_forward_change(struct inet6_dev *idev)
> dev = idev->dev;
> if (idev->cnf.forwarding)
> dev_disable_lro(dev);
> + else
> + dev_enable_lro(dev);
> if (dev->flags & IFF_MULTICAST) {
> if (idev->cnf.forwarding) {
> ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
Same applies here.
If anything this patch seems like more of a feature request then a fix.
It would likely create a desirable effect for some, but have some
undesirable side-effects for other users.
- Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists