| 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: <4b2c5770-ab53-43f6-8c68-7e2f4a912d8e@gmail.com>
Date: Tue, 16 Sep 2025 15:53:17 +0200
From: Richard Gobert <richardbgobert@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
netdev@...r.kernel.org, pabeni@...hat.com, ecree.xilinx@...il.com
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
horms@...nel.org, corbet@....net, saeedm@...dia.com, tariqt@...dia.com,
mbloch@...dia.com, leon@...nel.org, dsahern@...nel.org,
ncardwell@...gle.com, kuniyu@...gle.com, shuah@...nel.org, sdf@...ichev.me,
aleksander.lobakin@...el.com, florian.fainelli@...adcom.com,
alexander.duyck@...il.com, linux-kernel@...r.kernel.org,
linux-net-drivers@....com
Subject: Re: [PATCH net-next v5 4/5] net: gro: remove unnecessary df checks
Willem de Bruijn wrote:
> Richard Gobert wrote:
>> Currently, packets with fixed IDs will be merged only if their
>> don't-fragment bit is set. This restriction is unnecessary since packets
>> without the don't-fragment bit will be forwarded as-is even if they were
>> merged together.
>
> Please expand why this is true.
>
> Because either NETIF_F_TSO_MANGLEID is set or segmentation
> falls back onto software GSO which handles the two FIXEDID
> variants correctly now, I guess?
>
This is true because the merged packets will be segmented back to
their original forms before being forwarded. As you already said, the IDs
will either stay identical or potentially become incrementing if MANGLEID
is set, either of which is fine.
>> If packets are merged together and then fragmented, they will first be
>> re-split into segments before being further fragmented, so the behavior
>> is identical whether or not the packets were first merged together.
>
> I don't follow this scenario. Fragmentation of a GSO packet after GRO
> and before GSO?
>
Yes. One could worry that merging packets with the same ID but without DF
would cause issues if they are then fragmented by the host. What I'm saying
is that if such packets are merged and then fragmented, they will first be
segmented back to their original forms by GSO before being further fragmented
(see ip_finish_output_gso). The fragmentation occurs as if the packets were
never merged to begin with. IOW, fragmentation occurs the same way regardless
of whether the packets were merged (GRO + GSO is transparent). I thought I'd
mention this to clarify why this patch doesn't cause any issues.
>> Clean up the code by removing the unnecessary don't-fragment checks.
>>
>> Signed-off-by: Richard Gobert <richardbgobert@...il.com>
>> ---
>> include/net/gro.h | 5 ++---
>> net/ipv4/af_inet.c | 3 ---
>> tools/testing/selftests/net/gro.c | 9 ++++-----
>> 3 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/net/gro.h b/include/net/gro.h
>> index 6aa563eec3d0..f14b7e88dbef 100644
>> --- a/include/net/gro.h
>> +++ b/include/net/gro.h
>> @@ -448,17 +448,16 @@ static inline int inet_gro_flush(const struct iphdr *iph, const struct iphdr *ip
>> const u32 id2 = ntohl(*(__be32 *)&iph2->id);
>> const u16 ipid_offset = (id >> 16) - (id2 >> 16);
>> const u16 count = NAPI_GRO_CB(p)->count;
>> - const u32 df = id & IP_DF;
>>
>> /* All fields must match except length and checksum. */
>> - if ((iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 & IP_DF)))
>> + if ((iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | ((id ^ id2) & IP_DF))
>> return true;
>>
>> /* When we receive our second frame we can make a decision on if we
>> * continue this flow as an atomic flow with a fixed ID or if we use
>> * an incrementing ID.
>> */
>> - if (count == 1 && df && !ipid_offset)
>> + if (count == 1 && !ipid_offset)
>> NAPI_GRO_CB(p)->ip_fixedid |= 1 << inner;
>>
>> return ipid_offset ^ (count * !(NAPI_GRO_CB(p)->ip_fixedid & (1 << inner)));
>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> index fc7a6955fa0a..c0542d9187e2 100644
>> --- a/net/ipv4/af_inet.c
>> +++ b/net/ipv4/af_inet.c
>> @@ -1393,10 +1393,7 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
>>
>> segs = ERR_PTR(-EPROTONOSUPPORT);
>>
>> - /* 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;
>>
>> if (!skb->encapsulation || encap)
>> udpfrag = !!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP);
>> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
>> index d5824eadea10..3d4a82a2607c 100644
>> --- a/tools/testing/selftests/net/gro.c
>> +++ b/tools/testing/selftests/net/gro.c
>> @@ -670,7 +670,7 @@ static void send_flush_id_case(int fd, struct sockaddr_ll *daddr, int tcase)
>> iph2->id = htons(9);
>> break;
>>
>> - case 3: /* DF=0, Fixed - should not coalesce */
>> + case 3: /* DF=0, Fixed - should coalesce */
>> iph1->frag_off &= ~htons(IP_DF);
>> iph1->id = htons(8);
>>
>> @@ -1188,10 +1188,9 @@ static void gro_receiver(void)
>> correct_payload[0] = PAYLOAD_LEN * 2;
>> check_recv_pkts(rxfd, correct_payload, 1);
>>
>> - printf("DF=0, Fixed - should not coalesce: ");
>> - correct_payload[0] = PAYLOAD_LEN;
>> - correct_payload[1] = PAYLOAD_LEN;
>> - check_recv_pkts(rxfd, correct_payload, 2);
>> + printf("DF=0, Fixed - should coalesce: ");
>> + correct_payload[0] = PAYLOAD_LEN * 2;
>> + check_recv_pkts(rxfd, correct_payload, 1);
>>
>> printf("DF=1, 2 Incrementing and one fixed - should coalesce only first 2 packets: ");
>> correct_payload[0] = PAYLOAD_LEN * 2;
>> --
>> 2.36.1
>>
>
>
Powered by blists - more mailing lists