netdev - Re: [PATCH bpf-next v2 3/5] selftests/bpf: use thoff instead of nhoff in BPF flow dissector

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20181206024715.y7ofum6plgxhr5sa@ast-mbp.dhcp.thefacebook.com>
Date:   Wed, 5 Dec 2018 18:47:18 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
Cc:     Song Liu <liu.song.a23@...il.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Networking <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        simon.horman@...ronome.com
Subject: Re: [PATCH bpf-next v2 3/5] selftests/bpf: use thoff instead of
 nhoff in BPF flow dissector

On Tue, Dec 04, 2018 at 03:26:15PM -0800, Stanislav Fomichev wrote:
> On 12/04, Song Liu wrote:
> > On Mon, Dec 3, 2018 at 8:01 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > >
> > > We are returning thoff from the flow dissector, not the nhoff. Pass
> > > thoff along with nhoff to the bpf program (initially thoff == nhoff)
> > > and expect flow dissector amend/return thoff, not nhoff.
> > >
> > > This avoids confusion, when by the time bpf flow dissector exits,
> > > nhoff == thoff, which doesn't make much sense (this is relevant
> > > in the context of the next patch, where I add simple selftest
> > > and manually construct expected flow_keys).
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
> > >  net/core/flow_dissector.c              |  1 +
> > >  tools/testing/selftests/bpf/bpf_flow.c | 36 ++++++++++++--------------
> > >  2 files changed, 18 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > > index 3c8a78decbc0..ac19da6f390b 100644
> > > --- a/net/core/flow_dissector.c
> > > +++ b/net/core/flow_dissector.c
> > > @@ -708,6 +708,7 @@ bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
> > >         memset(flow_keys, 0, sizeof(*flow_keys));
> > >         cb->qdisc_cb.flow_keys = flow_keys;
> > >         flow_keys->nhoff = skb_network_offset(skb);
> > > +       flow_keys->thoff = flow_keys->nhoff;
> > 
> > Do we need this fix without this set? If yes, do we need it for bpf
> > tree as well?
> No, I don't think so. This just changes input to the flow dissector
> slightly (going forward).
> It used to be nhoff in, thoff out. Now it's thoff in (with nhoff for
> backwards compatibility) and thoff out.

That is still an api change.
Also patch 4 is a fix.
I think patches 3 and 4 need to go into bpf tree first.
Then wait for them to merge into bpf-next and resubmit the rest.