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

[Gal Pressman <gal@nvidia.com>]

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? 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).