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

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

On Wed, May 13, 2026, at 14:01, Gal Pressman wrote:
> On 13/05/2026 14:48, Alice Mikityanska wrote:
>> 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?
>
> IMHO, the motivation you provided in your previous message for the
> checksum fix is convincing, but as I said, I think the commit message
> should be phrased to explain the bug rather than the refactoring (and
> add a Fixes tag).

I can put it like this:

udp: gso: Fix handling checksum in __udp_gso_segment

The cited commit started using msslen for uh->len, but still uses newlen
to adjust uh->check. Although the checksum is ignored in most cases due
to the hardware offload, __udp_gso_segment attempts to maintain the
correct one. Fix uh->check and adjust it by the right value.

Additionally, after the fix, newlen becomes assigned and unused before
the loop. The code can be simplified a bit if mss adjustment is dropped,
so that newlen becomes equal to msslen before the loop, and msslen can
be also dropped, saving a few lines of code.

This brings us back to one variable, drops an unneeded arithmetic for
mss, and fixes the UDP checksum.

Fixes: b10b446ce7ad ("udp: gso: Use single MSS length in UDP header for GSO_PARTIAL")
Signed-off-by: Alice Mikityanska <alice@isovalent.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>