| 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: <aMGwcyKTvmz5StN1@shredder>
Date: Wed, 10 Sep 2025 20:08:03 +0300
From: Ido Schimmel <idosch@...dia.com>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org,
Jay Vosburgh <jv@...sburgh.net>,
Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jiri Pirko <jiri@...nulli.us>, Simon Horman <horms@...nel.org>,
Nikolay Aleksandrov <razor@...ckwall.org>,
Shuah Khan <shuah@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
Kuniyuki Iwashima <kuniyu@...gle.com>,
Ahmed Zaki <ahmed.zaki@...el.com>,
Alexander Lobakin <aleksander.lobakin@...el.com>,
bridge@...ts.linux.dev, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next 1/5] net: add a common function to compute
features from lowers devices
On Wed, Sep 10, 2025 at 04:29:35PM +0200, Sabrina Dubroca wrote:
> 2025-08-31, 18:35:49 +0300, Ido Schimmel wrote:
> > On Fri, Aug 29, 2025 at 09:54:26AM +0000, Hangbin Liu wrote:
> > > Some high level virtual drivers need to compute features from lower
> > > devices. But each has their own implementations and may lost some
> > > feature compute. Let's use one common function to compute features
> > > for kinds of these devices.
> > >
> > > The new helper uses the current bond implementation as the reference
> > > one, as the latter already handles all the relevant aspects: netdev
> > > features, TSO limits and dst retention.
> > >
> > > Suggested-by: Paolo Abeni <pabeni@...hat.com>
> > > Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> > > ---
> > > include/linux/netdevice.h | 19 ++++++++++
> > > net/core/dev.c | 79 +++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 98 insertions(+)
> > >
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index f3a3b761abfb..42742a47f2c6 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -5279,6 +5279,25 @@ int __netdev_update_features(struct net_device *dev);
> > > void netdev_update_features(struct net_device *dev);
> > > void netdev_change_features(struct net_device *dev);
> > >
> > > +/* netdevice features */
> > > +#define VIRTUAL_DEV_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
> > > + NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \
> > > + NETIF_F_GSO_ENCAP_ALL | \
> > > + NETIF_F_HIGHDMA | NETIF_F_LRO)
> > > +
> > > +#define VIRTUAL_DEV_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
> > > + NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE | \
> > > + NETIF_F_GSO_PARTIAL)
> > > +
> > > +#define VIRTUAL_DEV_MPLS_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
> > > + NETIF_F_GSO_SOFTWARE)
> > > +
> > > +#define VIRTUAL_DEV_XFRM_FEATURES (NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
> > > + NETIF_F_GSO_ESP)
> > > +
> > > +#define VIRTUAL_DEV_GSO_PARTIAL_FEATURES (NETIF_F_GSO_ESP)
> > > +void netdev_compute_features_from_lowers(struct net_device *dev);
> > > +
> > > void netif_stacked_transfer_operstate(const struct net_device *rootdev,
> > > struct net_device *dev);
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 1d1650d9ecff..fcad2a9f6b65 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -12577,6 +12577,85 @@ netdev_features_t netdev_increment_features(netdev_features_t all,
> > > }
> > > EXPORT_SYMBOL(netdev_increment_features);
> > >
> > > +/**
> > > + * netdev_compute_features_from_lowers - compute feature from lowers
> > > + * @dev: the upper device
> > > + *
> > > + * Recompute the upper device's feature based on all lower devices.
> > > + */
> > > +void netdev_compute_features_from_lowers(struct net_device *dev)
> > > +{
> > > + unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
> > > + netdev_features_t gso_partial_features = VIRTUAL_DEV_GSO_PARTIAL_FEATURES;
> > > +#ifdef CONFIG_XFRM_OFFLOAD
> > > + netdev_features_t xfrm_features = VIRTUAL_DEV_XFRM_FEATURES;
> > ^ double space (in other places as well)
> >
> > > +#endif
> > > + netdev_features_t mpls_features = VIRTUAL_DEV_MPLS_FEATURES;
> > > + netdev_features_t vlan_features = VIRTUAL_DEV_VLAN_FEATURES;
> > > + netdev_features_t enc_features = VIRTUAL_DEV_ENC_FEATURES;
> > > + unsigned short max_hard_header_len = ETH_HLEN;
>
> Going back to this discussion about hard_header_len:
>
> > hard_header_len is not really a feature, so does not sound like it
> > belongs here. I'm pretty sure it's not needed at all.
> >
> > It was added to the bond driver in 2006 by commit 54ef31371407 ("[PATCH]
> > bonding: Handle large hard_header_len") citing panics with gianfar on
> > xmit. In 2009 commit 93c1285c5d92 ("gianfar: reallocate skb when
> > headroom is not enough for fcb") fixed the gianfar driver to stop
> > assuming that it has enough room to push its custom header. Further,
> > commit bee9e58c9e98 ("gianfar:don't add FCB length to hard_header_len")
> > from 2012 fixed this driver to use needed_headroom instead of
> > hard_header_len.
> >
> > The team driver is also adjusting hard_header_len according to the lower
> > devices, but it most likely copied it from the bond driver. On the other
> > hand, the bridge driver does not mess with hard_header_len and no
> > problems were reported there (that I know of).
> >
> > Might be a good idea to remove this hard_header_len logic from bond and
> > team and instead set their needed_headroom according to the lower device
> > with the highest needed_headroom. Paolo added similar logic in bridge
> > and ovs but the use case is a bit different there.
>
> I'm not convinced removing adapting hard_header_len on bond/team is
> correct, even with old and broken drivers getting fixed years
> ago. hard_header_len will be used on the TX path (for some devices
> like bridge/macvlan via dev_forward_skb() and similar helpers, for IP
> tunnels setting their MTU, and via LL_RESERVED_SPACE).
>
> So I think we should keep setting hard_header_len to the largest of
> all lowers.
It is not clear to me why we are setting hard_header_len to the largest
of all lowers and not needed_headroom. While bond/team allow
non-Ethernet lowers (unlike bridge, which is also adjusted to use this
helper), they do verify that all the lower devices are of the same type.
Shouldn't devices of the same type have the same hardware header length?
On the other hand, needed_headroom can and does vary between devices of
the same type.
Powered by blists - more mailing lists