On Fri, May 7, 2021 at 4:25 AM Dongseok Yi [off-list ref] wrote:

On Thu, May 06, 2021 at 09:53:45PM -0400, Willem de Bruijn wrote:

quoted

On Thu, May 6, 2021 at 9:45 PM Yunsheng Lin [off-list ref] wrote:

quoted

On 2021/5/7 9:25, Willem de Bruijn wrote:

quoted

quoted

quoted

quoted

head_skb's data_len is the sum of skb_gro_len for each skb of the frags.
data_len could be 8 if server sent a small size packet and it is GROed
to head_skb.

Please let me know if I am missing something.

This is my understanding of the data path. This is a forwarding path
for TCP traffic.

GRO is enabled and will coalesce multiple segments into a single large
packet. In bad cases, the coalesced packet payload is > MSS, but < MSS
+ 20.

Somewhere between GRO and GSO you have a BPF program that converts the
IPv6 address to IPv4.

Your understanding is right. The data path is GRO -> BPF 6 to 4 ->
GSO.

quoted

There is no concept of head_skb at the time of this BPF program. It is
a single SKB, with an skb linear part and multiple data items in the
frags (no frag_list).

Sorry for the confusion. head_skb what I mentioned was a skb linear
part. I'm considering a single SKB with frags too.

quoted

When entering the GSO stack, this single skb now has a payload length
< MSS. So it would just make a valid TCP packet on its own?

skb_gro_len is only relevant inside the GRO stack. It internally casts
the skb->cb[] to NAPI_GRO_CB. This field is a scratch area that may be
reused for other purposes later by other layers of the datapath. It is
not safe to read this inside bpf_skb_proto_6_to_4.

The condition what I made uses skb->data_len not skb_gro_len. Does
skb->data_len have a different meaning on each layer? As I know,
data_len indicates the amount of frags or frag_list. skb->data_len
should be > 20 in the sample case because the payload size of the skb
linear part is the same with mss.

Ah, got it.

data_len is the length of the skb minus the length in the skb linear
section (as seen in skb_headlen).

So this gso skb consists of two segments, the first one entirely
linear, the payload of the second is in skb_shinfo(skb)->frags[0].

It is not guaranteed that gso skbs built from two individual skbs end
up looking like that. Only protocol headers in the linear segment and
the payload of both in frags is common.

quoted

We can modify netif_needs_gso as another option to hit
skb_needs_linearize in validate_xmit_skb. But I think we should compare
skb->gso_size and skb->data_len too to check if mss exceed a payload
size.

The rest of the stack does not build such gso packets with payload len
< mss, so we should not have to add workarounds in the gso hot path
for this.

Also no need to linearize this skb. I think that if the bpf program
would just clear the gso type, the packet would be sent correctly.
Unless I'm missing something.

Does the checksum/len field in ip and tcp/udp header need adjusting
before clearing gso type as the packet has became bigger?

gro takes care of this. see for instance inet_gro_complete for updates
to the ip header.

I think clearing the gso type will get an error at tcp4_gso_segment
because netif_needs_gso returns true in validate_xmit_skb.

Oh right. Whether a packet is gso is defined by gso_size being
non-zero, not by gso_type.

quoted

quoted

Also, instead of testing skb->data_len, may test the skb->len?

skb->len - (mac header + ip/ipv6 header + udp/tcp header) > mss + len_diff

Yes. Essentially doing the same calculation as the gso code that is
causing the packet to be dropped.

BPF program is usually out of control. Can we take a general approach?
The below 2 cases has no issue when mss upgrading.
1) skb->data_len > mss + 20
2) skb->data_len < mss && skb->data_len > 20
The corner case is when
3) skb->data_len > mss && skb->data_len < mss + 20

Again, you cannot use skb->data_len alone to make inferences about the
size of the second packet.

But to cover #3 case, we should check the condition Yunsheng Lin said.
What if we do mss upgrading for both #1 and #2 cases only?

+               unsigned short off_len = skb->data_len > shinfo->gso_size ?
+                       shinfo->gso_size : 0;
[...]
                /* Due to IPv4 header, MSS can be upgraded. */
-               skb_increase_gso_size(shinfo, len_diff);
+               if (skb->data_len - off_len > len_diff)
+                       skb_increase_gso_size(shinfo, len_diff);

That generates TCP packets with different MSS within the same stream.

My suggestion remains to just not change MSS at all. But this has to
be a new flag to avoid changing established behavior.