| 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: <willemdebruijn.kernel.2f9db40362380@gmail.com>
Date: Tue, 02 Sep 2025 14:22:05 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Richard Gobert <richardbgobert@...il.com>,
netdev@...r.kernel.org
Cc: davem@...emloft.net,
edumazet@...gle.com,
kuba@...nel.org,
pabeni@...hat.com,
horms@...nel.org,
corbet@....net,
saeedm@...dia.com,
tariqt@...dia.com,
mbloch@...dia.com,
leon@...nel.org,
ecree.xilinx@...il.com,
dsahern@...nel.org,
ncardwell@...gle.com,
kuniyu@...gle.com,
shuah@...nel.org,
sdf@...ichev.me,
aleksander.lobakin@...el.com,
florian.fainelli@...adcom.com,
willemdebruijn.kernel@...il.com,
alexander.duyck@...il.com,
linux-kernel@...r.kernel.org,
linux-net-drivers@....com,
Richard Gobert <richardbgobert@...il.com>
Subject: Re: [PATCH net-next v4 3/5] net: gso: restore ids of outer ip headers
correctly
Richard Gobert wrote:
> Currently, NETIF_F_TSO_MANGLEID indicates that the inner-most ID can
> be mangled. Outer IDs can always be mangled.
>
> Make GSO preserve outer IDs by default, with NETIF_F_TSO_MANGLEID allowing
> both inner and outer IDs to be mangled.
>
> This commit also modifies a few drivers that use SKB_GSO_FIXEDID directly.
>
> Signed-off-by: Richard Gobert <richardbgobert@...il.com>
> ---
> .../networking/segmentation-offloads.rst | 9 ++++-----
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 8 ++++++--
> drivers/net/ethernet/sfc/ef100_tx.c | 17 +++++++++++++----
> include/linux/netdevice.h | 9 +++++++--
> include/linux/skbuff.h | 6 +++++-
> net/core/dev.c | 4 +++-
> net/ipv4/af_inet.c | 13 ++++++-------
> net/ipv4/tcp_offload.c | 5 +----
> 8 files changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/networking/segmentation-offloads.rst b/Documentation/networking/segmentation-offloads.rst
> index 085e8fab03fd..d5dccfc6b82b 100644
> --- a/Documentation/networking/segmentation-offloads.rst
> +++ b/Documentation/networking/segmentation-offloads.rst
> @@ -46,7 +46,9 @@ GSO type SKB_GSO_TCP_FIXEDID is specified then we will not increment the IP
> ID and all segments will use the same IP ID. If a device has
> NETIF_F_TSO_MANGLEID set then the IP ID can be ignored when performing TSO
> and we will either increment the IP ID for all frames, or leave it at a
> -static value based on driver preference.
> +static value based on driver preference. For outer headers of encapsulated
> +packets, the device drivers must guarantee that the IPv4 ID field is
> +incremented in the case that a given header does not have the DF bit set.
Please split this into three paragraphs on FIXEDID, FIXED_INNER and
MANGLEID.
Specifically the use of FIXEDID to mean uncapped or outer should be
explicitly mentioned (as discussed previously).
Also, I understood that MANGLEID now means that both inner and outer
IP ID can be mangled. But this comment appears to say otherwise.
Maybe it helps to be more explicit also about behavior without DF.
> Partial Generic Segmentation Offload
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index b8c609d91d11..505c4ce7cef8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
> index e6b6be549581..24971346df00 100644
> --- a/drivers/net/ethernet/sfc/ef100_tx.c
> +++ b/drivers/net/ethernet/sfc/ef100_tx.c
Not sure whether these driver changes need to be separate patches.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index f3a3b761abfb..3d19c888b839 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -5290,13 +5290,18 @@ void skb_warn_bad_offload(const struct sk_buff *skb);
>
> static inline bool net_gso_ok(netdev_features_t features, int gso_type)
> {
> - netdev_features_t feature = (netdev_features_t)gso_type << NETIF_F_GSO_SHIFT;
> + netdev_features_t feature;
> +
> + if (gso_type & (SKB_GSO_TCP_FIXEDID | SKB_GSO_TCP_FIXEDID_INNER))
> + gso_type |= __SKB_GSO_TCP_FIXEDID;
> +
> + feature = ((netdev_features_t)gso_type << NETIF_F_GSO_SHIFT) & NETIF_F_GSO_MASK;
>
> /* check flags correspondence */
> BUILD_BUG_ON(SKB_GSO_TCPV4 != (NETIF_F_TSO >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_DODGY != (NETIF_F_GSO_ROBUST >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_TCP_ECN != (NETIF_F_TSO_ECN >> NETIF_F_GSO_SHIFT));
> - BUILD_BUG_ON(SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
> + BUILD_BUG_ON(__SKB_GSO_TCP_FIXEDID != (NETIF_F_TSO_MANGLEID >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_TCPV6 != (NETIF_F_TSO6 >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_FCOE != (NETIF_F_FSO >> NETIF_F_GSO_SHIFT));
> BUILD_BUG_ON(SKB_GSO_GRE != (NETIF_F_GSO_GRE >> NETIF_F_GSO_SHIFT));
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index ca8be45dd8be..cf95b325f9b4 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -674,7 +674,7 @@ enum {
> /* This indicates the tcp segment has CWR set. */
> SKB_GSO_TCP_ECN = 1 << 2,
>
> - SKB_GSO_TCP_FIXEDID = 1 << 3,
> + __SKB_GSO_TCP_FIXEDID = 1 << 3,
>
> SKB_GSO_TCPV6 = 1 << 4,
>
> @@ -707,6 +707,10 @@ enum {
> SKB_GSO_FRAGLIST = 1 << 18,
>
> SKB_GSO_TCP_ACCECN = 1 << 19,
> +
> + /* These don't correspond with netdev features. */
Can use clarification. Something like
/* These indirectly together map onto the same netdev feature:
* If NETIF_F_TSO_MANGLE is set it may mangle both inner and outer.
*/
> + SKB_GSO_TCP_FIXEDID = 1 << 30,
> + SKB_GSO_TCP_FIXEDID_INNER = 1 << 31,
> };
>
> #if BITS_PER_LONG > 32
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 93a25d87b86b..f57c8dbf307f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3769,7 +3769,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
> features &= ~dev->gso_partial_features;
>
> /* Make sure to clear the IPv4 ID mangling feature if the
> - * IPv4 header has the potential to be fragmented.
> + * IPv4 header has the potential to be fragmented. For
> + * encapsulated packets, the outer headers are guaranteed to
> + * have incrementing IDs if DF is not set.
This is saying that if !DF then both inner and outer must be
incrementing?
Maybe the outer headers are [also] garuanteed to have incrementing IDs.
> */
> if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4) {
> struct iphdr *iph = skb->encapsulation ?
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 76e38092cd8a..fc7a6955fa0a 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1393,14 +1393,13 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
>
> segs = ERR_PTR(-EPROTONOSUPPORT);
>
> - if (!skb->encapsulation || encap) {
> - udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
> - fixedid = !!(skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID);
> + /* fixed ID is invalid if DF bit is not set */
> + fixedid = !!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCP_FIXEDID << encap));
> + if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
> + goto out;
>
> - /* fixed ID is invalid if DF bit is not set */
> - if (fixedid && !(ip_hdr(skb)->frag_off & htons(IP_DF)))
> - goto out;
> - }
> + if (!skb->encapsulation || encap)
> + udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
>
> ops = rcu_dereference(inet_offloads[proto]);
> if (likely(ops && ops->callbacks.gso_segment)) {
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index 1949eede9ec9..e6612bd84d09 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -471,7 +471,6 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
> const u16 offset = NAPI_GRO_CB(skb)->network_offsets[skb->encapsulation];
> const struct iphdr *iph = (struct iphdr *)(skb->data + offset);
> struct tcphdr *th = tcp_hdr(skb);
> - bool is_fixedid;
>
> if (unlikely(NAPI_GRO_CB(skb)->is_flist)) {
> skb_shinfo(skb)->gso_type |= SKB_GSO_FRAGLIST | SKB_GSO_TCPV4;
> @@ -485,10 +484,8 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
> th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
> iph->daddr, 0);
>
> - is_fixedid = (NAPI_GRO_CB(skb)->ip_fixedid >> skb->encapsulation) & 1;
> -
> skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4 |
> - (is_fixedid * SKB_GSO_TCP_FIXEDID);
> + (NAPI_GRO_CB(skb)->ip_fixedid * SKB_GSO_TCP_FIXEDID);
This was only just introduced. And is still needed?
Powered by blists - more mailing lists