Re: [PATCH bpf-next v2 2/2] bpf: Implement formatted output helpers with bstr_printf

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Florent Revest <revest@chromium.org>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	KP Singh <kpsingh@kernel.org>,
	Brendan Jackman <jackmanb@google.com>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Steven Rostedt <rostedt@goodmis.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next v2 2/2] bpf: Implement formatted output helpers with bstr_printf
Date: Tue, 27 Apr 2021 16:46:37 -0700	[thread overview]
Message-ID: <CAADnVQLQmt0-D_e=boXoK=FLRoXv9xzkCwM24zpbZERrEexLCw@mail.gmail.com> (raw)
In-Reply-To: <20210427174313.860948-3-revest@chromium.org>

On Tue, Apr 27, 2021 at 10:43 AM Florent Revest <revest@chromium.org> wrote:
> +                       if (fmt[i + 1] == 'B') {
> +                               if (tmp_buf)  {
> +                                       err = snprintf(tmp_buf,
> +                                                      (tmp_buf_end - tmp_buf),
> +                                                      "%pB",
...
> +                       if ((tmp_buf_end - tmp_buf) < sizeof_cur_ip) {

I removed a few redundant () like above and applied.

>                 if (fmt[i] == 'l') {
> -                       cur_mod = BPF_PRINTF_LONG;
> +                       sizeof_cur_arg = sizeof(long);
>                         i++;
>                 }
>                 if (fmt[i] == 'l') {
> -                       cur_mod = BPF_PRINTF_LONG_LONG;
> +                       sizeof_cur_arg = sizeof(long long);
>                         i++;
>                 }

This bit got me thinking.
I understand that this is how bpf_trace_printk behaved
and the sprintf continued the tradition, but I think it will
surprise bpf users.
The bpf progs are always 64-bit. The sizeof(long) == 8
inside any bpf program. So printf("%ld") matches that long.
The clang could even do type checking to make sure the prog
is passing the right type into printf() if we add
__attribute__ ((format (printf))) to bpf_helper_defs.h
But this sprintf() implementation will trim the value to 32-bit
to satisfy 'fmt' string on 32-bit archs.
So bpf program behavior would be different on 32 and 64-bit archs.
I think that would be confusing, since the rest of bpf prog is
portable. The progs work the same way on all archs
(except endianess, of course).
I'm not sure how to fix it though.
The sprintf cannot just pass 64-bit unconditionally, since
bstr_printf on 32-bit archs will process %ld incorrectly.
The verifier could replace %ld with %Ld.
The fmt string is a read only string for bpf_snprintf,
but for bpf_trace_printk it's not and messing with it at run-time
is not good. Copying the fmt string is not great either.
Messing with internals of bstr_printf is ugly too.
Maybe we just have to live with this quirk ?
Just add a doc to uapi/bpf.h to discourage %ld and be done?