Re: [PATCH bpf-next 1/2] bpf: Add a bpf_kallsyms_lookup helper

[Florent Revest]

On Mon, Dec 14, 2020 at 7:47 AM Yonghong Song <yhs@xxxxxx> wrote:
> On 12/11/20 6:40 AM, Florent Revest wrote:
> > On Wed, Dec 2, 2020 at 10:18 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> >> I still think that adopting printk/vsnprintf for this instead of
> >> reinventing the wheel
> >> is more flexible and easier to maintain long term.
> >> Almost the same layout can be done with vsnprintf
> >> with exception of \0 char.
> >> More meaningful names, etc.
> >> See Documentation/core-api/printk-formats.rst
> >
> > I agree this would be nice. I finally got a bit of time to experiment
> > with this and I noticed a few things:
> >
> > First of all, because helpers only have 5 arguments, if we use two for
> > the output buffer and its size and two for the format string and its
> > size, we are only left with one argument for a modifier. This is still
> > enough for our usecase (where we'd only use "%ps" for example) but it
> > does not strictly-speaking allow for the same layout that Andrii
> > proposed.
>
> See helper bpf_seq_printf. It packs all arguments for format string and
> puts them into an array. bpf_seq_printf will unpack them as it parsed
> through the format string. So it should be doable to have more than
> "%ps" in format string.

This could be a nice trick, thank you for the suggestion Yonghong :)

My understanding is that this would also require two extra args (one
for the array of arguments and one for the size of this array) so it
would still not fit the 5 arguments limit I described in my previous
email.
eg: this would not be possible:
long bpf_snprintf(const char *out, u32 out_size,
                  const char *fmt, u32 fmt_size,
                 const void *data, u32 data_len)

Would you then suggest that we also put the format string and its
length in the first and second cells of this array and have something
along the line of:
long bpf_snprintf(const char *out, u32 out_size,
                  const void *args, u32 args_len) ?
This seems like a fairly opaque signature to me and harder to verify.