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

[Gal Pressman <gal@nvidia.com>]

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