netdev - Re: [PATCH bpf-next 2/5] bpf: add bpf_trace

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAEf4BzZf84FWnrz7zimcW0tw-k1im6kaJJ+g6ypzushXEb3oeA@mail.gmail.com>
Date:   Tue, 24 Aug 2021 14:24:18 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Dave Marchevsky <davemarchevsky@...com>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Yonghong Song <yhs@...com>,
        Florent Revest <revest@...omium.org>,
        Networking <netdev@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/5] bpf: add bpf_trace_vprintk helper

On Tue, Aug 24, 2021 at 2:00 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Tue, Aug 24, 2021 at 11:24 AM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >
> > On Tue, Aug 24, 2021 at 11:17 AM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Tue, Aug 24, 2021 at 11:02 AM Andrii Nakryiko
> > > <andrii.nakryiko@...il.com> wrote:
> > > >
> > > > On Tue, Aug 24, 2021 at 10:57 AM Alexei Starovoitov
> > > > <alexei.starovoitov@...il.com> wrote:
> > > > >
> > > > > On Mon, Aug 23, 2021 at 9:50 PM Andrii Nakryiko
> > > > > <andrii.nakryiko@...il.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 20, 2021 at 7:59 PM Dave Marchevsky <davemarchevsky@...com> wrote:
> > > > > > >
> > > > > > > This helper is meant to be "bpf_trace_printk, but with proper vararg
> > > > > >
> > > > > > We have bpf_snprintf() and bpf_seq_printf() names for other BPF
> > > > > > helpers using the same approach. How about we call this one simply
> > > > > > `bpf_printf`? It will be in line with other naming, it is logical BPF
> > > > > > equivalent of user-space printf (which outputs to stderr, which in BPF
> > > > > > land is /sys/kernel/debug/tracing/trace_pipe). And it will be logical
> > > > > > to have a nice and short BPF_PRINTF() convenience macro provided by
> > > > > > libbpf.
> > > > > >
> > > > > > > support". Follow bpf_snprintf's example and take a u64 pseudo-vararg
> > > > > > > array. Write to dmesg using the same mechanism as bpf_trace_printk.
> > > > > >
> > > > > > Are you sure about the dmesg part?... bpf_trace_printk is outputting
> > > > > > into /sys/kernel/debug/tracing/trace_pipe.
> > > > >
> > > > > Actually I like bpf_trace_vprintk() name, since it makes it obvious that
> > > >
> > > > It's the inconsistency with bpf_snprintf() and bpf_seq_printf() that's
> > > > mildly annoying (it's f at the end, and no v- prefix). Maybe
> > > > bpf_trace_printf() then? Or is it too close to bpf_trace_printk()?
> > >
> > > bpf_trace_printf could be ok, but see below.
> > >
> > > > But
> > > > either way you would be using BPF_PRINTF() macro for this. And we can
> > > > make that macro use bpf_trace_printk() transparently for <3 args, so
> > > > that new macro works on old kernels.
> > >
> > > Cannot we change the existing bpf_printk() macro to work on old and new kernels?
> >
> > Only if we break backwards compatibility. And I only know how to
> > detect the presence of new helper with CO-RE, which automatically
> > makes any BPF program using this macro CO-RE-dependent, which might
> > not be what users want (vmlinux BTF is still not universally
> > available). If I could do something like that without breaking change
> > and without CO-RE, I'd update bpf_printk() to use `const char *fmt`
> > for format string a long time ago. But adding CO-RE dependency for
> > bpf_printk() seems like a no-go.
>
> I see. Naming is the hardest.
> I think Dave's current choice of lower case bpf_vprintk() macro and
> bpf_trace_vprintk()
> helper fits the existing bpf_printk/bpf_trace_printk the best.
> Yes, it's inconsistent with BPF_SEQ_PRINTF/BPF_SNPRINTF,
> but consistent with trace_printk. Whichever way we go it will be inconsistent.
> Stylistically I like the lower case macro, since it doesn't scream at me.

Ok, it's fine. Even more so because we don't need a new macro, we can
just extend the existing bpf_printk() macro to automatically pick
bpf_trace_printk() if more than 3 arguments is provided.

Dave, you'll have to solve a bit of a puzzle macro-wise, but it's
possible to use either bpf_trace_printk() or bpf_trace_vprintk()
transparently for the user.

The only downside is that for <3 args, for backwards compatibility,
we'd have to stick to

char ___fmt[] = fmt;

vs more efficient

static const char ___fmt[] = fmt;

But I'm thinking it might be time to finally make this improvement. We
can also allow users to fallback to less efficient ways for really old
kernels with some extra flag, like so

#ifdef BPF_NO_GLOBAL_DATA
char ___fmt[] = fmt;
#else
static const char ___fmt[] = fmt;
#end

Thoughts?