Message-ID: <1352757747.48575.1605647318836.JavaMail.zimbra@efficios.com>
Date:   Tue, 17 Nov 2020 16:08:38 -0500 (EST)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     rostedt <rostedt@...dmis.org>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Matt Mullins <mmullins@...x.us>,
        Ingo Molnar <mingo@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] tracepoint: Do not fail unregistering a probe due to
 memory allocation

----- On Nov 17, 2020, at 3:34 PM, rostedt rostedt@...dmis.org wrote:

> On Tue, 17 Nov 2020 14:47:20 -0500 (EST)
> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> 
>> There seems to be more effect on the data size: adding the "stub_func" field
>> in struct tracepoint adds 8320 bytes of data to my vmlinux. But considering
>> the layout of struct tracepoint:
>> 
>> struct tracepoint {
>>         const char *name;               /* Tracepoint name */
>>         struct static_key key;
>>         struct static_call_key *static_call_key;
>>         void *static_call_tramp;
>>         void *iterator;
>>         int (*regfunc)(void);
>>         void (*unregfunc)(void);
>>         struct tracepoint_func __rcu *funcs;
>>         void *stub_func;
>> };
>> 
>> I would argue that we have many other things to optimize there if we want to
>> shrink the bloat, starting with static keys and system call reg/unregfunc
>> pointers.
> 
> This is the part that I want to decrease, and yes there's other fish to fry
> in that code, but I really don't want to be adding more.

I agree on the goal of not bloating the code and data size of tracepoints, but
I also don't want to introduce subtle hard-to-debug undefined behaviors.

> 
>> 
>> > 
>> > Since all tracepoints callbacks have at least one parameter (__data), we
>> > could declare tp_stub_func as:
>> > 
>> > static void tp_stub_func(void *data, ...)
>> > {
>> >	return;
>> > }
>> > 
>> > And now C knows that tp_stub_func() can be called with one or more
>> > parameters, and had better be able to deal with it!
>> 
>> AFAIU this won't work.
>> 
>> C99 6.5.2.2 Function calls
>> 
>> "If the function is defined with a type that is not compatible with the type (of
>> the
>> expression) pointed to by the expression that denotes the called function, the
>> behavior is
>> undefined."
> 
> But is it really a problem in practice. I'm sure we could create an objtool
> function to check to make sure we don't break anything at build time.

There are also tools like UBSAN which will trigger whenever an undefined behavior
is executed. Having tools which can validate that the generated assembly happens to
work does not make it OK to generate code with undefined behavior.

> 
>> 
>> and
>> 
>> 6.7.5.3 Function declarators (including prototypes), item 15:
>> 
>> "For two function types to be compatible, both shall specify compatible return
>> types.
> 
> But all tracepoint callbacks have void return types, which means they are
> compatible.

Yes, my concern is about what follows just after:

> 
>> 
>> Moreover, the parameter type lists, if both are present, shall agree in the
>> number of
>> parameters and in use of the ellipsis terminator; corresponding parameters shall
>> have
>> compatible types. [...]"
> 
> Which is why I gave the stub function's first parameter the same type that
> all tracepoint callbacks have a prototype that starts with "void *data"
> 
> and my solution is to define:
> 
>	void tp_stub_func(void *data, ...) { return; }
> 
> Which is in line with: "corresponding parameters shall have compatible
> types". The corresponding parameter is simply "void *data".

No, you forgot about the part "[...] shall agree [...] in use of the ellipsis
terminator"

That last part about agreeing about use of the ellipsis terminator is what
makes your last solution run into undefined behavior territory. The caller
and callee don't agree on the use of ellipsis terminator: the caller does not
use it, but the callee expects it.

> 
>> 
>> What you suggest here is to use the ellipsis in the stub definition, but the
>> caller
>> prototype does not use the ellipsis, which brings us into undefined behavior
>> territory
>> again.
> 
> And I believe the "undefined behavior" is that you can't trust what is in
> the parameters if the callee chooses to look at them, and that is not the
> case here.

I am aware of no such definition of "undefined behavior". So you would be
very much dependent on the compiler choosing a not-so-hurtful way to deal
with this behavior then.

> But since the called function doesn't care, I highly doubt it
> will ever be an issue. I mean, the only way this can break is if the caller
> places something in the stack that it expects the callee to fix.

AFAIU an undefined behavior is something we really try to avoid in C. As I said
earlier, it seems to work in practice because the cdecl calling convention leaves
the stack cleanup to the caller. But I'm worried about subtle portability issues
that may arise due to this.

> With all the functions in assembly we have, I'm pretty confident that if a compiler
> does something like this, it would break all over the place.

Fair point. Then maybe we should write the stub in assembly ?

Thanks,

Mathieu

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

Powered by blists - more mailing lists