On Tue, May 11, 2021 at 01:38:41PM -0400, Willem de Bruijn wrote:

On Mon, May 10, 2021 at 9:11 PM Dongseok Yi [off-list ref] wrote:

quoted

On Mon, May 10, 2021 at 09:46:25AM -0400, Willem de Bruijn wrote:

quoted

On Mon, May 10, 2021 at 9:19 AM Willem de Bruijn
[off-list ref] wrote:

quoted

quoted

quoted

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.

I don't understand why the mss size should be kept in GSO step. Will
there be any issue with different mss?

This issue has come up before and that has been the feedback from
TCP experts at one point.

quoted

In general, upgrading mss make sense when 6 to 4. The new flag would be
set by user to not change mss. What happened if user does not set the
flag? I still think we should fix the issue with a general approach. Or
can we remove the skb_increase_gso_size line?

Admins that insert such BPF packets should be aware of these issues.
And likely be using clamping. This is a known issue.

We arrived that the flag approach in bpf_skb_net_shrink. Extending
that  to bpf_skb_change_proto would be consistent.

As for more generic approach: does downgrading to non-TSO by clearing
gso_size work for this edge case?

It can hit __skb_linearize in validate_xmit_skb and frags will be
copied to a linear part. The linear part size can exceed the MTU of
skb->dev unexpectedly.

When does skb_needs_linearize return true here (besides lack of
scatter-gather support, which would also preclude TSO)?

As I know not every netdev support NETIF_F_SG. TSO requires SG.

    /* TSO requires that SG is present as well. */
    if ((features & NETIF_F_ALL_TSO) && !(features & NETIF_F_SG)) {
        netdev_dbg(dev, "Dropping TSO features since no SG feature.\n");
        features &= ~NETIF_F_ALL_TSO;
    }

quoted

I will make another patch with the flag approach.