netdev - Re: [PATCH bpf-next V2 3/6] selftests/bpf: xdp_hw

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]
Message-ID: <CAADnVQLC7ma7SWPOcjXhsZ2N0OyVtBr7TzCoT-_Dn+zQ2DEyWg@mail.gmail.com>
Date:   Wed, 22 Mar 2023 12:30:37 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Jesper Dangaard Brouer <jbrouer@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Larysa Zaremba <larysa.zaremba@...el.com>,
        xdp-hints@...-project.net, anthony.l.nguyen@...el.com,
        "Song, Yoong Siang" <yoong.siang.song@...el.com>,
        "Ong, Boon Leong" <boon.leong.ong@...el.com>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH bpf-next V2 3/6] selftests/bpf: xdp_hw_metadata RX hash
 return code info

On Wed, Mar 22, 2023 at 12:23 PM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> On Wed, Mar 22, 2023 at 12:17 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Wed, Mar 22, 2023 at 12:00 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > >
> > > On Wed, Mar 22, 2023 at 9:07 AM Alexei Starovoitov
> > > <alexei.starovoitov@...il.com> wrote:
> > > >
> > > > On Wed, Mar 22, 2023 at 9:05 AM Jesper Dangaard Brouer
> > > > <jbrouer@...hat.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 21/03/2023 19.47, Stanislav Fomichev wrote:
> > > > > > On Tue, Mar 21, 2023 at 6:47 AM Jesper Dangaard Brouer
> > > > > > <brouer@...hat.com> wrote:
> > > > > >>
> > > > > >> When driver developers add XDP-hints kfuncs for RX hash it is
> > > > > >> practical to print the return code in bpf_printk trace pipe log.
> > > > > >>
> > > > > >> Print hash value as a hex value, both AF_XDP userspace and bpf_prog,
> > > > > >> as this makes it easier to spot poor quality hashes.
> > > > > >>
> > > > > >> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> > > > > >> ---
> > > > > >>   .../testing/selftests/bpf/progs/xdp_hw_metadata.c  |    9 ++++++---
> > > > > >>   tools/testing/selftests/bpf/xdp_hw_metadata.c      |    5 ++++-
> > > > > >>   2 files changed, 10 insertions(+), 4 deletions(-)
> > > > > >>
> > > > > >> diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > > > > >> index 40c17adbf483..ce07010e4d48 100644
> > > > > >> --- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > > > > >> +++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> > > > > >> @@ -77,10 +77,13 @@ int rx(struct xdp_md *ctx)
> > > > > >>                  meta->rx_timestamp = 0; /* Used by AF_XDP as not avail signal */
> > > > > >>          }
> > > > > >>
> > > > > >> -       if (!bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash))
> > > > > >> -               bpf_printk("populated rx_hash with %u", meta->rx_hash);
> > > > > >> -       else
> > > > > >> +       ret = bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash);
> > > > > >> +       if (ret >= 0) {
> > > > > >> +               bpf_printk("populated rx_hash with 0x%08X", meta->rx_hash);
> > > > > >> +       } else {
> > > > > >> +               bpf_printk("rx_hash not-avail errno:%d", ret);
> > > > > >>                  meta->rx_hash = 0; /* Used by AF_XDP as not avail signal */
> > > > > >> +       }
> > > > > >>
> > > > > >>          return bpf_redirect_map(&xsk, ctx->rx_queue_index, XDP_PASS);
> > > > > >>   }
> > > > > >> diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > > > > >> index 400bfe19abfe..f3ec07ccdc95 100644
> > > > > >> --- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > > > > >> +++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > > > > >> @@ -3,6 +3,9 @@
> > > > > >>   /* Reference program for verifying XDP metadata on real HW. Functional test
> > > > > >>    * only, doesn't test the performance.
> > > > > >>    *
> > > > > >> + * BPF-prog bpf_printk info outout can be access via
> > > > > >> + * /sys/kernel/debug/tracing/trace_pipe
> > > > > >
> > > > > > s/outout/output/
> > > > > >
> > > > >
> > > > > Fixed in V3
> > > > >
> > > > > > But let's maybe drop it? If you want to make it more usable, let's
> > > > > > have a separate patch to enable tracing and periodically dump it to
> > > > > > the console instead (as previously discussed).
> > > > >
> > > > > Cat'ing /sys/kernel/debug/tracing/trace_pipe work for me regardless of
> > > > > setting in
> > > > > /sys/kernel/debug/tracing/events/bpf_trace/bpf_trace_printk/enable
> > > > >
> > > > > We likely need a followup patch that adds a BPF config switch that can
> > > > > disable bpf_printk calls, because this adds overhead and thus affects
> > > > > the timestamps.
> > > >
> > > > No. This is by design.
> > > > Do not use bpf_printk* in production.
> > >
> > > But that's not for the production? xdp_hw_metadata is a small tool to
> > > verify that the metadata being dumped is correct (during the
> > > development).
> > > We have a proper (less verbose) selftest in
> > > {progs,prog_tests}/xdp_metadata.c (over veth).
> > > This xdp_hw_metadata was supposed to be used for running it against
> > > the real hardware, so having as much debugging at hand as possible
> > > seems helpful? (at least it was helpful to me when playing with mlx4)
> >
> > The only use of bpf_printk is for debugging of bpf progs themselves.
> > It should not be used in any tool.
>
> Hmm, good point. I guess it also means we won't have to mess with
> enabling/dumping ftrace (and don't need this comment about cat'ing the
> file).
> Jesper, maybe we can instead pass the status of those
> bpf_xdp_metadata_xxx kfuncs via 'struct xdp_meta'? And dump this info
> from the userspace if needed.

There are so many other ways for bpf prog to communicate with user space.
Use ringbuf, perf_event buffer, global vars, maps, etc.
trace_pipe is debug only because it's global and will conflict with
all other debug sessions.