On Thu, Dec 19, 2019 at 3:22 AM Steffen Klassert
[off-list ref] wrote:

On Wed, Dec 18, 2019 at 11:02:39AM -0500, Willem de Bruijn wrote:

quoted

On Wed, Dec 18, 2019 at 8:35 AM Steffen Klassert
[off-list ref] wrote:

quoted

+struct sk_buff *skb_segment_list(struct sk_buff *skb,
+                                netdev_features_t features,
+                                unsigned int offset)
+{
+       struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
+       unsigned int tnl_hlen = skb_tnl_header_len(skb);
+       unsigned int delta_truesize = 0;
+       unsigned int delta_len = 0;
+       struct sk_buff *tail = NULL;
+       struct sk_buff *nskb;
+
+       skb_push(skb, -skb_network_offset(skb) + offset);
+
+       skb_shinfo(skb)->frag_list = NULL;
+
+       do {
+               nskb = list_skb;
+               list_skb = list_skb->next;
+
+               if (!tail)
+                       skb->next = nskb;
+               else
+                       tail->next = nskb;
+
+               tail = nskb;
+
+               delta_len += nskb->len;
+               delta_truesize += nskb->truesize;
+
+               skb_push(nskb, -skb_network_offset(nskb) + offset);
+
+               if (!secpath_exists(nskb))
+                       __skb_ext_copy(nskb, skb);

Of all the possible extensions, why is this only relevant to secpath?

I wrote this before we had these extensions and adjusted it
when the extensions where introduced to make it compile again.
I think we can just copy the extensions unconditionally.

quoted

More in general, this function open codes a variety of skb fields that
carry over from skb to nskb. How did you select this subset of fields?

I tried to find the subset of __copy_skb_header() that is needed to copy.
Some fields of nskb are still valid, and some (csum related) fields
should not be copied from skb to nskb.

Duplicating that code is kind of fragile wrt new fields being added to
skbs later (such as the recent skb_ext example).

Perhaps we can split __copy_skb_header further and call the
inner part directly.

Or, at least follow the order of operations exactly and add a comment
that this code was taken verbatim from __copy_skb_header.

quoted

quoted

+
+               memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
+
+               nskb->tstamp = skb->tstamp;
+               nskb->dev = skb->dev;
+               nskb->queue_mapping = skb->queue_mapping;
+
+               nskb->mac_len = skb->mac_len;
+               nskb->mac_header = skb->mac_header;
+               nskb->transport_header = skb->transport_header;
+               nskb->network_header = skb->network_header;
+               skb_dst_copy(nskb, skb);
+
+               skb_headers_offset_update(nskb, skb_headroom(nskb) - skb_headroom(skb));
+               skb_copy_from_linear_data_offset(skb, -tnl_hlen,
+                                                nskb->data - tnl_hlen,
+                                                offset + tnl_hlen);
+
+               if (skb_needs_linearize(nskb, features) &&
+                   __skb_linearize(nskb))
+                       goto err_linearize;
+
+       } while (list_skb);
+
+       skb->truesize = skb->truesize - delta_truesize;
+       skb->data_len = skb->data_len - delta_len;
+       skb->len = skb->len - delta_len;
+       skb->ip_summed = nskb->ip_summed;
+       skb->csum_level = nskb->csum_level;

This changed from the previous version, where nskb inherited ip_summed
and csum_level from skb. Why is that?

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.

Either way, would you mind briefly documenting the checksum behavior
in the commit message? It's not trivial and I doubt I'll recall the
details in six months.

Really about patch 4: that squashed in a lot of non-trivial scaffolding
from previous patch 'UDP: enable GRO by default'. Does it make sense
to keep that in a separate patch? That should be a noop, which we can
verify. And it makes patch 4 easier to reason about on its own, too.