| 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
| ||
|
Date: Mon, 30 Aug 2021 16:55:09 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Dave Marchevsky <davemarchevsky@...com>
Cc: bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Yonghong Song <yhs@...com>, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 bpf-next 3/6] libbpf: Modify bpf_printk to choose
helper based on arg count
On Wed, Aug 25, 2021 at 12:58 PM Dave Marchevsky <davemarchevsky@...com> wrote:
>
> Instead of being a thin wrapper which calls into bpf_trace_printk,
> libbpf's bpf_printk convenience macro now chooses between
> bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding
> format string) is >3, use bpf_trace_vprintk, otherwise use the older
> helper.
>
> The motivation behind this added complexity - instead of migrating
> entirely to bpf_trace_vprintk - is to maintain good developer experience
> for users compiling against new libbpf but running on older kernels.
> Users who are passing <=3 args to bpf_printk will see no change in their
> bytecode.
>
> __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF
> macros elsewhere in the file - it allows use of bpf_trace_vprintk
> without manual conversion of varargs to u64 array. Previous
> implementation of bpf_printk macro is moved to __bpf_printk for use by
> the new implementation.
>
> This does change behavior of bpf_printk calls with >3 args in the "new
> libbpf, old kernels" scenario. On my system, using a clang built from
> recent upstream sources (14.0.0 https://github.com/llvm/llvm-project.git
> 50b62731452cb83979bbf3c06e828d26a4698dca), attempting to use 4 args to
> __bpf_printk (old impl) results in a compile-time error:
>
> progs/trace_printk.c:21:21: error: too many args to 0x6cdf4b8: i64 = Constant<6>
> trace_printk_ret = __bpf_printk("testing,testing %d %d %d %d\n",
>
> I was able to replicate this behavior with an older clang as well. When
> the format string has >3 format specifiers, there is no output to the
> trace_pipe in either case.
>
> After this patch, using bpf_printk with 4 args would result in a
> trace_vprintk helper call being emitted and a load-time failure on older
> kernels.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@...com>
> ---
> tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++-------
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index b9987c3efa3c..5f087306cdfe 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -14,14 +14,6 @@
> #define __type(name, val) typeof(val) *name
> #define __array(name, val) typeof(val) *name[]
>
> -/* Helper macro to print out debug messages */
> -#define bpf_printk(fmt, ...) \
> -({ \
> - char ____fmt[] = fmt; \
> - bpf_trace_printk(____fmt, sizeof(____fmt), \
> - ##__VA_ARGS__); \
> -})
> -
> /*
> * Helper macro to place programs, maps, license in
> * different sections in elf_bpf file. Section names
> @@ -224,4 +216,41 @@ enum libbpf_tristate {
> ___param, sizeof(___param)); \
> })
>
> +/* Helper macro to print out debug messages */
> +#define __bpf_printk(fmt, ...) \
> +({ \
> + char ____fmt[] = fmt; \
> + bpf_trace_printk(____fmt, sizeof(____fmt), \
> + ##__VA_ARGS__); \
> +})
> +
> +/*
> + * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments
> + * instead of an array of u64.
> + */
> +#define __bpf_vprintk(fmt, args...) \
> +({ \
> + static const char ___fmt[] = fmt; \
> + unsigned long long ___param[___bpf_narg(args)]; \
> + \
> + _Pragma("GCC diagnostic push") \
> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + ___bpf_fill(___param, args); \
> + _Pragma("GCC diagnostic pop") \
> + \
> + bpf_trace_vprintk(___fmt, sizeof(___fmt), \
> + ___param, sizeof(___param)); \
nit: is this really misaligned or it's just Gmail's rendering?
> +})
> +
> +#define ___bpf_pick_printk(...) \
> + ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
> + __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
> + __bpf_vprintk, __bpf_vprintk, __bpf_printk, __bpf_printk, \
> + __bpf_printk, __bpf_printk)
There is no best solution with macros, but I think this one is
extremely error prone because __bpf_nth invocation is very long and
it's hard to even see where printk turns into vprintk.
How about doing it similarly to ___empty in bpf_core_read.h? It will
be something like this (untested and not even compiled, just a demo)
#define __bpf_printk_kind(...) ___bpf_nth(_, ##__VA_ARGS__, new, new,
new, new, new, <however many>, new, old /*3*/, old /*2*/, old /*1*/,
old /*0*/)
#define bpf_printk(fmt, args...) ___bpf_apply(___bpf_printk_,
___bpf_narg(args))(fmt, args)
And you'll have s/__bpf_printk/__bpf_printk_old/ (using
bpf_trace_printk) and s/__bpf_printk_new/__bpf_vprintk/ (using
bpf_trace_vprintk).
This new/old distinction makes it a bit clearer to me. I find
__bpf_nth so counterintuitive that I try not to use it directly
anywhere at all.
> +
> +#define bpf_printk(fmt, args...) \
> +({ \
> + ___bpf_pick_printk(args)(fmt, args); \
> +})
not sure ({ }) buys you anything?...
> +
> #endif
> --
> 2.30.2
>
Powered by blists - more mailing lists