On Tue, Dec 17, 2024 at 3:32 PM Linus Torvalds torvalds@linux-foundation.org wrote:
If the *only* thing that accesses that word array is vbin_printf and bstr_printf, then I could just change the packing to act like va_list does (ie the word array would *actually* be a word array, and char and short values would get individual words).
But at least the bpf cde seems to know about the crazy packing, and also does that
tmp_buf = PTR_ALIGN(tmp_buf, sizeof(u32));in bpf_bprintf_prepare() because it knows that it's not *actually* an array of words despite it being documented as such.
Of course, the bpf code only does the packed access thing for '%c', and doesn't seem to know that the same is true of '%hd' and '%hhd', presumably because nobody actually uses that.
Let's add Alexei to the participants. I think bpf too would actually prefer that the odd char/short packing *not* be done, if only because it clearly does the wrong thing as-is for non-%c argument (ie those %hd/%hhd cases).
We reject %hd case as EINVAL and do byte copy for %c. All that was done as part of commit 48cac3f4a96d ("bpf: Implement formatted output helpers with bstr_printf") that cleaned things up greatly. The byte copy for %c wasn't deliberate to save space. Just happen to work with bstr_printf(). We can totally switch to u32 if that's the direction for bstr_printf. To handle %s we use bpf_trace_copy_string(tmp_buf, ) which does _nofault() underneath. Since the tmp_buf is byte packed because of strings the %c case just adds a byte too. Strings and %c can be made u32 aligned.
Since we're on this topic, Daniel is looking to reuse format_decode() in bpf_bprintf_prepare() to get rid of our manual format validation.