Subject: Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT

From: Mathieu Desnoyers <[email-masked]>
Date: Tue, 27 Mar 2018 15:10:55 -0400 (EDT)

----- On Mar 27, 2018, at 3:00 PM, rostedt [email-masked] wrote:

> On Tue, 27 Mar 2018 11:45:34 -0700
> Alexei Starovoitov <[email-masked]> wrote:
> 
>> >> +
>> >> +	snprintf(buf, sizeof(buf), "__bpf_trace_%s", tp->name);
>> >> +	addr = kallsyms_lookup_name(buf);
>> >> +	if (!addr)
>> >> +		return -ENOENT;
>> >> +
>> >> +	return tracepoint_probe_register(tp, (void *)addr, prog);
>> >
>> > You are putting in a hell of a lot of trust with kallsyms returning
>> > properly. I can see this being very fragile. This is calling a function
>> > based on the result of kallsyms. I'm sure the security folks would love
>> > this.
>> >
>> > There's a few things to make this a bit more robust. One is to add a
>> > table that points to all __bpf_trace_* functions, and verify that the
>> > result from kallsyms is in that table.
>> >
>> > Honestly, I think this is too much of a short cut and a hack. I know
>> > you want to keep it "simple" and save space, but you really should do
>> > it the same way ftrace and perf do it. That is, create a section and
>> > have all tracepoints create a structure that holds a pointer to the
>> > tracepoint and to the bpf probe function. Then you don't even need the
>> > kernel_tracepoint_find_by_name(), you just iterate over your table and
>> > you get the tracepoint and the bpf function associated to it.
>> >
>> > Relying on kallsyms to return an address to execute is just way too
>> > extreme and fragile for my liking.
>> 
>> Wasting extra 8bytes * number_of_tracepoints just for lack of trust
>> in kallsyms doesn't sound like good trade off to me.
>> If kallsyms are inaccurate all sorts of things will break:
>> kprobes, livepatch, etc.
>> I'd rather suggest for ftrace to use kallsyms approach as well
>> and reduce memory footprint.
> 
> If Linus, Thomas, Peter, Ingo, and the security folks trust kallsyms to
> return a valid function pointer from a name, then sure, we can try
> going that way.

This will crash on ARM Thumb2 kernels. Also, how is this expected to
work on PowerPC ABIv1 without KALLSYMS_ALL ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com