| Message ID | 20210423011517.4069221-1-revest@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Implement BPF formatted output helpers with bstr_printf | expand |
On 23/04/2021 03.15, Florent Revest wrote: > Our formatted output helpers are currently implemented with > snprintf-like functions which take arguments as va_list but the types > stored in a va_list need to be known at compilation time which causes > problems when dealing with arguments from the BPF world that are always > u64 but considered differently depending on the format specifiers they > are associated with at runtime. > > This series replaces snprintf usages with bstr_printf calls. This lets > us construct a binary representation of arguments in bpf_printf_prepare > at runtime that matches an ABI that is neither arch nor compiler > specific. > > 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). Rasmus
On Fri, Apr 23, 2021 at 10:50 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > On 23/04/2021 03.15, Florent Revest wrote: > > Our formatted output helpers are currently implemented with > > snprintf-like functions which take arguments as va_list but the types > > stored in a va_list need to be known at compilation time which causes > > problems when dealing with arguments from the BPF world that are always > > u64 but considered differently depending on the format specifiers they > > are associated with at runtime. > > > > This series replaces snprintf usages with bstr_printf calls. This lets > > us construct a binary representation of arguments in bpf_printf_prepare > > at runtime that matches an ABI that is neither arch nor compiler > > specific. > > > > 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 ?
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