On Mon, Jan 20, 2020 at 3:36 AM Steffen Klassert
[off-list ref] wrote:

On Wed, Jan 15, 2020 at 10:43:08AM -0500, Willem de Bruijn wrote:

quoted

quoted

quoted

quoted

Maybe we can be conservative here and do a full
__copy_skb_header for now. The initial version
does not necessarily need to be the most performant
version. We could try to identify the correct subset
of header fields later then.

We should probably aim for the right set from the start. If you think
this set is it, let's keep it.

I'd prefer to do a full __copy_skb_header for now and think a bit
longer if that what I chose is really the correct subset.

Ok

quoted

quoted

quoted

quoted

quoted

I had to set ip_summed to CHECKSUM_UNNECESSARY on GRO to
make sure the noone touches the checksum of the head
skb. Otherise netfilter etc. tries to touch the csum.

Before chaining I make sure that ip_summed and csum_level is
the same for all chained skbs and here I restore the original
value from nskb.

This is safe because the skb_gro_checksum_validate will have validated
already on CHECKSUM_PARTIAL? What happens if there is decap or encap
in the path? We cannot revert to CHECKSUM_PARTIAL after that, I
imagine.

Yes, the checksum is validated with skb_gro_checksum_validate. If the
packets are UDP encapsulated, they are segmented before decapsulation.
Original values are already restored. If an additional encapsulation
happens, the encap checksum will be calculated after segmentation.
Original values are restored before that.

I was wondering more about additional other encapsulation protocols.

quoted

From a quick read, it seems like csum_level is associated only with

CHECKSUM_UNNECESSARY.

What if a device returns CHECKSUM_COMPLETE for packets with a tunnel
that is decapsulated before forwarding. Say, just VLAN. That gets
untagged in __netif_receive_skb_core with skb_vlan_untag calling
skb_pull_rcsum. After segmentation the ip_summed is restored, with
skb->csum still containing the unmodified csum that includes the VLAN
tag?

Hm, that could be really a problem. So setting CHECKSUM_UNNECESSARY
should be ok, but restoring the old values are not. Our checksum
magic is rather complex, it's hard to get it right for all possible
cases. Maybe we can just set CHECKSUM_UNNECESSARY for all packets
and keep this value after segmentation.

Note that I'm not 100% sure that the issue can occur. But it seems likely.

Yes, inverse CHECKSUM_UNNECESSARY conversion after verifying the checksum is
probably the way to go. Inverse, because it is the opposite of
__skb_gro_checksum_convert.

I'm not sure if I understand what you mean here.

I mean that I agree that it's okay to convert from CHECKSUM_COMPLETE
to CHECKSUM_UNNECESSARY if the UDP checksum has been validated.

I'd do the following
for fraglist GRO in udp4_gro_complete:

        if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
                if (skb->csum_level < SKB_MAX_CSUM_LEVEL)
                        skb->csum_level++;
        } else {
                skb->ip_summed = CHECKSUM_UNNECESSARY;
                skb->csum_level = 0;
        }

Akin to __skb_incr_checksum_unnecessary, but applying conversion both
in the case of CHECKSUM_NONE and CHECKSUM_COMPLETE. Makes sense.

and then copy these values to the segments after segmentation.