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
| ||
|
Date: Sun, 6 Dec 2020 22:19:56 -0500 From: Limin Wang <lwang.nbl@...il.com> To: Jarod Wilson <jarod@...hat.com> Cc: Michal Kubecek <mkubecek@...e.cz>, Netdev <netdev@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org> Subject: Re: LRO: creating vlan subports affects parent port's LRO settings I might be wrong. One potential issue I found in netdev_sync_upper_features() is that it depends on the wanted_feature of upper_dev if (!(upper->wanted_features & feature) && (features & feature)) { netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n", &feature, upper->name); features &= ~feature; } } Suppose a new vlan device will have the LRO bit in its features because lower_dev (real_dev) supports LRO ( assuming with proposed changes above), if the vlan_dev's wanted_feature doesn't include LRO, the NETIF_F_LRO may still be dropped due to this. One could manually use "ethtool -K vlan_dev lro on" to enable LRO in the subport's wanted_features, but that has to be done on all vlan_dev's of the same real_dev. (it is not uncommon that a parent port may have hundreds of vlan subports) Does that mean the vlan_dev->wanted_feature has to include LRO bit at creation time to avoid explicitly setting later on for each and every vlan subports? On Sun, Dec 6, 2020 at 5:58 PM Jarod Wilson <jarod@...hat.com> wrote: > > 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