netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
	Matt Mullins <mmullins@mmlx.us>, Ingo Molnar <mingo@redhat.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Dmitry Vyukov <dvyukov@google.com>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>, netdev <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>, Kees Cook <keescook@chromium.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
Date: Tue, 17 Nov 2020 15:34:51 -0500	[thread overview]
Message-ID: <20201117153451.3015c5c9@gandalf.local.home> (raw)
In-Reply-To: <375636043.48251.1605642440621.JavaMail.zimbra@efficios.com>

On Tue, 17 Nov 2020 14:47:20 -0500 (EST)
Mathieu Desnoyers <mathieu.desnoyers@efficios.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

  reply	other threads:[~2020-11-17 20:34 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-16 22:51 [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation Steven Rostedt
2020-11-16 23:16 ` Steven Rostedt
2020-11-17 19:15 ` Mathieu Desnoyers
2020-11-17 19:21   ` Steven Rostedt
2020-11-17 19:47     ` Mathieu Desnoyers
2020-11-17 20:34       ` Steven Rostedt [this message]
2020-11-17 20:58         ` Steven Rostedt
2020-11-17 21:22           ` Mathieu Desnoyers
2020-11-17 22:16             ` Steven Rostedt
2020-11-17 23:08               ` Mathieu Desnoyers
2020-11-18  1:11                 ` Steven Rostedt
2020-11-17 21:08         ` Mathieu Desnoyers
2020-11-18 13:21         ` violating function pointer signature Peter Zijlstra
2020-11-18 13:59           ` Florian Weimer
2020-11-18 14:12             ` Peter Zijlstra
2020-11-18 14:18               ` Florian Weimer
2020-11-18 14:34                 ` [PATCH v3] tracepoint: Do not fail unregistering a probe due to memory allocation Steven Rostedt
2020-11-24  5:59                   ` Matt Mullins
2020-11-18 14:22             ` violating function pointer signature Steven Rostedt
2020-11-18 19:46               ` Alexei Starovoitov
2020-11-18 20:02                 ` Steven Rostedt
2020-11-18 14:02           ` Steven Rostedt
2020-11-18 16:01             ` Mathieu Desnoyers
2020-11-18 16:19               ` David Laight
2020-11-18 16:50           ` Nick Desaulniers
2020-11-18 17:17             ` Steven Rostedt
2020-11-18 18:12               ` Segher Boessenkool
2020-11-18 18:31                 ` Florian Weimer
2020-11-18 18:55                   ` Segher Boessenkool
2020-11-18 18:58                   ` Steven Rostedt
2020-11-18 18:59                     ` Steven Rostedt
2020-11-18 19:11                     ` Segher Boessenkool
2020-11-18 19:33                       ` Steven Rostedt
2020-11-18 19:48                         ` Segher Boessenkool
2020-11-18 20:44                           ` Steven Rostedt
2020-11-19  8:21                           ` Peter Zijlstra
2020-11-19  8:36                       ` Peter Zijlstra
2020-11-19 14:37                         ` Segher Boessenkool
2020-11-19 14:59                           ` Steven Rostedt
2020-11-19 16:35                             ` Segher Boessenkool
2020-11-19 17:42                               ` David Laight
2020-11-19 19:27                                 ` Segher Boessenkool
2020-11-19 17:04                             ` Alexei Starovoitov
2020-11-19 17:30                               ` Steven Rostedt
2020-11-20  1:31                               ` Nick Desaulniers
2020-11-17 21:33 ` [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation Kees Cook
2020-11-17 22:19   ` Steven Rostedt
2020-11-17 23:12     ` Mathieu Desnoyers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201117153451.3015c5c9@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dvyukov@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jpoimboe@redhat.com \
    --cc=kafai@fb.com \
    --cc=keescook@chromium.org \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mingo@redhat.com \
    --cc=mmullins@mmlx.us \
    --cc=netdev@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).