Re: [PATCH net-next v2 1/8] udp: fixup csum for GSO receive slow path

On Mon, 2021-03-29 at 08:28 -0400, Willem de Bruijn wrote:

On Mon, Mar 29, 2021 at 7:26 AM Paolo Abeni [off-list ref] wrote:

quoted

On Fri, 2021-03-26 at 14:30 -0400, Willem de Bruijn wrote:

quoted

On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni [off-list ref] wrote:

quoted

When UDP packets generated locally by a socket with UDP_SEGMENT
traverse the following path:

UDP tunnel(xmit) -> veth (segmentation) -> veth (gro) ->
        UDP tunnel (rx) -> UDP socket (no UDP_GRO)

they are segmented as part of the rx socket receive operation, and
present a CHECKSUM_NONE after segmentation.

would be good to capture how this happens, as it was not immediately obvious.

The CHECKSUM_PARTIAL is propagated up to the UDP tunnel processing,
where we have:

        __iptunnel_pull_header() -> skb_pull_rcsum() ->
skb_postpull_rcsum() -> __skb_postpull_rcsum() and the latter do the
conversion.

Please capture this in the commit message.

I will do.

quoted

quoted

quoted

Additionally the segmented packets UDP CB still refers to the original
GSO packet len. Overall that causes unexpected/wrong csum validation
errors later in the UDP receive path.

We could possibly address the issue with some additional checks and
csum mangling in the UDP tunnel code. Since the issue affects only
this UDP receive slow path, let's set a suitable csum status there.

v1 -> v2:
 - restrict the csum update to the packets strictly needing them
 - hopefully clarify the commit message and code comments

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
+       if (skb->ip_summed == CHECKSUM_NONE && !skb->csum_valid)
+               skb->csum_valid = 1;

Not entirely obvious is that UDP packets arriving on a device with rx
checksum offload off, i.e., with CHECKSUM_NONE, are not matched by
this test.

I assume that such packets are not coalesced by the GRO layer in the
first place. But I can't immediately spot the reason for it..

Packets with CHECKSUM_NONE are actually aggregated by the GRO engine.

Their checksum is validated by:

udp4_gro_receive -> skb_gro_checksum_validate_zero_check()
        -> __skb_gro_checksum_validate -> __skb_gro_checksum_validate_complete()

and skb->ip_summed is changed to CHECKSUM_UNNECESSARY by:

__skb_gro_checksum_validate -> skb_gro_incr_csum_unnecessary
        -> __skb_incr_checksum_unnecessary()

and finally to CHECKSUM_PARTIAL by:

udp4_gro_complete() -> udp_gro_complete() -> udp_gro_complete_segment()

Do you prefer I resubmit with some more comments, either in the commit
message or in the code?

That breaks the checksum-and-copy optimization when delivering to
local sockets. I wonder if that is a regression.

The conversion to CHECKSUM_UNNECESSARY happens since
commit 573e8fca255a27e3573b51f9b183d62641c47a3d.

Even the conversion to CHECKSUM_PARTIAL happens independently from this
series, since commit 6f1c0ea133a6e4a193a7b285efe209664caeea43.

I don't see a regression here ?!?

Thanks!

Paolo