netdev - Re: LRO: creating vlan subports affects parent port's LRO settings

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201206164924.baczz7eyxz6czro2@lion.mk-sys.cz>
Date:   Sun, 6 Dec 2020 17:49:24 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Jarod Wilson <jarod@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
        Limin Wang <lwang.nbl@...il.com>
Subject: Re: LRO: creating vlan subports affects parent port's LRO settings

On Sat, Dec 05, 2020 at 07:04:06PM -0500, Jarod Wilson wrote:
> On Mon, Nov 23, 2020 at 7:27 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > On Thu, 19 Nov 2020 20:37:27 -0500 Limin Wang wrote:
> > > Under relatively recent kernels (v4.4+), creating a vlan subport on a
> > > LRO supported parent NIC may turn LRO off on the parent port and
> > > further render its LRO feature practically unchangeable.
> >
> > That does sound like an oversight in commit fd867d51f889 ("net/core:
> > generic support for disabling netdev features down stack").
> >
> > Are you able to create a patch to fix this?
> 
> Something like this, perhaps? Completely untested copy-pasta'd
> theoretical patch:
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8588ade790cb..a5ce372e02ba 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9605,8 +9605,10 @@ int __netdev_update_features(struct net_device *dev)
>         features = netdev_fix_features(dev, features);
> 
>         /* some features can't be enabled if they're off on an upper device */
> -       netdev_for_each_upper_dev_rcu(dev, upper, iter)
> -               features = netdev_sync_upper_features(dev, upper, features);
> +       netdev_for_each_upper_dev_rcu(dev, upper, iter) {
> +               if (netif_is_lag_master(upper) || netif_is_bridge_master(upper))
> +                       features = netdev_sync_upper_features(dev,
> upper, features);
> +       }
> 
>         if (dev->features == features)
>                 goto sync_lower;
> @@ -9633,8 +9635,10 @@ int __netdev_update_features(struct net_device *dev)
>         /* some features must be disabled on lower devices when disabled
>          * on an upper device (think: bonding master or bridge)
>          */
> -       netdev_for_each_lower_dev(dev, lower, iter)
> -               netdev_sync_lower_features(dev, lower, features);
> +       if (netif_is_lag_master(dev) || netif_is_bridge_master(dev)) {
> +               netdev_for_each_lower_dev(dev, lower, iter)
> +                       netdev_sync_lower_features(dev, lower, features);
> +       }
> 
>         if (!err) {
>                 netdev_features_t diff = features ^ dev->features;
> 
> I'm not sure what all other upper devices this excludes besides just
> vlan ports though, so perhaps safer add upper device types to not do
> feature sync on than to choose which ones to do them on?

I'm not sure excluding devices from feature sync is the right way,
whether it's an explicit list types or default. The logic still makes
sense to me. Couldn't we address the issue by either setting features in
NETIF_F_UPPER_DISABLES) by default for a new vlan (and probably macvlan)
device? Or perhaps inheriting their values from the lower device.

Michal