linux-kernel - Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201117153451.3015c5c9@gandalf.local.home>
Date:   Tue, 17 Nov 2020 15:34:51 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
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 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.

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

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

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

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

-- Steve