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

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
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>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next 0/2] Implement BPF formatted output helpers with bstr_printf
Date: Fri, 23 Apr 2021 16:31:54 +0200	[thread overview]
Message-ID: <8ef7319d-7692-0067-bb86-a0d5465e997f@rasmusvillemoes.dk> (raw)
In-Reply-To: <CABRcYmLDBfoM8rOwPf+SdqkmJgtFRLYF94S4Fv2eU=Uwg4asTQ@mail.gmail.com>

On 23/04/2021 15.26, Florent Revest wrote:
> On Fri, Apr 23, 2021 at 10:50 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>

>>> This solves a bug reported by Rasmus Villemoes that would mangle
>>> arguments on 32 bit machines.
>>
>> That's not entirely accurate. The arguments are also mangled on x86-64,
>> it's just that in a few cases that goes unnoticed. That's why I
>> suggested you try and take your test case (which I assume had been
>> passing with flying colours on x86-64) and rearrange the specifiers,
>> arguments and expected output string so that the (morally) 32 bit
>> arguments end up beyond those-that-end-up-in-the-reg_save_area.
>>
>> IOWs, it is the 32 bit arguments that are mangled (because they get
>> passed as-if they were actually 64 bits), and that applies on all
>> architectures; nothing to do with sizeof(long).
> 
> Mh, yes, I get your point and I agree that my description does not
> really fit what you reported.
> 
> I tried what you suggested though, with the current bpf-next/master on x86_64:
> BPF_SNPRINTF(out, sizeof(out),
> "%u %d %u %d %u %d %u %d %u %d %u %d",
> 1, -2, 3, -4, 5, -6, 7, -8, 9, -10, 11, -12);
> 
> And out is "1 -2 3 -4 5 -6 7 -8 9 -10 11 -12" so i can't seem to be
> able to produce the bug you described.
> Do you think I'm missing something? Would you try it differently ?
> 

Nah, sorry, I must have misremembered the x86-64 ABI. Re-reading it, it
clearly says as the very first thing "The size of each argument gets
rounded up to eightbytes". So each of the ints that get passed on the
stack do indeed occupy 8 bytes (i.e., the overflow_area pointer gets
adjusted by 8 bytes, for both va_arg(ap, int) and va_arg(ap, long)). So
it will indeed work on x86-64. And probably other 64 bit ABIs behave the
same way (it would make sense) - at least ppc64 and arm64 seem to behave
like that.

So in a round-about way it's probably true that the bug would only be
seen on 32 bit machines, but only because all (relevant) 64 bit arches
seem to, on the ABI level, effectively do argument promotion to u64 anyway.

Rasmus