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

From: Yonghong Song <yhs@fb.com>
To: Florent Revest <revest@chromium.org>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	KP Singh <kpsingh@chromium.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Florent Revest <revest@google.com>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next 1/2] bpf: Add a bpf_kallsyms_lookup helper
Date: Sun, 13 Dec 2020 22:47:01 -0800	[thread overview]
Message-ID: <221fb873-80fc-5407-965e-b075c964fa13@fb.com> (raw)
In-Reply-To: <CABRcYm+zjC-WH2gxtfEX5S6mZj-5_ByAzVd5zi3aRmQv-asYqg@mail.gmail.com>

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.

> 
>> If we force fmt to come from readonly map then bpf_trace_printk()-like
>> run-time check of fmt string can be moved into load time check
>> and performance won't suffer.
> 
> Regarding this bit, I have the impression that this would not be
> possible, but maybe I'm missing something ? :)
> 
> The iteration that bpf_trace_printk does over the format string
> argument is not only used for validation. It is also used to remember
> what extra operations need to be done based on the modifier types. For
> example, it remembers whether an arg should be interpreted as 32bits or
> 64bits. In the case of string printing, it also remembers whether it is
> a kernel-space or user-space pointer so that bpf_trace_copy_string can
> be called with the right arg. If we were to run the iteration over the format
> string in the verifier, how would you recommend that we
> "remember" the modifier type until the helper gets called ?
> 