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  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:   Sun, 6 Dec 2020 17:58:29 -0500
From:   Jarod Wilson <jarod@...hat.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     Netdev <netdev@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>,
        Limin Wang <lwang.nbl@...il.com>
Subject: Re: LRO: creating vlan subports affects parent port's LRO settings

On Sun, Dec 6, 2020 at 11:49 AM Michal Kubecek <mkubecek@...e.cz> wrote:
>
> 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.

Yeah, I think you're right, excluding devices entirely from sync is a
bad idea, it should be only certain features that don't get sync'd for
devices that say they don't want them (i.e., vlan devs and macvlan
devs). I'll do a bit more reading of the code and ponder. I'm not
familiar with the intricacies of NETIF_F_UPPER_DISABLES just yet.

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

Powered by blists - more mailing lists