Messages in this thread | | | Subject | Re: [PATCH bpf-next 1/2] bpf: Add a bpf_kallsyms_lookup helper | From | Yonghong Song <> | Date | Thu, 17 Dec 2020 20:39:44 -0800 |
| |
On 12/17/20 7:20 PM, Alexei Starovoitov wrote: > On Thu, Dec 17, 2020 at 09:26:09AM -0800, Yonghong Song wrote: >> >> >> On 12/17/20 7:31 AM, Florent Revest wrote: >>> On Mon, Dec 14, 2020 at 7:47 AM Yonghong Song <yhs@fb.com> wrote: >>>> On 12/11/20 6:40 AM, Florent Revest wrote: >>>>> On Wed, Dec 2, 2020 at 10:18 PM Alexei Starovoitov >>>>> <alexei.starovoitov@gmail.com> 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) >> >> Right. bpf allows only up to 5 parameters. >>> >>> 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. >> >> One way is to define an explicit type for args, something like >> struct bpf_fmt_str_data { >> char *fmt; >> u64 fmt_len; >> u64 data[]; >> }; > > that feels a bit convoluted. > > The reason I feel unease with the helper as was originally proposed > and with Andrii's proposal is all the extra strlen and strcpy that > needs to be done. In the helper we have to call kallsyms_lookup() > which is ok interface for what it was desinged to do, > but it's awkward to use to construct new string ("%s [%s]", sym, modname) > or to send two strings into a ring buffer. > Andrii's zero separator idea will simplify bpf prog, but user space > would need to do strlen anyway if it needs to pretty print. > If we take pain on converting addr to sym+modname let's figure out > how to make it easy for the bpf prog to do and easy for user space to consume. > That's why I proposed snprintf. > > As far as 6 arg issue: > long bpf_snprintf(const char *out, u32 out_size, > const char *fmt, u32 fmt_size, > const void *data, u32 data_len); > Yeah. It won't work as-is, but fmt_size is unnecessary nowadays. > The verifier understands read-only data. > Hence the helper can be: > long bpf_snprintf(const char *out, u32 out_size, > const char *fmt, > const void *data, u32 data_len); > The 3rd arg cannot be ARG_PTR_TO_MEM. > Instead we can introduce ARG_PTR_TO_CONST_STR in the verifier.
This should work except if fmt string is on the stack. Maybe this is an okay tradeoff.
> See check_mem_access() where it's doing bpf_map_direct_read(). > That 'fmt' string will be accessed through the same bpf_map_direct_read(). > The verifier would need to check that it's NUL-terminated valid string. > It should probably do % specifier checks at the same time. > At the end bpf_snprintf() will have 5 args and when wrapped with > BPF_SNPRINTF() macro it will accept arbitrary number of arguments to print. > It also will be generally useful to do all other kinds of pretty printing. >
| |