On Mon, Oct 11, 2021 at 11:30 PM Alexei Starovoitov <ast@xxxxxx> wrote:
>
> On 10/7/21 5:02 PM, andrii.nakryiko@xxxxxxxxx wrote:
> >
> > One interesting possibility that is now open by these changes is that it's
> > possible to do:
> >
> > bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah");
> >
> > and it will work as expected.
>
> Could you explain what would be the difference vs existing bpf_printk macro?
> Why these patches enable above to work ?
Good question. Have you ever noticed warnings like this during selftests build:
libbpf: elf: skipping unrecognized data section(6) .rodata.str1.1
I'm recalling from memory a bit here, so excuse me if I get some small
details wrong. Let's say we have a `bpf_printk("Hello!\n");` call. It
is turned into:
static const char fmt[] = "Hello!\n";
bpf_trace_printk(fmt, 7);
`fmt` variable above is always in .rodata section, it will even have a
dedicated BTF VAR with the name '<func>.fmt'. For reasons unknown to
me (Yonghong will know, probably), the compiler *also* and *sometimes*
puts the same "Hello!\n" string into the .rodata.str1.1 section. So we
end up with this warning about unknown and skipped .rodata.str1.1.
Good news is that we actually never reference "Hello!\n" from
.rodata.str1.1, which is why the bpf_printk() works today.
But if you were to rewrite the above snippet as more natural:
bpf_trace_printk("Hello!\n", 7);
... and compiler puts that "Hello!\n" into .rodata.str1.1 (and *not*
into .rodata), then we'd have a relocation against .rodata.str1.1,
which will fail because up until now libbpf never did anything with
.rodata.str1.1.
So it's a bit roundabout way to say that with this patch set, no
matter which .rodata* section compiler decided to put compile-time
constants into (doesn't have to be string, could be struct literal for
initializing a struct, for example) it should work, because it's just
a normal relocation, so all the libbpf relocation logic works, and for
kernel it will be just another global data ARRAY, so everything works
as expected. What was necessary to make this work was internal
refactoring to remove a simplifying assumption that there could be
only one .rodata map, and one .data map, etc.
Hope that clears it a bit.
>
> > I haven't updated libbpf-provided helpers in
> > bpf_helpers.h for snprintf, seq_printf, and printk, because using
> > `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill
> > out the buffer at runtime (no copying), but it also enforces that format
> > string is compile-time string literal:
> >
> > const char *s = NULL;
> >
> > bpf_printk("hi"); /* works */
> > bpf_printk(s); /* compilation error */
> >
> > By passing fmt directly to bpf_trace_printk() would actually compile
> > bpf_printk(s) above with no warnings and will not work at runtime, which is
> > worse user experience, IMO.
>
> What is the example of "compile with no warning and not work at runtime" ?
Right, so imagine we rewritten bpf_printk to expand into
bpf_trace_printk(fmt, sizeof(fmt), args...));
Now, if you do bpf_printk("BLAH"), everything works, fmt string
literal is passed properly and its size is calculated as 5. But if you
now pass const char *, you end up doing:
bpf_trace_printk(s, sizeof(s) /* == 8 */, args...);
See how passing a pointer is right for the first argument, but using
it to determine the string size is completely wrong. And the worst
thing is it won't trigger any warning. There's a similar danger of
using sizeof() with arrays. Pure C language gotcha, nothing BPF
specific.
