Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
On Tue, 17 Nov 2020 18:08:19 -0500 (EST) Mathieu Desnoyers wrote: > Because of this end-of-loop condition ^ > which is also testing for a NULL func. So if we reach a stub, we end up > stopping > iteration and not firing the following tracepoint probes. Ah right. OK, since it's looking like we're going to have to modify the tracepoint macro anyway, I'll just go with the 1UL approach. -- Steve
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
- On Nov 17, 2020, at 5:19 PM, rostedt rost...@goodmis.org wrote:
> On Tue, 17 Nov 2020 13:33:42 -0800
> Kees Cook wrote:
>
>> As I think got discussed in the thread, what you had here wouldn't work
>> in a CFI build if the function prototype of the call site and the
>> function don't match. (Though I can't tell if .func() is ever called?)
>>
>> i.e. .func's prototype must match tp_stub_func()'s.
>>
>
>
> Hmm, I wonder how you handle tracepoints? This is called here:
>
> include/linux/tracepoint.h:
>
>
> #define DEFINE_TRACE_FN(_name, _reg, _unreg, proto, args) \
> static const char __tpstrtab_##_name[] \
> __section("__tracepoints_strings") = #_name;\
> extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> int __traceiter_##_name(void *__data, proto); \
> struct tracepoint __tracepoint_##_name __used \
> __section("__tracepoints") = { \
> .name = __tpstrtab_##_name, \
> .key = STATIC_KEY_INIT_FALSE, \
> .static_call_key = _CALL_KEY(tp_func_##_name), \
> .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> .iterator = &__traceiter_##_name, \
> .regfunc = _reg,\
> .unregfunc = _unreg,\
> .funcs = NULL };\
> __TRACEPOINT_ENTRY(_name); \
> int __traceiter_##_name(void *__data, proto)\
> { \
> struct tracepoint_func *it_func_ptr;\
> void *it_func; \
> \
> it_func_ptr = \
> rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
> do {\
> it_func = (it_func_ptr)->func; \
> __data = (it_func_ptr)->data; \
>
> ((void(*)(void *, proto))(it_func))(__data, args); \
>
> called above
>
> Where args is unique for every tracepoint, but func is simply a void
> pointer.
That being said, the called functions have a prototype which match the
caller prototype exactly. So within the tracepoint internal data structures,
this function pointer is indeed a void pointer, but it is cast to a prototype
matching the callees to perform the calls. I suspect that as long as CFI checks
that caller/callees prototypes are compatible at runtime when the actual
calls happen, this all works fine.
Thanks,
Mathieu
>
> -- Steve
>
>
> } while ((++it_func_ptr)->func);\
> return 0; \
> } \
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
- On Nov 17, 2020, at 5:16 PM, rostedt rost...@goodmis.org wrote:
> On Tue, 17 Nov 2020 16:22:23 -0500 (EST)
> Mathieu Desnoyers wrote:
>
>> If we don't call the stub, then there is no point in having the stub at
>> all, and we should just compare to a constant value, e.g. 0x1UL. As far
>> as I can recall, comparing with a small immediate constant is more efficient
>> than comparing with a loaded value on many architectures.
>
> Why 0x1UL, and not just set it to NULL.
>
> do {\
> it_func = (it_func_ptr)->func; \
> __data = (it_func_ptr)->data; \
> if (likely(it_func))\
> ((void(*)(void *, proto))(it_func))(__data,
> args); \
> } while ((++it_func_ptr)->func);
Because of this end-of-loop condition ^
which is also testing for a NULL func. So if we reach a stub, we end up stopping
iteration and not firing the following tracepoint probes.
Thanks,
Mathieu
>
>
> -- Steve
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
On Tue, 17 Nov 2020 13:33:42 -0800
Kees Cook wrote:
> As I think got discussed in the thread, what you had here wouldn't work
> in a CFI build if the function prototype of the call site and the
> function don't match. (Though I can't tell if .func() is ever called?)
>
> i.e. .func's prototype must match tp_stub_func()'s.
>
Hmm, I wonder how you handle tracepoints? This is called here:
include/linux/tracepoint.h:
#define DEFINE_TRACE_FN(_name, _reg, _unreg, proto, args) \
static const char __tpstrtab_##_name[] \
__section("__tracepoints_strings") = #_name;\
extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
int __traceiter_##_name(void *__data, proto); \
struct tracepoint __tracepoint_##_name __used \
__section("__tracepoints") = { \
.name = __tpstrtab_##_name, \
.key = STATIC_KEY_INIT_FALSE, \
.static_call_key = _CALL_KEY(tp_func_##_name), \
.static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
.iterator = &__traceiter_##_name, \
.regfunc = _reg,\
.unregfunc = _unreg,\
.funcs = NULL };\
__TRACEPOINT_ENTRY(_name); \
int __traceiter_##_name(void *__data, proto)\
{ \
struct tracepoint_func *it_func_ptr;\
void *it_func; \
\
it_func_ptr = \
rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
do {\
it_func = (it_func_ptr)->func; \
__data = (it_func_ptr)->data; \
((void(*)(void *, proto))(it_func))(__data, args); \
called above
Where args is unique for every tracepoint, but func is simply a void
pointer.
-- Steve
} while ((++it_func_ptr)->func);\
return 0; \
} \
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
On Tue, 17 Nov 2020 16:22:23 -0500 (EST)
Mathieu Desnoyers wrote:
> If we don't call the stub, then there is no point in having the stub at
> all, and we should just compare to a constant value, e.g. 0x1UL. As far
> as I can recall, comparing with a small immediate constant is more efficient
> than comparing with a loaded value on many architectures.
Why 0x1UL, and not just set it to NULL.
do {\
it_func = (it_func_ptr)->func; \
__data = (it_func_ptr)->data; \
if (likely(it_func))\
((void(*)(void *, proto))(it_func))(__data,
args); \
} while ((++it_func_ptr)->func);
-- Steve
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
On Mon, Nov 16, 2020 at 05:51:07PM -0500, Steven Rostedt wrote: > [ Kees, I added you because you tend to know about these things. > Is it OK to assign a void func(void) that doesn't do anything and returns > nothing to a function pointer that could be call with parameters? We need > to add stubs for tracepoints when we fail to allocate a new array on > removal of a callback, but the callbacks do have arguments, but the stub > called does not have arguments. > > Matt, Does this patch fix the error your patch was trying to fix? > ] As I think got discussed in the thread, what you had here wouldn't work in a CFI build if the function prototype of the call site and the function don't match. (Though I can't tell if .func() is ever called?) i.e. .func's prototype must match tp_stub_func()'s. -- Kees Cook
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
- On Nov 17, 2020, at 3:58 PM, rostedt rost...@goodmis.org wrote: > On Tue, 17 Nov 2020 15:34:51 -0500 > Steven Rostedt wrote: [...] > If it comes down to not trusting calling a stub, I'll still keep the stub > logic in, and just add the following: If we don't call the stub, then there is no point in having the stub at all, and we should just compare to a constant value, e.g. 0x1UL. As far as I can recall, comparing with a small immediate constant is more efficient than comparing with a loaded value on many architectures. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
- On Nov 17, 2020, at 3:34 PM, rostedt rost...@goodmis.org wrote:
> On Tue, 17 Nov 2020 14:47:20 -0500 (EST)
> Mathieu Desnoyers 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
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
On Tue, 17 Nov 2020 15:34:51 -0500
Steven Rostedt wrote:
> On Tue, 17 Nov 2020 14:47:20 -0500 (EST)
> Mathieu Desnoyers 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.
If it comes down to not trusting calling a stub, I'll still keep the stub
logic in, and just add the following:
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 0f21617f1a66..d50a1a652d61 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -33,6 +33,8 @@ struct trace_eval_map {
#define TRACEPOINT_DEFAULT_PRIO10
+extern void tp_stub_func(void *data, ...);
+
extern struct srcu_struct tracepoint_srcu;
extern int
@@ -310,7 +312,8 @@ static inline struct tracepoint
*tracepoint_ptr_deref(tracepoint_ptr_t *p)
do {\
it_func = (it_func_ptr)->func; \
__data = (it_func_ptr)->data; \
- ((void(*)(void *, proto))(it_func))(__data, args); \
+ if (likely(it_func != tp_stub_func))\
+ ((void(*)(void *, proto))(it_func))(__data,
args); \
} while ((++it_func_ptr)->func);\
return 0; \
} \
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 774b3733cbbe..f3bb0ee478d1 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -54,7 +54,7 @@ struct tp_probes {
};
/* Called in removal of a func but failed to allocate a new tp_funcs */
-static void tp_stub_func(void)
+void tp_stub_func(void *data, ...)
{
return;
}
-- Steve
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 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
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
- On Nov 17, 2020, at 2:21 PM, rostedt rost...@goodmis.org wrote:
> On Tue, 17 Nov 2020 14:15:10 -0500 (EST)
> Mathieu Desnoyers wrote:
>
>
>> diff --git a/include/linux/tracepoint-defs.h
>> b/include/linux/tracepoint-defs.h
>> index e7c2276be33e..e0351bb0b140 100644
>> --- a/include/linux/tracepoint-defs.h
>> +++ b/include/linux/tracepoint-defs.h
>> @@ -38,6 +38,7 @@ struct tracepoint {
>> int (*regfunc)(void);
>> void (*unregfunc)(void);
>> struct tracepoint_func __rcu *funcs;
>> + void *stub_func;
>> };
>>
>> #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
>> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
>> index 0f21617f1a66..b0b805de3779 100644
>> --- a/include/linux/tracepoint.h
>> +++ b/include/linux/tracepoint.h
>> @@ -287,6 +287,9 @@ static inline struct tracepoint
>> *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>> #define DEFINE_TRACE_FN(_name, _reg, _unreg, proto, args) \
>> static const char __tpstrtab_##_name[] \
>> __section("__tracepoints_strings") = #_name;\
>> + static void __cold __tracepoint_stub_func_##_name(void *__data,
>> proto) \
>> + { \
>> + } \
>
> The thing is, tracepoints are already bloated. I do not want to add
> something like this that will unnecessarily add more text.
I've measured the impact of adding these stubs to kernel/sched/core.o, and
it's entirely lost in the code alignment (it effectively adds 0 byte of text
to my build) when using the "cold" attribute.
Over an entire vmlinux, it adds 256 bytes of text overall, for a 0.008% code
size
increase.
Can you measure any significant code size increase with this approach ?
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.
>
> 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."
and
6.7.5.3 Function declarators (including prototypes), item 15:
"For two function types to be compatible, both shall specify compatible return
types.
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. [...]"
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.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
On Tue, 17 Nov 2020 14:15:10 -0500 (EST)
Mathieu Desnoyers wrote:
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index e7c2276be33e..e0351bb0b140 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -38,6 +38,7 @@ struct tracepoint {
> int (*regfunc)(void);
> void (*unregfunc)(void);
> struct tracepoint_func __rcu *funcs;
> + void *stub_func;
> };
>
> #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 0f21617f1a66..b0b805de3779 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -287,6 +287,9 @@ static inline struct tracepoint
> *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> #define DEFINE_TRACE_FN(_name, _reg, _unreg, proto, args) \
> static const char __tpstrtab_##_name[] \
> __section("__tracepoints_strings") = #_name;\
> + static void __cold __tracepoint_stub_func_##_name(void *__data,
> proto) \
> + { \
> + } \
The thing is, tracepoints are already bloated. I do not want to add
something like this that will unnecessarily add more text.
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!
-- Steve
> extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> int __traceiter_##_name(void *__data, proto); \
> struct tracepoint __tracepoint_##_name __used \
> @@ -298,7 +301,8 @@ static inline struct tracepoint
> *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> .iterator = &__traceiter_##_name, \
> .regfunc = _reg,\
> .unregfunc = _unreg,\
> - .funcs = NULL };\
> + .funcs = NULL, \
> + .stub_func = __tracepoint_stub_func_##_name, }; \
> __TRACEPOINT_ENTRY(_name); \
> int __traceiter_##_name(void *__data, proto)\
> { \
>
> Thanks,
>
> Mathieu
>
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
- On Nov 16, 2020, at 5:51 PM, rostedt rost...@goodmis.org wrote:
> [ Kees, I added you because you tend to know about these things.
> Is it OK to assign a void func(void) that doesn't do anything and returns
> nothing to a function pointer that could be call with parameters? We need
> to add stubs for tracepoints when we fail to allocate a new array on
> removal of a callback, but the callbacks do have arguments, but the stub
> called does not have arguments.
[...]
> +/* Called in removal of a func but failed to allocate a new tp_funcs */
> +static void tp_stub_func(void)
> +{
> + return;
> +}
> +
In C, the "void" unnamed function parameter specifies that the function has no
parameters. C99 section 6.7.5.3 Function declarators (including prototypes):
"The special case of an unnamed parameter of type void as the only item in the
list
specifies that the function has no parameters."
The C99 standard section "6.5.2.2 Function calls" states:
"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."
"J.2 Undefined behavior" states:
"For a call to a function without a function prototype in scope, the number of
arguments does not equal the number of parameters (6.5.2.2)."
I suspect that calling a function expecting no parameters from a call site with
parameters might work for the cdecl calling convention because the caller
is responsible for stack cleanup, but it seems to rely on a behavior which is
very much tied to the calling convention, and within "undefined behavior"
territory
as far as the C standard is concerned.
I checked whether going for "void tp_stub_func()" instead would work better,
but it seems to also mean "no parameter" when applied to the function
definition.
It's only when applied on the function declarator that is not part of the
definition
that it means no information about the number or type of parameters is supplied.
(see C99 "6.7.5.3 Function declarators (including prototypes)" items 14 and 15)
And the kernel builds with "-Werror=strict-prototypes" which would not allow
it anyway.
One thing which would work with respect to the C standard is to define one stub
function per tracepoint. This add 16 bytes of code per TRACE_DEFINE() on x86-64,
but by specifying that those are cache-cold with "__cold", I notice that it adds
no extra code size my build of kernel/sched/core.o which contains 30 tracepoint
definitions.
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index e7c2276be33e..e0351bb0b140 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -38,6 +38,7 @@ struct tracepoint {
int (*regfunc)(void);
void (*unregfunc)(void);
struct tracepoint_func __rcu *funcs;
+ void *stub_func;
};
#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 0f21617f1a66..b0b805de3779 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -287,6 +287,9 @@ static inline struct tracepoint
*tracepoint_ptr_deref(tracepoint_ptr_t *p)
#define DEFINE_TRACE_FN(_name, _reg, _unreg, proto, args) \
static const char __tpstrtab_##_name[] \
__section("__tracepoints_strings") = #_name;\
+ static void __cold __tracepoint_stub_func_##_name(void *__data, proto) \
+ { \
+ } \
extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
int __traceiter_##_name(void *__data, proto); \
struct tracepoint __tracepoint_##_name __used \
@@ -298,7 +301,8 @@ static inline struct tracepoint
*tracepoint_ptr_deref(tracepoint_ptr_t *p)
.iterator = &__traceiter_##_name, \
.regfunc = _reg,\
.unregfunc = _unreg,\
- .funcs = NULL };\
+ .funcs = NULL, \
+ .stub_func = __tracepoint_stub_func_##_name, }; \
__TRACEPOINT_ENTRY(_name); \
int __traceiter_##_name(void *__data, proto)\
{ \
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Re: [PATCH] tracepoint: Do not fail unregistering a probe due to memory allocation
On Mon, 16 Nov 2020 17:51:07 -0500
Steven Rostedt wrote:
> [ Kees, I added you because you tend to know about these things.
> Is it OK to assign a void func(void) that doesn't do anything and returns
> nothing to a function pointer that could be call with parameters? We need
> to add stubs for tracepoints when we fail to allocate a new array on
> removal of a callback, but the callbacks do have arguments, but the stub
> called does not have arguments.
>
> Matt, Does this patch fix the error your patch was trying to fix?
> ]
>
> The list of tracepoint callbacks is managed by an array that is protected
> by RCU. To update this array, a new array is allocated, the updates are
> copied over to the new array, and then the list of functions for the
> tracepoint is switched over to the new array. After a completion of an RCU
> grace period, the old array is freed.
>
> This process happens for both adding a callback as well as removing one.
> But on removing a callback, if the new array fails to be allocated, the
> callback is not removed, and may be used after it is freed by the clients
> of the tracepoint.
>
> There's really no reason to fail if the allocation for a new array fails
> when removing a function. Instead, the function can simply be replaced by a
> stub function that could be cleaned up on the next modification of the
> array. That is, instead of calling the function registered to the
> tracepoint, it would call a stub function in its place.
>
> Link: https://lore.kernel.org/r/20201115055256.65625-1-mmull...@mmlx.us
>
> Cc: sta...@vger.kernel.org
> Fixes: 97e1c18e8d17b ("tracing: Kernel Tracepoints")
> Reported-by: syzbot+83aa762ef23b6f0d1...@syzkaller.appspotmail.com
> Reported-by: syzbot+d29e58bb557324e55...@syzkaller.appspotmail.com
> Reported-by: Matt Mullins
Forgot my:
Signed-off-by: Steven Rostedt (VMware)
and tested with adding this (just to see if paths are hit).
-- Steve
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 774b3733cbbe..96f081ff5284 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -167,6 +167,7 @@ func_add(struct tracepoint_func **funcs, struct
tracepoint_func *tp_func,
/* Need to copy one at a time to remove stubs */
int probes = 0;
+ printk("HERE stub_funcs=%d\n", stub_funcs);
pos = -1;
for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
if (old[nr_probes].func == tp_stub_func)
@@ -235,7 +236,7 @@ static void *func_remove(struct tracepoint_func **funcs,
int j = 0;
/* N -> M, (N > 1, M > 0) */
/* + 1 for NULL */
- new = allocate_probes(nr_probes - nr_del + 1, __GFP_NOFAIL);
+ new = NULL; //allocate_probes(nr_probes - nr_del + 1,
__GFP_NOFAIL);
if (new) {
for (i = 0; old[i].func; i++)
if ((old[i].func != tp_func->func
