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: <CAKfmpSdv5onOGk=VtEO1fWxxhaVvi96Tz-wCFcNE5R9cdXNgkg@mail.gmail.com>
Date:   Sat, 5 Dec 2020 19:04:06 -0500
From:   Jarod Wilson <jarod@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Limin Wang <lwang.nbl@...il.com>, Netdev <netdev@...r.kernel.org>
Subject: Re: LRO: creating vlan subports affects parent port's LRO settings

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?

-- 
Jarod Wilson
jarod@...hat.com