| 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: <9be872a7-1523-48ef-86a4-dad899fc0a03@gmail.com>
Date: Mon, 18 Aug 2025 13:46:53 +0200
From: Richard Gobert <richardbgobert@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, horms@...nel.org, corbet@....net, shenjian15@...wei.com,
salil.mehta@...wei.com, shaojijie@...wei.com, andrew+netdev@...n.ch,
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, ahmed.zaki@...el.com,
aleksander.lobakin@...el.com, linux-kernel@...r.kernel.org,
linux-net-drivers@....com
Subject: Re: [PATCH net-next 3/5] net: gso: restore ids of outer ip headers
correctly
Willem de Bruijn wrote:
> Willem de Bruijn wrote:
>> 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. In the future, we could add
>>> NETIF_F_TSO_MANGLEID_{INNER,OUTER} to provide more granular control to
>>> drivers.
>>>
>>> This commit also modifies a few drivers that use SKB_GSO_FIXEDID directly.
>>>
>>> Signed-off-by: Richard Gobert <richardbgobert@...il.com>
>>> ---
>>> Documentation/networking/segmentation-offloads.rst | 4 ++--
>>> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 2 +-
>>> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 8 ++++++--
>>> drivers/net/ethernet/sfc/ef100_tx.c | 14 ++++++++------
>>> include/linux/netdevice.h | 9 +++++++--
>>> include/linux/skbuff.h | 6 +++++-
>>> net/core/dev.c | 7 +++----
>>> net/ipv4/af_inet.c | 13 ++++++-------
>>> net/ipv4/tcp_offload.c | 4 +---
>>> 9 files changed, 39 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/Documentation/networking/segmentation-offloads.rst b/Documentation/networking/segmentation-offloads.rst
>>> index 085e8fab03fd..21c759b81f4e 100644
>>> --- a/Documentation/networking/segmentation-offloads.rst
>>> +++ b/Documentation/networking/segmentation-offloads.rst
>>> @@ -42,8 +42,8 @@ also point to the TCP header of the packet.
>>>
>>> For IPv4 segmentation we support one of two types in terms of the IP ID.
>>> The default behavior is to increment the IP ID with every segment. If the
>>> -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
>>> +GSO type SKB_GSO_TCP_FIXEDID_{OUTER,INNER} 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.
>>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>>> index bfa5568baa92..b28f890b0af5 100644
>>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>>> @@ -3868,7 +3868,7 @@ static int hns3_gro_complete(struct sk_buff *skb, u32 l234info)
>>> skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;
>>>
>>> if (l234info & BIT(HNS3_RXD_GRO_FIXID_B))
>>> - skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID;
>>> + skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID_OUTER;
>>>
>>> skb->csum_start = (unsigned char *)th - skb->head;
>>> skb->csum_offset = offsetof(struct tcphdr, check);
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>> index b8c609d91d11..78df60c62225 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>> @@ -1289,8 +1289,12 @@ static void mlx5e_shampo_update_ipv4_tcp_hdr(struct mlx5e_rq *rq, struct iphdr *
>>> tcp->check = ~tcp_v4_check(skb->len - tcp_off, ipv4->saddr,
>>> ipv4->daddr, 0);
>>> skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV4;
>>> - if (ntohs(ipv4->id) == rq->hw_gro_data->second_ip_id)
>>> - skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID;
>>> + if (ntohs(ipv4->id) == rq->hw_gro_data->second_ip_id) {
>>> + bool encap = rq->hw_gro_data->fk.control.flags & FLOW_DIS_ENCAPSULATION;
>>> +
>>> + skb_shinfo(skb)->gso_type |= encap ?
>>> + SKB_GSO_TCP_FIXEDID_INNER : SKB_GSO_TCP_FIXEDID_OUTER;
>>> + }
>>>
>>> skb->csum_start = (unsigned char *)tcp - skb->head;
>>> skb->csum_offset = offsetof(struct tcphdr, check);
>>> diff --git a/drivers/net/ethernet/sfc/ef100_tx.c b/drivers/net/ethernet/sfc/ef100_tx.c
>>> index e6b6be549581..aab2425e62bb 100644
>>> --- a/drivers/net/ethernet/sfc/ef100_tx.c
>>> +++ b/drivers/net/ethernet/sfc/ef100_tx.c
>>> @@ -189,7 +189,8 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
>>> {
>>> bool gso_partial = skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL;
>>> unsigned int len, ip_offset, tcp_offset, payload_segs;
>>> - u32 mangleid = ESE_GZ_TX_DESC_IP4_ID_INC_MOD16;
>>> + u32 mangleid_outer = ESE_GZ_TX_DESC_IP4_ID_INC_MOD16;
>>> + u32 mangleid_inner = ESE_GZ_TX_DESC_IP4_ID_INC_MOD16;
>>> unsigned int outer_ip_offset, outer_l4_offset;
>>> u16 vlan_tci = skb_vlan_tag_get(skb);
>>> u32 mss = skb_shinfo(skb)->gso_size;
>>> @@ -200,8 +201,10 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
>>> bool outer_csum;
>>> u32 paylen;
>>>
>>> - if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID)
>>> - mangleid = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
>>> + if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID_OUTER)
>>> + mangleid_outer = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
>>> + if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_FIXEDID_INNER)
>>> + mangleid_inner = ESE_GZ_TX_DESC_IP4_ID_NO_OP;
>>> if (efx->net_dev->features & NETIF_F_HW_VLAN_CTAG_TX)
>>> vlan_enable = skb_vlan_tag_present(skb);
>>>
>>> @@ -239,14 +242,13 @@ static void ef100_make_tso_desc(struct efx_nic *efx,
>>> ESF_GZ_TX_TSO_CSO_INNER_L4, 1,
>>> ESF_GZ_TX_TSO_INNER_L3_OFF_W, ip_offset >> 1,
>>> ESF_GZ_TX_TSO_INNER_L4_OFF_W, tcp_offset >> 1,
>>> - ESF_GZ_TX_TSO_ED_INNER_IP4_ID, mangleid,
>>> + ESF_GZ_TX_TSO_ED_INNER_IP4_ID, mangleid_inner,
>>> ESF_GZ_TX_TSO_ED_INNER_IP_LEN, 1,
>>> ESF_GZ_TX_TSO_OUTER_L3_OFF_W, outer_ip_offset >> 1,
>>> ESF_GZ_TX_TSO_OUTER_L4_OFF_W, outer_l4_offset >> 1,
>>> ESF_GZ_TX_TSO_ED_OUTER_UDP_LEN, udp_encap && !gso_partial,
>>> ESF_GZ_TX_TSO_ED_OUTER_IP_LEN, encap && !gso_partial,
>>> - ESF_GZ_TX_TSO_ED_OUTER_IP4_ID, encap ? mangleid :
>>> - ESE_GZ_TX_DESC_IP4_ID_NO_OP,
>>> + ESF_GZ_TX_TSO_ED_OUTER_IP4_ID, mangleid_outer,
>>> ESF_GZ_TX_TSO_VLAN_INSERT_EN, vlan_enable,
>>> ESF_GZ_TX_TSO_VLAN_INSERT_TCI, vlan_tci
>>> );
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index 5e5de4b0a433..e55ba6918b0a 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -5287,13 +5287,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_OUTER | SKB_GSO_TCP_FIXEDID_INNER))
>>> + gso_type |= __SKB_GSO_TCP_FIXEDID;
>>
>> This is quite peculiar.
>>
>> Is there a real use case for specifying FIXEDID separately for outer
>> and inner? Can the existing single bit govern both together instead?
>> That would be a lot simpler.
>
> I guess not, as with GRO this is under control of the sender, and
> possibly a separate middlebox in control of encapsulation.
>
> Still, possible to preserve existing FIXEDID for the unencapsulated
> or inner, and add only one extra FIXEDID for outer? Or is there value
> in having three bits?
The ideal solution would be to split NETIF_F_TSO_MANGLEID into
NETIF_F_TSO_MANGLEID_OUTER and NETIF_F_TSO_MANGLEID_INNER.
As I noted, we can add this in the future, then each SKB_GSO_FIXEDID
bit will have a corresponding NETIF_F_TSO_MANGLEID bit. This would
be a much bigger change, as it requires modifying all of the
drivers that use NETIF_F_TSO_MANGLEID. My proposed solution is
somewhat of a middle-ground, as it preserves the current behavior of
NETIF_F_TSO_MANGLEID.
Preserving the existing FIXEDID for unencapsulated or inner headers
seems confusing to me, and just moves the ugly code elsewhere. We also
still need to compare both SKB_GSO_FIXEDID bits against NETIF_F_TSO_MANGLEID.
Powered by blists - more mailing lists