Re: [PATCH net] udp: Fix UDP length on last GSO_PARTIAL segment

["Alice Mikityanska" <alice.kernel@fastmail.im>]

On Wed, May 13, 2026, at 13:28, Gal Pressman wrote:
> On 13/05/2026 13:08, Alice Mikityanska wrote:
>> On Wed, May 13, 2026, at 11:43, Gal Pressman wrote:
>>> On 13/05/2026 12:17, Alice Mikityanska wrote:
>>>> On Wed, May 13, 2026, at 09:43, Gal Pressman wrote:
>>>>> Following the cited commit, __udp_gso_segment() writes single MSS length
>>>>> in the UDP header.
>>>>> The cited patch doesn't account for the fact that the last segment could
>>>>> be a GSO skb by itself. This could happen when the size of the packet is
>>>>> a multiple of MSS, hence the first segment is also the last one (there
>>>>> is no need for a remainder skb).
>>>>>
>>>>> When the post-loop segment is a GSO skb, assign the single MSS length in
>>>>> the UDP header.
>>>>>
>>>>> Fixes: b10b446ce7ad ("udp: gso: Use single MSS length in UDP header for 
>>>>> GSO_PARTIAL")
>>>>> Reported-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>>>>> Closes: 
>>>>> https://lore.kernel.org/all/6c3fb15e-711d-4b8d-b152-e03d9b05293f@linux.dev/
>>>>> Tested-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>>>>> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
>>>>> Signed-off-by: Gal Pressman <gal@nvidia.com>
>>>>> ---
>>>>>  net/ipv4/udp_offload.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>>>> index a0813d425b71..71df45f9488a 100644
>>>>> --- a/net/ipv4/udp_offload.c
>>>>> +++ b/net/ipv4/udp_offload.c
>>>>> @@ -604,7 +604,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>>>>>  		       seg->data_len);
>>>>>  	check = csum16_add(csum16_sub(uh->check, uh->len), newlen);
>>>>>
>>>>> -	uh->len = newlen;
>>>>> +	uh->len = skb_is_gso(seg) ? msslen : newlen;
>>>>>  	uh->check = check;
>>>>
>>>> This is going to have the same checksum bug as your first commit, which
>>>> I'm fixing in [1]. You should use the right value of either msslen or
>>>> newlen when modifying check a couple of lines above.
>>>
>>> I tend to agree that the checksum seems to have the wrong value, the
>>> reason I chose not to change it is because my work only moved the UDP
>>> length assignment from the drivers to the stack.
>>>
>>> The "wrong" checksum value was used regardless of my change,
>> 
>> The wrong checksum was visible only inside the driver, and since the
>> hardware didn't care (due to the offload), it worked well. It was the
>> driver business to make sure the corresponding hardware likes the
>> packet. After commit b10b446ce7ad ("udp: gso: Use single MSS length in
>> UDP header for GSO_PARTIAL"), however, the wrong checksum moved one
>> abstraction layer higher - to the networking stack, which attempts to
>> keep the checksum correct for a more generic case. It's not the same,
>> and while I'm trying to fix one occurrence, I'd prefer not to
>> introduce more.
>> 
>>> and I
>>> prefer not to change it as part of this work.
>>>
>>>>
>>>> That said, maybe you can base your patch on top of my checksum fix? For
>>>> the last packet, it will then be:
>>>>
>>>> if (!skb_is_gso(seg))
>>>>         newlen = /* the new value */;
>>>> /* keep newlen as is otherwise: my newlen is your msslen */
>>>> check = csum16_add(csum16_sub(uh->check, uh->len), newlen);
>>>> uh->len = newlen;
>>>> uh->check = check;
>>>>
>>>> [1]: https://lore.kernel.org/netdev/20260512165648.386518-3-alice.kernel@fastmail.im/
>>>
>>> As I mentioned in the other thread, this fix goes to net, I can't take
>>> your patch.
>> 
>> Ah, that's right. Still, I guess you can take mine to net for the
>> checksum fix.
>
> Do you think it is suitable for net?

This is always a mystery to me... I've had different experiences:
sometimes I am asked to resubmit less important fixes to -next,
sometimes a fix is a fix and must go to net. This one is a refactoring
useful for my further patches + the checksum fix, which we get for free
and which I considered not important enough to submit to net (hardware
offload fixes the checksum in most cases anyway). Following the "a fix
is a fix" logic, we may try sending it to net. What do you think?

> Would you like to rephrase the
> commit message accordingly?
>
> If you can send me an updated version I will take it and submit both (or
> you can submit mine if you prefer).