| 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: <aO5X7368r8veRe5J@horms.kernel.org>
Date: Tue, 14 Oct 2025 15:02:23 +0100
From: Simon Horman <horms@...nel.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: 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>,
Sabrina Dubroca <sdubroca@...hat.com>,
Jiri Pirko <jiri@...nulli.us>, Ido Schimmel <idosch@...dia.com>,
Shuah Khan <shuah@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
Stanislav Fomichev <stfomichev@...il.com>,
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: [PATCHv4 net-next 1/4] net: add a common function to compute
features from lowers devices
On Tue, Oct 14, 2025 at 08:02:14AM +0000, Hangbin Liu wrote:
...
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a64cef2c537e..54f0e792fbd2 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -12616,6 +12616,101 @@ 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
> + * @update_header: whether to update upper device's header_len/headroom/tailroom
> + *
> + * Recompute the upper device's feature based on all lower devices.
> + */
> +void netdev_compute_features_from_lowers(struct net_device *dev, bool update_header)
> +{
> + 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;
> +#endif
Hi Hangbin,
It would be nice to avoid the #ifdefs in this function.
Could xfrm_features be declared unconditoinally.
And then used behind if(IS_ENABLED(CONFIG_XFRM_OFFLOAD)) conditions?
This would increase compile coverage (and readability IMHO).
> + 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_header_len = ETH_HLEN;
> + unsigned int tso_max_size = TSO_MAX_SIZE;
> + u16 tso_max_segs = TSO_MAX_SEGS;
> + struct net_device *lower_dev;
> + unsigned short max_headroom;
> + unsigned short max_tailroom;
> + struct list_head *iter;
> +
> + mpls_features = netdev_base_features(mpls_features);
> + vlan_features = netdev_base_features(vlan_features);
> + enc_features = netdev_base_features(enc_features);
> +
> + netdev_for_each_lower_dev(dev, lower_dev, iter) {
> + gso_partial_features = netdev_increment_features(gso_partial_features,
> + lower_dev->gso_partial_features,
> + VIRTUAL_DEV_GSO_PARTIAL_FEATURES);
> +
> + vlan_features = netdev_increment_features(vlan_features,
> + lower_dev->vlan_features,
> + VIRTUAL_DEV_VLAN_FEATURES);
> +
> + enc_features = netdev_increment_features(enc_features,
> + lower_dev->hw_enc_features,
> + VIRTUAL_DEV_ENC_FEATURES);
> +
> +#ifdef CONFIG_XFRM_OFFLOAD
> + xfrm_features = netdev_increment_features(xfrm_features,
> + lower_dev->hw_enc_features,
> + VIRTUAL_DEV_XFRM_FEATURES);
> +#endif
> +
> + mpls_features = netdev_increment_features(mpls_features,
> + lower_dev->mpls_features,
> + VIRTUAL_DEV_MPLS_FEATURES);
> +
> + dst_release_flag &= lower_dev->priv_flags;
> +
> + if (update_header) {
> + max_header_len = max_t(unsigned short, max_header_len,
> + lower_dev->hard_header_len);
Both the type of max_header_len and .hard_header_len is unsigned short.
So I think max() can be used here instead of max_t(). Likewise for the
following two lines.
> + max_headroom = max_t(unsigned short, max_headroom,
> + lower_dev->needed_headroom);
Max Headroom [1] is used uninitialised the first time we reach here.
Likewise for max_tailroom below.
[1] https://en.wikipedia.org/wiki/Max_Headroom
Flagged by Smatch.
> + max_tailroom = max_t(unsigned short, max_tailroom,
> + lower_dev->needed_tailroom);
> + }
> +
> + tso_max_size = min(tso_max_size, lower_dev->tso_max_size);
> + tso_max_segs = min(tso_max_segs, lower_dev->tso_max_segs);
> + }
> +
> + dev->gso_partial_features = gso_partial_features;
> + dev->vlan_features = vlan_features;
> + dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
> + NETIF_F_HW_VLAN_CTAG_TX |
> + NETIF_F_HW_VLAN_STAG_TX;
> +#ifdef CONFIG_XFRM_OFFLOAD
> + dev->hw_enc_features |= xfrm_features;
> +#endif
> + dev->mpls_features = mpls_features;
> +
> + dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> + if ((dev->priv_flags & IFF_XMIT_DST_RELEASE_PERM) &&
> + dst_release_flag == (IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM))
> + dev->priv_flags |= IFF_XMIT_DST_RELEASE;
> +
> + if (update_header) {
> + dev->hard_header_len = max_header_len;
> + dev->needed_headroom = max_headroom;
> + dev->needed_tailroom = max_tailroom;
Also, maybe it can't happen in practice. But I think that max_headroom and
max_tailroom will may be used uninitialised here if the previous
'update_header' condition is never reached/met.
Also flagged by Smatch.
> + }
> +
> + netif_set_tso_max_segs(dev, tso_max_segs);
> + netif_set_tso_max_size(dev, tso_max_size);
> +
> + netdev_change_features(dev);
> +}
> +EXPORT_SYMBOL(netdev_compute_features_from_lowers);
> +
...
Powered by blists - more mailing lists