| 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: <63632c0f-8577-4ecd-8431-7d85fb464bab@gmail.com>
Date: Mon, 15 Sep 2025 11:11:30 +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, 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,
alexander.duyck@...il.com, linux-kernel@...r.kernel.org,
linux-net-drivers@....com
Subject: Re: [PATCH net-next v4 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. Merged packets are re-split into segments
>> before being fragmented, so the result is the same as if the packets
>> weren't merged to begin with.
>
> This can perhaps be reworded a bit for clarity. Something like "With
> the changes in the earlier patches in this series, the ID state (fixed
> or incrementing) is now recorded for both inner and outer IPv4 headers,
> so the restriction to only coalesce packets with fixed IDs can now be
> lifted."
This restriction is unnecessary regardless of this patch series. I'll
rephrase it anyway.
>>
>> Remove 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 322c5517f508..691f267b3969 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;
>
> This is just a cleanup?
>
> If so, please make a brief note in the commit message. I end up
> staring whether there is some deeper meaning relevant to the
> functional change.
>
Will do.
>>
>> /* 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;
>
> I understand why the GRO constraint can now be relaxed. But why does
> this also affect GSO?
>
> Fixed ID is invalid on the wire if DF is not set. Is the idea behind
> this change that GRO + GSO is just forwarding existing packets. Even
> if the incoming packets were invalid on this point?
>
Basically. Such packets are forwarded both with and without GRO, and since
GSO restores the packets to their original form before forwarding, there is
no reason not to perform GRO. These checks are redundant and are somewhat
confusing.
Note also that FIXEDID can only be set by GRO, and before this patch, GRO
didn't accept packets that had fixed IDs with DF not set, so the GSO check
was redundant anyway. With this patch, the removal of the GSO check is
necessary as otherwise GSO will not be able to restore the packets to their
original form.
>>
>> 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