Re: [PATCH] net: fix wrong network header length

[Maciej Żenczykowski]

On Thu, Feb 10, 2022 at 8:12 PM Lina Wang <lina.wang@xxxxxxxxxxxx> wrote:
>
> On Thu, 2022-02-10 at 17:02 +0100, Paolo Abeni wrote:
>
> > > @@ -3682,6 +3682,7 @@ struct sk_buff *skb_segment_list(struct
> > > sk_buff *skb,
> > >     struct sk_buff *tail = NULL;
> > >     struct sk_buff *nskb, *tmp;
> > >     int err;
> > > +   unsigned int len_diff = 0;
> >
> > Mintor nit: please respect the reverse x-mas tree order.
> >
>
> Yes,v2 has change unsigned int to int

Reverse christmas tree, means from longest to shortest, like so:

     struct sk_buff *tail = NULL;
     struct sk_buff *nskb, *tmp;
+   int len_diff = 0;
     int err;

That said, I think the = 0 is not needed, so this can be just

+ int len_diff, err;

>
> > >
> > >     skb_push(skb, -skb_network_offset(skb) + offset);
> > > @@ -3721,9 +3722,11 @@ struct sk_buff *skb_segment_list(struct
> > > sk_buff *skb,
> > >             skb_push(nskb, -skb_network_offset(nskb) + offset);
> > >
> > >             skb_release_head_state(nskb);
> > > +           len_diff = skb_network_header_len(nskb) -
> > > skb_network_header_len(skb);
> > >              __copy_skb_header(nskb, skb);
> > >
> > >             skb_headers_offset_update(nskb, skb_headroom(nskb) -
> > > skb_headroom(skb));
> > > +           nskb->transport_header += len_diff;
> >
> > This does not look correct ?!? the network hdr position for nskb will
> > still be uncorrect?!? and even the mac hdr likely?!? possibly you
> > need
> > to change the offset in skb_headers_offset_update().
> >
>
> Network hdr position and mac hdr are both right, because bpf processing &
> skb_headers_offset_update have updated them to right position. After bpf
> loading, the first skb's network header&mac_header became 44, transport
> header still is 64. After skb_headers_offset_update, fraglist skb's mac
> header and network header are still 24, the same with original packet.
> Just fraglist skb's transport header became 44, as original is 64.
> Only transport header cannot be easily updated the same offset, because
> 6to4 has different network header.
>
> Actually,at the beginning, I want to change skb_headers_offset_update, but
> it has been called also in other place, maybe a new function should be
> needed here.
>
> Skb_headers_offset_update has other wrong part in my scenary,
> inner_transport_header\inner_network_header\inner_mac_header shouldnot be
> changed, but they are been updated because of different headroom. They are
> not used later, so wrong value didnot affect anything.
>
> > Paolo
> >
>
> Thanks!Maciej Żenczykowski, Kernel Networking Developer @ Google