lkml.org 
[lkml]   [2020]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH bpf-next 1/2] bpf: Add a bpf_kallsyms_lookup helper
From
Date


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.
>

\
 
 \ /
  Last update: 2020-12-18 05:42    [W:0.117 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site