netdev - Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Apr 2019 16:37:50 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
Cc:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, davem@...emloft.net, ast@...nel.org,
        daniel@...earbox.net, simon.horman@...ronome.com,
        willemb@...gle.com, peterpenkov96@...il.com,
        Maxim Krasnyansky <maxk@....qualcomm.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        intel-wired-lan@...ts.osuosl.org,
        Yisen Zhuang <yisen.zhuang@...wei.com>,
        Salil Mehta <salil.mehta@...wei.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Igor Russkikh <igor.russkikh@...antia.com>
Subject: Re: [PATCH bpf-next v5 5/6] net: pass net argument to the
 eth_get_headlen

On Fri, Apr 19, 2019 at 04:29:44PM -0700, Stanislav Fomichev wrote:
> On 04/18, Alexei Starovoitov wrote:
> > On Thu, Apr 18, 2019 at 05:43:50PM -0700, Stanislav Fomichev wrote:
> > > On 04/18, Alexei Starovoitov wrote:
> > > > On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> > > > > Update all users eth_get_headlen to pass network namespace
> > > > > and pass it down to the flow dissector. This commit is a noop
> > > > > until administrator inserts BPF flow dissector program.
> > > > > 
> > > > > Cc: Maxim Krasnyansky <maxk@....qualcomm.com>
> > > > > Cc: Saeed Mahameed <saeedm@...lanox.com>
> > > > > Cc: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > > > > Cc: intel-wired-lan@...ts.osuosl.org
> > > > > Cc: Yisen Zhuang <yisen.zhuang@...wei.com>
> > > > > Cc: Salil Mehta <salil.mehta@...wei.com>
> > > > > Cc: Michael Chan <michael.chan@...adcom.com>
> > > > > Cc: Igor Russkikh <igor.russkikh@...antia.com>
> > > > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ... 
> > > > Also please add C based test for skb-less flow_dissector.
> > > > Current test_flow_dissector.sh doesn't seem to cover it.
> > > It doesn't look like we can exercise skb-less flow dissector from
> > > test_flow_dissector.sh; we need to trigger some driver code, which is
> > > hard when we send the packets on the localhost in
> > > test_flow_dissector.sh.
> > > 
> > > To test skb-less dissector I convert BPF_PROG_TEST_RUN to always use skb-less
> > > mode. test_flow_dissector.sh tests skb-mode, prog_tests/flow_dissector.c
> > > tests skb-less mode.
> > 
> > I saw that but I'm afraid it's not enough.
> > tun_get_user() is calling it, so it should be possible to test
> > skb-less mode via tun.
> Spent some time today looking into how to exercise this path in the tun
> driver: doing writev() with IFF_NAPI_FRAGS IFF_TAP device would trigger
> eth_get_headlen, but it looks like there is no way to do a test with
> pass/no-pass result around that.
> 
> The problem is - we don't actually do anything with the result of
> eth_get_headlen, there is only a sanity check for "headlen >
> skb_headlen(skb)" which can't trigger for BPF flow dissector; we
> carefully clamp thoff/nhoff and should not return offset outside the
> input buffer.
> 
> By reading git history it looks like this call to eth_get_headlen was
> added there to only make it possible for tools like syzbot to fuzz flow
> dissector. That's why we don't care about the result, we just do that
> simple sanity check. The main goal is to trigger some problem
> (loop/warning) in the flow dissector code.
> 
> tl;dr - no mater which bpf flow dissector is attached to the namespace,
> it would not change behavior of the tun device; even empty 'return
> false' program would not alter it.

sure, but the program will run and the test can validate that the program
saw valid packet, parsed it correctly and returned correct dissection.
The results can be stored in a map and validated by the test.
iirc you were saying that you'll have one program doing dissection
for with-skb and skb-less cases.
I think it's important to have such program in selftests and being
run continuously for both cases.