Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <d5dd3f10c144f7150ec508fa8e6d7a78ceabfc10.camel@redhat.com> <20220211040629.23703-1-lina.wang@mediatek.com>
In-Reply-To: <20220211040629.23703-1-lina.wang@mediatek.com>
From:   =?UTF-8?Q?Maciej_=C5=BBenczykowski?= <maze@google.com>
Date:   Thu, 10 Feb 2022 21:54:23 -0800
Message-ID: <CANP3RGc42sobVoq8LCs=9dAgRZmerV7ev_EMi8Qjb+1ZKeO4jQ@mail.gmail.com>
Subject: Re: [PATCH] net: fix wrong network header length
To:     Lina Wang <lina.wang@mediatek.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Linux NetDev <netdev@vger.kernel.org>,
        Kernel hackers <linux-kernel@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Willem Bruijn <willemb@google.com>,
        Eric Dumazet <edumazet@google.com>,
        zhuoliang.zhang@mediatek.com, chao.song@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Feb 10, 2022 at 8:12 PM Lina Wang <lina.wang@mediatek.com> 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 =3D NULL;
> > >     struct sk_buff *nskb, *tmp;
> > >     int err;
> > > +   unsigned int len_diff =3D 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 =3D NULL;
     struct sk_buff *nskb, *tmp;
+   int len_diff =3D 0;
     int err;

That said, I think the =3D 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 =3D 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 +=3D 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, bu=
t
> 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 ar=
e
> not used later, so wrong value didnot affect anything.
>
> > Paolo
> >
>
> Thanks!Maciej =C5=BBenczykowski, Kernel Networking Developer @ Google