When CLANG_CFI is in use, using tracing will occasionally result in
CFI failures, e.g.
| CFI failure at syscall_trace_enter+0x66c/0x7d0 (target: tp_stub_func+0x0/0x2c; expected type: 0x4877830c)
| Internal error: Oops - CFI: 00000000f200823a [#1] PREEMPT SMP
| CPU: 2 PID: 118 Comm: klogd Not tainted 6.3.0-rc3-00002-gc242ea6f2f98 #2
| Hardware name: linux,dummy-virt (DT)
| pstate: 30400005 (nzCV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
| pc : syscall_trace_enter+0x66c/0x7d0
| lr : syscall_trace_enter+0x5e8/0x7d0
| sp : ffff800015ce7d80
| x29: ffff800015ce7d80 x28: ffff000017538000 x27: 0000000000000003
| x26: ffff8000084c9454 x25: ffff00001182bd10 x24: dfff800000000000
| x23: 1fffe00002ea7001 x22: ffff00001182bd10 x21: ffff000017538008
| x20: ffff000017538000 x19: ffff800015ce7eb0 x18: 0000000000000000
| x17: 000000004877830c x16: 00000000a540670c x15: 0000000000000000
| x14: 0000000000000000 x13: 0000000000000000 x12: ff80800008039d8c
| x11: ff80800008039dd0 x10: 0000000000000000 x9 : 0000000000000000
| x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000
| x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
| x2 : 00000000000000ce x1 : ffff800015ce7eb0 x0 : ffff800013d55000
| Call trace:
| syscall_trace_enter+0x66c/0x7d0
| el0_svc_common+0x1dc/0x268
| do_el0_svc+0x70/0x1a4
| el0_svc+0x58/0x14c
| el0t_64_sync_handler+0x84/0xf0
| el0t_64_sync+0x190/0x194
| Code: 72906191 72a90ef1 6b11021f 54000040 (d4304740)
| ---[ end trace 0000000000000000 ]---
This happens because the function prototype of tp_sub_func doesn't match
the prototype of the tracepoint function. As each tracepoint may have a
distinct prototype, it's not possible to share a common stub function.
Avoid this by creating a stub function for each tracepoint with the
correct prototype, and using these rather than tp_stub_func.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Masami Hiramatsu <[email protected]
---
include/linux/tracepoint-defs.h | 1 +
include/linux/tracepoint.h | 5 +++++
kernel/tracepoint.c | 31 ++++++++++++++-----------------
3 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index e7c2276be33eb..a306ac7255220 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -35,6 +35,7 @@ struct tracepoint {
struct static_call_key *static_call_key;
void *static_call_tramp;
void *iterator;
+ void *stub;
int (*regfunc)(void);
void (*unregfunc)(void);
struct tracepoint_func __rcu *funcs;
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 6811e43c1b5c2..1640926441910 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
__section("__tracepoints_strings") = #_name; \
extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
int __traceiter_##_name(void *__data, proto); \
+ void __tracestub_##_name(void *, proto); \
struct tracepoint __tracepoint_##_name __used \
__section("__tracepoints") = { \
.name = __tpstrtab_##_name, \
@@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
.static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
.static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
.iterator = &__traceiter_##_name, \
+ .stub = &__tracestub_##_name, \
.regfunc = _reg, \
.unregfunc = _unreg, \
.funcs = NULL }; \
@@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
} \
return 0; \
} \
+ void __tracestub_##_name(void *__data, proto) \
+ { \
+ } \
DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);
#define DEFINE_TRACE(name, proto, args) \
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 8d1507dd07246..d9186629a0095 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -98,12 +98,6 @@ struct tp_probes {
struct tracepoint_func probes[];
};
-/* Called in removal of a func but failed to allocate a new tp_funcs */
-static void tp_stub_func(void)
-{
- return;
-}
-
static inline void *allocate_probes(int count)
{
struct tp_probes *p = kmalloc(struct_size(p, probes, count),
@@ -177,13 +171,15 @@ static void debug_print_probes(struct tracepoint_func *funcs)
}
static struct tracepoint_func *
-func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
+func_add(struct tracepoint *tp, struct tracepoint_func **funcs,
+ struct tracepoint_func *tp_func,
int prio)
{
struct tracepoint_func *old, *new;
int iter_probes; /* Iterate over old probe array. */
int nr_probes = 0; /* Counter for probes */
int pos = -1; /* Insertion position into new array */
+ void *stub_func = tp->stub;
if (WARN_ON(!tp_func->func))
return ERR_PTR(-EINVAL);
@@ -193,7 +189,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
if (old) {
/* (N -> N+1), (N != 0, 1) probes */
for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
- if (old[iter_probes].func == tp_stub_func)
+ if (old[iter_probes].func == stub_func)
continue; /* Skip stub functions. */
if (old[iter_probes].func == tp_func->func &&
old[iter_probes].data == tp_func->data)
@@ -208,7 +204,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
if (old) {
nr_probes = 0;
for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
- if (old[iter_probes].func == tp_stub_func)
+ if (old[iter_probes].func == stub_func)
continue;
/* Insert before probes of lower priority */
if (pos < 0 && old[iter_probes].prio < prio)
@@ -229,11 +225,12 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
return old;
}
-static void *func_remove(struct tracepoint_func **funcs,
- struct tracepoint_func *tp_func)
+static void *func_remove(struct tracepoint *tp, struct tracepoint_func **funcs,
+ struct tracepoint_func *tp_func)
{
int nr_probes = 0, nr_del = 0, i;
struct tracepoint_func *old, *new;
+ void *stub_func = tp->stub;
old = *funcs;
@@ -246,7 +243,7 @@ static void *func_remove(struct tracepoint_func **funcs,
for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
if ((old[nr_probes].func == tp_func->func &&
old[nr_probes].data == tp_func->data) ||
- old[nr_probes].func == tp_stub_func)
+ old[nr_probes].func == stub_func)
nr_del++;
}
}
@@ -269,7 +266,7 @@ static void *func_remove(struct tracepoint_func **funcs,
for (i = 0; old[i].func; i++) {
if ((old[i].func != tp_func->func ||
old[i].data != tp_func->data) &&
- old[i].func != tp_stub_func)
+ old[i].func != stub_func)
new[j++] = old[i];
}
new[nr_probes - nr_del].func = NULL;
@@ -277,12 +274,12 @@ static void *func_remove(struct tracepoint_func **funcs,
} else {
/*
* Failed to allocate, replace the old function
- * with calls to tp_stub_func.
+ * with calls to stub_func.
*/
for (i = 0; old[i].func; i++) {
if (old[i].func == tp_func->func &&
old[i].data == tp_func->data)
- WRITE_ONCE(old[i].func, tp_stub_func);
+ WRITE_ONCE(old[i].func, stub_func);
}
*funcs = old;
}
@@ -335,7 +332,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
tp_funcs = rcu_dereference_protected(tp->funcs,
lockdep_is_held(&tracepoints_mutex));
- old = func_add(&tp_funcs, func, prio);
+ old = func_add(tp, &tp_funcs, func, prio);
if (IS_ERR(old)) {
WARN_ON_ONCE(warn && PTR_ERR(old) != -ENOMEM);
return PTR_ERR(old);
@@ -400,7 +397,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
tp_funcs = rcu_dereference_protected(tp->funcs,
lockdep_is_held(&tracepoints_mutex));
- old = func_remove(&tp_funcs, func);
+ old = func_remove(tp, &tp_funcs, func);
if (WARN_ON_ONCE(IS_ERR(old)))
return PTR_ERR(old);
--
2.30.2
On Thu, 23 Mar 2023 11:40:12 +0000
Mark Rutland <[email protected]> wrote:
> When CLANG_CFI is in use, using tracing will occasionally result in
> CFI failures, e.g.
>
> | CFI failure at syscall_trace_enter+0x66c/0x7d0 (target: tp_stub_func+0x0/0x2c; expected type: 0x4877830c)
> | Internal error: Oops - CFI: 00000000f200823a [#1] PREEMPT SMP
> | CPU: 2 PID: 118 Comm: klogd Not tainted 6.3.0-rc3-00002-gc242ea6f2f98 #2
> | Hardware name: linux,dummy-virt (DT)
> | pstate: 30400005 (nzCV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> | pc : syscall_trace_enter+0x66c/0x7d0
> | lr : syscall_trace_enter+0x5e8/0x7d0
> | sp : ffff800015ce7d80
> | x29: ffff800015ce7d80 x28: ffff000017538000 x27: 0000000000000003
> | x26: ffff8000084c9454 x25: ffff00001182bd10 x24: dfff800000000000
> | x23: 1fffe00002ea7001 x22: ffff00001182bd10 x21: ffff000017538008
> | x20: ffff000017538000 x19: ffff800015ce7eb0 x18: 0000000000000000
> | x17: 000000004877830c x16: 00000000a540670c x15: 0000000000000000
> | x14: 0000000000000000 x13: 0000000000000000 x12: ff80800008039d8c
> | x11: ff80800008039dd0 x10: 0000000000000000 x9 : 0000000000000000
> | x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000
> | x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> | x2 : 00000000000000ce x1 : ffff800015ce7eb0 x0 : ffff800013d55000
> | Call trace:
> | syscall_trace_enter+0x66c/0x7d0
> | el0_svc_common+0x1dc/0x268
> | do_el0_svc+0x70/0x1a4
> | el0_svc+0x58/0x14c
> | el0t_64_sync_handler+0x84/0xf0
> | el0t_64_sync+0x190/0x194
> | Code: 72906191 72a90ef1 6b11021f 54000040 (d4304740)
> | ---[ end trace 0000000000000000 ]---
>
> This happens because the function prototype of tp_sub_func doesn't match
> the prototype of the tracepoint function. As each tracepoint may have a
> distinct prototype, it's not possible to share a common stub function.
I hate C!
>
> Avoid this by creating a stub function for each tracepoint with the
> correct prototype, and using these rather than tp_stub_func.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Masami Hiramatsu <[email protected]
> ---
> include/linux/tracepoint-defs.h | 1 +
> include/linux/tracepoint.h | 5 +++++
> kernel/tracepoint.c | 31 ++++++++++++++-----------------
> 3 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index e7c2276be33eb..a306ac7255220 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -35,6 +35,7 @@ struct tracepoint {
> struct static_call_key *static_call_key;
> void *static_call_tramp;
> void *iterator;
> + void *stub;
> int (*regfunc)(void);
> void (*unregfunc)(void);
> struct tracepoint_func __rcu *funcs;
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 6811e43c1b5c2..1640926441910 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> __section("__tracepoints_strings") = #_name; \
> extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> int __traceiter_##_name(void *__data, proto); \
> + void __tracestub_##_name(void *, proto); \
> struct tracepoint __tracepoint_##_name __used \
> __section("__tracepoints") = { \
> .name = __tpstrtab_##_name, \
> @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
> .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> .iterator = &__traceiter_##_name, \
> + .stub = &__tracestub_##_name, \
> .regfunc = _reg, \
> .unregfunc = _unreg, \
> .funcs = NULL }; \
> @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> } \
> return 0; \
> } \
> + void __tracestub_##_name(void *__data, proto) \
> + { \
> + } \
I purposely did not do this because this adds over a thousand stub
functions! It adds one for *every* tracepoint (and that is a superset of
trace events).
Is there some other way we could do this?
C really really needs a way to make a generic void do_nothing(...) function!
I added some compiler folks to the Cc to hear our grievances.
-- Steve
> DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);
>
> #define DEFINE_TRACE(name, proto, args) \
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 8d1507dd07246..d9186629a0095 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -98,12 +98,6 @@ struct tp_probes {
> struct tracepoint_func probes[];
> };
>
> -/* Called in removal of a func but failed to allocate a new tp_funcs */
> -static void tp_stub_func(void)
> -{
> - return;
> -}
> -
> static inline void *allocate_probes(int count)
> {
> struct tp_probes *p = kmalloc(struct_size(p, probes, count),
> @@ -177,13 +171,15 @@ static void debug_print_probes(struct tracepoint_func *funcs)
> }
>
> static struct tracepoint_func *
> -func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> +func_add(struct tracepoint *tp, struct tracepoint_func **funcs,
> + struct tracepoint_func *tp_func,
> int prio)
> {
> struct tracepoint_func *old, *new;
> int iter_probes; /* Iterate over old probe array. */
> int nr_probes = 0; /* Counter for probes */
> int pos = -1; /* Insertion position into new array */
> + void *stub_func = tp->stub;
>
> if (WARN_ON(!tp_func->func))
> return ERR_PTR(-EINVAL);
> @@ -193,7 +189,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> if (old) {
> /* (N -> N+1), (N != 0, 1) probes */
> for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> - if (old[iter_probes].func == tp_stub_func)
> + if (old[iter_probes].func == stub_func)
> continue; /* Skip stub functions. */
> if (old[iter_probes].func == tp_func->func &&
> old[iter_probes].data == tp_func->data)
> @@ -208,7 +204,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> if (old) {
> nr_probes = 0;
> for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> - if (old[iter_probes].func == tp_stub_func)
> + if (old[iter_probes].func == stub_func)
> continue;
> /* Insert before probes of lower priority */
> if (pos < 0 && old[iter_probes].prio < prio)
> @@ -229,11 +225,12 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> return old;
> }
>
> -static void *func_remove(struct tracepoint_func **funcs,
> - struct tracepoint_func *tp_func)
> +static void *func_remove(struct tracepoint *tp, struct tracepoint_func **funcs,
> + struct tracepoint_func *tp_func)
> {
> int nr_probes = 0, nr_del = 0, i;
> struct tracepoint_func *old, *new;
> + void *stub_func = tp->stub;
>
> old = *funcs;
>
> @@ -246,7 +243,7 @@ static void *func_remove(struct tracepoint_func **funcs,
> for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> if ((old[nr_probes].func == tp_func->func &&
> old[nr_probes].data == tp_func->data) ||
> - old[nr_probes].func == tp_stub_func)
> + old[nr_probes].func == stub_func)
> nr_del++;
> }
> }
> @@ -269,7 +266,7 @@ static void *func_remove(struct tracepoint_func **funcs,
> for (i = 0; old[i].func; i++) {
> if ((old[i].func != tp_func->func ||
> old[i].data != tp_func->data) &&
> - old[i].func != tp_stub_func)
> + old[i].func != stub_func)
> new[j++] = old[i];
> }
> new[nr_probes - nr_del].func = NULL;
> @@ -277,12 +274,12 @@ static void *func_remove(struct tracepoint_func **funcs,
> } else {
> /*
> * Failed to allocate, replace the old function
> - * with calls to tp_stub_func.
> + * with calls to stub_func.
> */
> for (i = 0; old[i].func; i++) {
> if (old[i].func == tp_func->func &&
> old[i].data == tp_func->data)
> - WRITE_ONCE(old[i].func, tp_stub_func);
> + WRITE_ONCE(old[i].func, stub_func);
> }
> *funcs = old;
> }
> @@ -335,7 +332,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
>
> tp_funcs = rcu_dereference_protected(tp->funcs,
> lockdep_is_held(&tracepoints_mutex));
> - old = func_add(&tp_funcs, func, prio);
> + old = func_add(tp, &tp_funcs, func, prio);
> if (IS_ERR(old)) {
> WARN_ON_ONCE(warn && PTR_ERR(old) != -ENOMEM);
> return PTR_ERR(old);
> @@ -400,7 +397,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
>
> tp_funcs = rcu_dereference_protected(tp->funcs,
> lockdep_is_held(&tracepoints_mutex));
> - old = func_remove(&tp_funcs, func);
> + old = func_remove(tp, &tp_funcs, func);
> if (WARN_ON_ONCE(IS_ERR(old)))
> return PTR_ERR(old);
>
[adding Sami and Peter]
On Thu, Mar 23, 2023 at 08:53:21AM -0400, Steven Rostedt wrote:
> On Thu, 23 Mar 2023 11:40:12 +0000
> Mark Rutland <[email protected]> wrote:
>
> > When CLANG_CFI is in use, using tracing will occasionally result in
> > CFI failures, e.g.
> >
> > | CFI failure at syscall_trace_enter+0x66c/0x7d0 (target: tp_stub_func+0x0/0x2c; expected type: 0x4877830c)
> > | Internal error: Oops - CFI: 00000000f200823a [#1] PREEMPT SMP
> > | CPU: 2 PID: 118 Comm: klogd Not tainted 6.3.0-rc3-00002-gc242ea6f2f98 #2
> > | Hardware name: linux,dummy-virt (DT)
> > | pstate: 30400005 (nzCV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > | pc : syscall_trace_enter+0x66c/0x7d0
> > | lr : syscall_trace_enter+0x5e8/0x7d0
> > | sp : ffff800015ce7d80
> > | x29: ffff800015ce7d80 x28: ffff000017538000 x27: 0000000000000003
> > | x26: ffff8000084c9454 x25: ffff00001182bd10 x24: dfff800000000000
> > | x23: 1fffe00002ea7001 x22: ffff00001182bd10 x21: ffff000017538008
> > | x20: ffff000017538000 x19: ffff800015ce7eb0 x18: 0000000000000000
> > | x17: 000000004877830c x16: 00000000a540670c x15: 0000000000000000
> > | x14: 0000000000000000 x13: 0000000000000000 x12: ff80800008039d8c
> > | x11: ff80800008039dd0 x10: 0000000000000000 x9 : 0000000000000000
> > | x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000
> > | x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> > | x2 : 00000000000000ce x1 : ffff800015ce7eb0 x0 : ffff800013d55000
> > | Call trace:
> > | syscall_trace_enter+0x66c/0x7d0
> > | el0_svc_common+0x1dc/0x268
> > | do_el0_svc+0x70/0x1a4
> > | el0_svc+0x58/0x14c
> > | el0t_64_sync_handler+0x84/0xf0
> > | el0t_64_sync+0x190/0x194
> > | Code: 72906191 72a90ef1 6b11021f 54000040 (d4304740)
> > | ---[ end trace 0000000000000000 ]---
> >
> > This happens because the function prototype of tp_sub_func doesn't match
> > the prototype of the tracepoint function. As each tracepoint may have a
> > distinct prototype, it's not possible to share a common stub function.
>
> I hate C!
>
> >
> > Avoid this by creating a stub function for each tracepoint with the
> > correct prototype, and using these rather than tp_stub_func.
> >
> > Signed-off-by: Mark Rutland <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Masami Hiramatsu <[email protected]
> > ---
> > include/linux/tracepoint-defs.h | 1 +
> > include/linux/tracepoint.h | 5 +++++
> > kernel/tracepoint.c | 31 ++++++++++++++-----------------
> > 3 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> > index e7c2276be33eb..a306ac7255220 100644
> > --- a/include/linux/tracepoint-defs.h
> > +++ b/include/linux/tracepoint-defs.h
> > @@ -35,6 +35,7 @@ struct tracepoint {
> > struct static_call_key *static_call_key;
> > void *static_call_tramp;
> > void *iterator;
> > + void *stub;
> > int (*regfunc)(void);
> > void (*unregfunc)(void);
> > struct tracepoint_func __rcu *funcs;
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 6811e43c1b5c2..1640926441910 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > __section("__tracepoints_strings") = #_name; \
> > extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> > int __traceiter_##_name(void *__data, proto); \
> > + void __tracestub_##_name(void *, proto); \
> > struct tracepoint __tracepoint_##_name __used \
> > __section("__tracepoints") = { \
> > .name = __tpstrtab_##_name, \
> > @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
> > .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> > .iterator = &__traceiter_##_name, \
> > + .stub = &__tracestub_##_name, \
> > .regfunc = _reg, \
> > .unregfunc = _unreg, \
> > .funcs = NULL }; \
> > @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > } \
> > return 0; \
> > } \
> > + void __tracestub_##_name(void *__data, proto) \
> > + { \
> > + } \
>
> I purposely did not do this because this adds over a thousand stub
> functions! It adds one for *every* tracepoint (and that is a superset of
> trace events).
>
> Is there some other way we could do this?
>
> C really really needs a way to make a generic void do_nothing(...) function!
>
> I added some compiler folks to the Cc to hear our grievances.
I pulled in Sami, who did much of the kCFI work, and PeterZ too...
We can't have a generic function that's compatible will all function
prototypes, since that mechanism would undermine the CFI scheme. Either callers
would always have to omit the check, or we're have to have a special "always
compatible" type hash, and both would be gigantic targets for attack.
Can we avoid the stub entirely? e.g. make hte call conditional on the func
pointer not being some bad value (e.g. like the error pointers?). That way we
could avoid the call, and we wouldn't need the stub implementation.
Another option would be to make the tracepoint function prototypes *actually*
compatible. If each tracepoint had something like:
struct tracepoint_##_name##_data {
unsigned long arg;
void *ptr;
char whatver[];
};
... then we could have tracepoint functions consistently take a void pointer to
that, and extract the args. We can wrap that in something like the syscall
wrappers so that we don't have to write that manually.
... or maybe we can somehow drop the stubs into a magic section and deduplicate
them?
Thanks.
Mark.
>
> -- Steve
>
>
> > DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);
> >
> > #define DEFINE_TRACE(name, proto, args) \
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 8d1507dd07246..d9186629a0095 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> > @@ -98,12 +98,6 @@ struct tp_probes {
> > struct tracepoint_func probes[];
> > };
> >
> > -/* Called in removal of a func but failed to allocate a new tp_funcs */
> > -static void tp_stub_func(void)
> > -{
> > - return;
> > -}
> > -
> > static inline void *allocate_probes(int count)
> > {
> > struct tp_probes *p = kmalloc(struct_size(p, probes, count),
> > @@ -177,13 +171,15 @@ static void debug_print_probes(struct tracepoint_func *funcs)
> > }
> >
> > static struct tracepoint_func *
> > -func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> > +func_add(struct tracepoint *tp, struct tracepoint_func **funcs,
> > + struct tracepoint_func *tp_func,
> > int prio)
> > {
> > struct tracepoint_func *old, *new;
> > int iter_probes; /* Iterate over old probe array. */
> > int nr_probes = 0; /* Counter for probes */
> > int pos = -1; /* Insertion position into new array */
> > + void *stub_func = tp->stub;
> >
> > if (WARN_ON(!tp_func->func))
> > return ERR_PTR(-EINVAL);
> > @@ -193,7 +189,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> > if (old) {
> > /* (N -> N+1), (N != 0, 1) probes */
> > for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> > - if (old[iter_probes].func == tp_stub_func)
> > + if (old[iter_probes].func == stub_func)
> > continue; /* Skip stub functions. */
> > if (old[iter_probes].func == tp_func->func &&
> > old[iter_probes].data == tp_func->data)
> > @@ -208,7 +204,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> > if (old) {
> > nr_probes = 0;
> > for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> > - if (old[iter_probes].func == tp_stub_func)
> > + if (old[iter_probes].func == stub_func)
> > continue;
> > /* Insert before probes of lower priority */
> > if (pos < 0 && old[iter_probes].prio < prio)
> > @@ -229,11 +225,12 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> > return old;
> > }
> >
> > -static void *func_remove(struct tracepoint_func **funcs,
> > - struct tracepoint_func *tp_func)
> > +static void *func_remove(struct tracepoint *tp, struct tracepoint_func **funcs,
> > + struct tracepoint_func *tp_func)
> > {
> > int nr_probes = 0, nr_del = 0, i;
> > struct tracepoint_func *old, *new;
> > + void *stub_func = tp->stub;
> >
> > old = *funcs;
> >
> > @@ -246,7 +243,7 @@ static void *func_remove(struct tracepoint_func **funcs,
> > for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> > if ((old[nr_probes].func == tp_func->func &&
> > old[nr_probes].data == tp_func->data) ||
> > - old[nr_probes].func == tp_stub_func)
> > + old[nr_probes].func == stub_func)
> > nr_del++;
> > }
> > }
> > @@ -269,7 +266,7 @@ static void *func_remove(struct tracepoint_func **funcs,
> > for (i = 0; old[i].func; i++) {
> > if ((old[i].func != tp_func->func ||
> > old[i].data != tp_func->data) &&
> > - old[i].func != tp_stub_func)
> > + old[i].func != stub_func)
> > new[j++] = old[i];
> > }
> > new[nr_probes - nr_del].func = NULL;
> > @@ -277,12 +274,12 @@ static void *func_remove(struct tracepoint_func **funcs,
> > } else {
> > /*
> > * Failed to allocate, replace the old function
> > - * with calls to tp_stub_func.
> > + * with calls to stub_func.
> > */
> > for (i = 0; old[i].func; i++) {
> > if (old[i].func == tp_func->func &&
> > old[i].data == tp_func->data)
> > - WRITE_ONCE(old[i].func, tp_stub_func);
> > + WRITE_ONCE(old[i].func, stub_func);
> > }
> > *funcs = old;
> > }
> > @@ -335,7 +332,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
> >
> > tp_funcs = rcu_dereference_protected(tp->funcs,
> > lockdep_is_held(&tracepoints_mutex));
> > - old = func_add(&tp_funcs, func, prio);
> > + old = func_add(tp, &tp_funcs, func, prio);
> > if (IS_ERR(old)) {
> > WARN_ON_ONCE(warn && PTR_ERR(old) != -ENOMEM);
> > return PTR_ERR(old);
> > @@ -400,7 +397,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
> >
> > tp_funcs = rcu_dereference_protected(tp->funcs,
> > lockdep_is_held(&tracepoints_mutex));
> > - old = func_remove(&tp_funcs, func);
> > + old = func_remove(tp, &tp_funcs, func);
> > if (WARN_ON_ONCE(IS_ERR(old)))
> > return PTR_ERR(old);
> >
>
On Thu, Mar 23, 2023 at 01:45:34PM +0000, Mark Rutland wrote:
> On Thu, Mar 23, 2023 at 08:53:21AM -0400, Steven Rostedt wrote:
> > On Thu, 23 Mar 2023 11:40:12 +0000
> > Mark Rutland <[email protected]> wrote:
> > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > index 6811e43c1b5c2..1640926441910 100644
> > > --- a/include/linux/tracepoint.h
> > > +++ b/include/linux/tracepoint.h
> > > @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > __section("__tracepoints_strings") = #_name; \
> > > extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> > > int __traceiter_##_name(void *__data, proto); \
> > > + void __tracestub_##_name(void *, proto); \
> > > struct tracepoint __tracepoint_##_name __used \
> > > __section("__tracepoints") = { \
> > > .name = __tpstrtab_##_name, \
> > > @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
> > > .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> > > .iterator = &__traceiter_##_name, \
> > > + .stub = &__tracestub_##_name, \
> > > .regfunc = _reg, \
> > > .unregfunc = _unreg, \
> > > .funcs = NULL }; \
> > > @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > } \
> > > return 0; \
> > > } \
> > > + void __tracestub_##_name(void *__data, proto) \
> > > + { \
> > > + } \
> >
> > I purposely did not do this because this adds over a thousand stub
> > functions! It adds one for *every* tracepoint (and that is a superset of
> > trace events).
> >
> > Is there some other way we could do this?
> >
> > C really really needs a way to make a generic void do_nothing(...) function!
> >
> > I added some compiler folks to the Cc to hear our grievances.
>
> I pulled in Sami, who did much of the kCFI work, and PeterZ too...
>
> We can't have a generic function that's compatible will all function
> prototypes, since that mechanism would undermine the CFI scheme. Either callers
> would always have to omit the check, or we're have to have a special "always
> compatible" type hash, and both would be gigantic targets for attack.
>
> Can we avoid the stub entirely? e.g. make hte call conditional on the func
> pointer not being some bad value (e.g. like the error pointers?). That way we
> could avoid the call, and we wouldn't need the stub implementation.
Along those lines (and as Peter also suggested over IRC), would something like
the below be preferable?
Mark.
---->8----
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 6811e43c1b5c..b8017e906049 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -33,6 +33,8 @@ struct trace_eval_map {
#define TRACEPOINT_DEFAULT_PRIO 10
+void tp_stub_func(void);
+
extern struct srcu_struct tracepoint_srcu;
extern int
@@ -324,6 +326,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
if (it_func_ptr) { \
do { \
it_func = READ_ONCE((it_func_ptr)->func); \
+ if (it_func == tp_stub_func) \
+ continue; \
__data = (it_func_ptr)->data; \
((void(*)(void *, proto))(it_func))(__data, args); \
} while ((++it_func_ptr)->func); \
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 8d1507dd0724..dcf5a637429f 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -99,7 +99,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)
{
return;
}
On Thu, 23 Mar 2023 08:53:21 -0400
Steven Rostedt <[email protected]> wrote:
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > __section("__tracepoints_strings") = #_name; \
> > extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> > int __traceiter_##_name(void *__data, proto); \
> > + void __tracestub_##_name(void *, proto); \
> > struct tracepoint __tracepoint_##_name __used \
> > __section("__tracepoints") = { \
> > .name = __tpstrtab_##_name, \
> > @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
> > .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> > .iterator = &__traceiter_##_name, \
> > + .stub = &__tracestub_##_name, \
> > .regfunc = _reg, \
> > .unregfunc = _unreg, \
> > .funcs = NULL }; \
> > @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > } \
> > return 0; \
> > } \
> > + void __tracestub_##_name(void *__data, proto) \
> > + { \
> > + } \
>
> I purposely did not do this because this adds over a thousand stub
> functions! It adds one for *every* tracepoint (and that is a superset of
> trace events).
And the commit that added this code:
befe6d946551 ("tracepoint: Do not fail unregistering a probe due to memory failure")
Has this in the change log:
[ Note, this version does use undefined compiler behavior (assuming that
a stub function with no parameters or return, can be called by a location
that thinks it has parameters but still no return value. Static calls
do the same thing, so this trick is not without precedent.
There's another solution that uses RCU tricks and is more complex, but
can be an alternative if this solution becomes an issue.
Link: https://lore.kernel.org/lkml/[email protected]/
]
-- Steve
On Thu, Mar 23, 2023 at 12:26:50PM -0400, Steven Rostedt wrote:
> On Thu, 23 Mar 2023 08:53:21 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > > --- a/include/linux/tracepoint.h
> > > +++ b/include/linux/tracepoint.h
> > > @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > __section("__tracepoints_strings") = #_name; \
> > > extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> > > int __traceiter_##_name(void *__data, proto); \
> > > + void __tracestub_##_name(void *, proto); \
> > > struct tracepoint __tracepoint_##_name __used \
> > > __section("__tracepoints") = { \
> > > .name = __tpstrtab_##_name, \
> > > @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
> > > .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> > > .iterator = &__traceiter_##_name, \
> > > + .stub = &__tracestub_##_name, \
> > > .regfunc = _reg, \
> > > .unregfunc = _unreg, \
> > > .funcs = NULL }; \
> > > @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > } \
> > > return 0; \
> > > } \
> > > + void __tracestub_##_name(void *__data, proto) \
> > > + { \
> > > + } \
> >
> > I purposely did not do this because this adds over a thousand stub
> > functions! It adds one for *every* tracepoint (and that is a superset of
> > trace events).
>
> And the commit that added this code:
>
> befe6d946551 ("tracepoint: Do not fail unregistering a probe due to memory failure")
>
> Has this in the change log:
>
> [ Note, this version does use undefined compiler behavior (assuming that
> a stub function with no parameters or return, can be called by a location
> that thinks it has parameters but still no return value. Static calls
> do the same thing, so this trick is not without precedent.
>
> There's another solution that uses RCU tricks and is more complex, but
> can be an alternative if this solution becomes an issue.
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> ]
FWIW, I'd be happy with that approach too -- we just happened to race with our
last replies. :)
Mark.
On Thu, 23 Mar 2023 16:27:37 +0000
Mark Rutland <[email protected]> wrote:
> On Thu, Mar 23, 2023 at 01:45:34PM +0000, Mark Rutland wrote:
> > On Thu, Mar 23, 2023 at 08:53:21AM -0400, Steven Rostedt wrote:
> > > On Thu, 23 Mar 2023 11:40:12 +0000
> > > Mark Rutland <[email protected]> wrote:
>
> > > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > > index 6811e43c1b5c2..1640926441910 100644
> > > > --- a/include/linux/tracepoint.h
> > > > +++ b/include/linux/tracepoint.h
> > > > @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > > __section("__tracepoints_strings") = #_name; \
> > > > extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
> > > > int __traceiter_##_name(void *__data, proto); \
> > > > + void __tracestub_##_name(void *, proto); \
> > > > struct tracepoint __tracepoint_##_name __used \
> > > > __section("__tracepoints") = { \
> > > > .name = __tpstrtab_##_name, \
> > > > @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > > .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
> > > > .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> > > > .iterator = &__traceiter_##_name, \
> > > > + .stub = &__tracestub_##_name, \
> > > > .regfunc = _reg, \
> > > > .unregfunc = _unreg, \
> > > > .funcs = NULL }; \
> > > > @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> > > > } \
> > > > return 0; \
> > > > } \
> > > > + void __tracestub_##_name(void *__data, proto) \
> > > > + { \
> > > > + } \
> > >
> > > I purposely did not do this because this adds over a thousand stub
> > > functions! It adds one for *every* tracepoint (and that is a superset of
> > > trace events).
> > >
> > > Is there some other way we could do this?
> > >
> > > C really really needs a way to make a generic void do_nothing(...) function!
> > >
> > > I added some compiler folks to the Cc to hear our grievances.
> >
> > I pulled in Sami, who did much of the kCFI work, and PeterZ too...
> >
> > We can't have a generic function that's compatible will all function
> > prototypes, since that mechanism would undermine the CFI scheme. Either callers
> > would always have to omit the check, or we're have to have a special "always
> > compatible" type hash, and both would be gigantic targets for attack.
> >
> > Can we avoid the stub entirely? e.g. make hte call conditional on the func
> > pointer not being some bad value (e.g. like the error pointers?). That way we
> > could avoid the call, and we wouldn't need the stub implementation.
>
> Along those lines (and as Peter also suggested over IRC), would something like
> the below be preferable?
So we had this discussion a while ago:
See befe6d946551 ("tracepoint: Do not fail unregistering a probe due to memory failure")
Where I believe one answer was to use NULL instead of a stub.
I have to go back and re-read that thread. Mathieu was involved with all
this too.
And as I mentioned in my other reply. There was a more complex solution
that could handle this if the stub solution ended up being an issue.
Repeated again so Mathieu doesn't have to search for it.
[ Note, this version does use undefined compiler behavior (assuming that
a stub function with no parameters or return, can be called by a location
that thinks it has parameters but still no return value. Static calls
do the same thing, so this trick is not without precedent.
There's another solution that uses RCU tricks and is more complex, but
can be an alternative if this solution becomes an issue.
Link: https://lore.kernel.org/lkml/[email protected]/
]
-- Steve
>
> Mark.
>
> ---->8----
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 6811e43c1b5c..b8017e906049 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -33,6 +33,8 @@ struct trace_eval_map {
>
> #define TRACEPOINT_DEFAULT_PRIO 10
>
> +void tp_stub_func(void);
> +
> extern struct srcu_struct tracepoint_srcu;
>
> extern int
> @@ -324,6 +326,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> if (it_func_ptr) { \
> do { \
> it_func = READ_ONCE((it_func_ptr)->func); \
> + if (it_func == tp_stub_func) \
> + continue; \
> __data = (it_func_ptr)->data; \
> ((void(*)(void *, proto))(it_func))(__data, args); \
> } while ((++it_func_ptr)->func); \
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 8d1507dd0724..dcf5a637429f 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -99,7 +99,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)
> {
> return;
> }
On 2023-03-23 12:34, Steven Rostedt wrote:
> On Thu, 23 Mar 2023 16:27:37 +0000
> Mark Rutland <[email protected]> wrote:
>
>> On Thu, Mar 23, 2023 at 01:45:34PM +0000, Mark Rutland wrote:
>>> On Thu, Mar 23, 2023 at 08:53:21AM -0400, Steven Rostedt wrote:
>>>> On Thu, 23 Mar 2023 11:40:12 +0000
>>>> Mark Rutland <[email protected]> wrote:
>>
>>>>> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
>>>>> index 6811e43c1b5c2..1640926441910 100644
>>>>> --- a/include/linux/tracepoint.h
>>>>> +++ b/include/linux/tracepoint.h
>>>>> @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>>>>> __section("__tracepoints_strings") = #_name; \
>>>>> extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name); \
>>>>> int __traceiter_##_name(void *__data, proto); \
>>>>> + void __tracestub_##_name(void *, proto); \
>>>>> struct tracepoint __tracepoint_##_name __used \
>>>>> __section("__tracepoints") = { \
>>>>> .name = __tpstrtab_##_name, \
>>>>> @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>>>>> .static_call_key = &STATIC_CALL_KEY(tp_func_##_name), \
>>>>> .static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
>>>>> .iterator = &__traceiter_##_name, \
>>>>> + .stub = &__tracestub_##_name, \
>>>>> .regfunc = _reg, \
>>>>> .unregfunc = _unreg, \
>>>>> .funcs = NULL }; \
>>>>> @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>>>>> } \
>>>>> return 0; \
>>>>> } \
>>>>> + void __tracestub_##_name(void *__data, proto) \
>>>>> + { \
>>>>> + } \
>>>>
>>>> I purposely did not do this because this adds over a thousand stub
>>>> functions! It adds one for *every* tracepoint (and that is a superset of
>>>> trace events).
>>>>
>>>> Is there some other way we could do this?
>>>>
>>>> C really really needs a way to make a generic void do_nothing(...) function!
>>>>
>>>> I added some compiler folks to the Cc to hear our grievances.
>>>
>>> I pulled in Sami, who did much of the kCFI work, and PeterZ too...
>>>
>>> We can't have a generic function that's compatible will all function
>>> prototypes, since that mechanism would undermine the CFI scheme. Either callers
>>> would always have to omit the check, or we're have to have a special "always
>>> compatible" type hash, and both would be gigantic targets for attack.
>>>
>>> Can we avoid the stub entirely? e.g. make hte call conditional on the func
>>> pointer not being some bad value (e.g. like the error pointers?). That way we
>>> could avoid the call, and we wouldn't need the stub implementation.
>>
>> Along those lines (and as Peter also suggested over IRC), would something like
>> the below be preferable?
>
> So we had this discussion a while ago:
>
> See befe6d946551 ("tracepoint: Do not fail unregistering a probe due to memory failure")
>
> Where I believe one answer was to use NULL instead of a stub.
>
> I have to go back and re-read that thread. Mathieu was involved with all
> this too.
>
> And as I mentioned in my other reply. There was a more complex solution
> that could handle this if the stub solution ended up being an issue.
>
> Repeated again so Mathieu doesn't have to search for it.
>
> [ Note, this version does use undefined compiler behavior (assuming that
> a stub function with no parameters or return, can be called by a location
> that thinks it has parameters but still no return value. Static calls
> do the same thing, so this trick is not without precedent.
>
> There's another solution that uses RCU tricks and is more complex, but
> can be an alternative if this solution becomes an issue.
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> ]
Ugh. The last thing we need there is more RCU complexity. My brain is still recovering
from fixing the last time the introduction of static calls special-cases ended up subtly
breaking tracepoints.
>
> -- Steve
>
>
>>
>> Mark.
>>
>> ---->8----
>> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
>> index 6811e43c1b5c..b8017e906049 100644
>> --- a/include/linux/tracepoint.h
>> +++ b/include/linux/tracepoint.h
>> @@ -33,6 +33,8 @@ struct trace_eval_map {
>>
>> #define TRACEPOINT_DEFAULT_PRIO 10
>>
>> +void tp_stub_func(void);
>> +
>> extern struct srcu_struct tracepoint_srcu;
>>
>> extern int
>> @@ -324,6 +326,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>> if (it_func_ptr) { \
>> do { \
>> it_func = READ_ONCE((it_func_ptr)->func); \
>> + if (it_func == tp_stub_func) \
>> + continue; \
Along the lines of my earlier proposal:
https://lore.kernel.org/all/[email protected]/
We could reserve (void *)0x1 as a stub value rather than adding an explicit stub function
if we end up skipping the call anyway. The check could be:
#define TP_STUB_FN ((void *)0x1)
if (unlikely(it_func == TP_STUB_FN))
continue;
And we would only need that check for CONFIG_HAVE_STATIC_CALL=y right ?
Thanks,
Mathieu
>> __data = (it_func_ptr)->data; \
>> ((void(*)(void *, proto))(it_func))(__data, args); \
>> } while ((++it_func_ptr)->func); \
>> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
>> index 8d1507dd0724..dcf5a637429f 100644
>> --- a/kernel/tracepoint.c
>> +++ b/kernel/tracepoint.c
>> @@ -99,7 +99,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)
>> {
>> return;
>> }
>
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On Thu, 23 Mar 2023 14:37:15 -0400
Mathieu Desnoyers <[email protected]> wrote:
> > Repeated again so Mathieu doesn't have to search for it.
> >
> > [ Note, this version does use undefined compiler behavior (assuming that
> > a stub function with no parameters or return, can be called by a location
> > that thinks it has parameters but still no return value. Static calls
> > do the same thing, so this trick is not without precedent.
> >
> > There's another solution that uses RCU tricks and is more complex, but
> > can be an alternative if this solution becomes an issue.
> >
> > Link: https://lore.kernel.org/lkml/[email protected]/
> > ]
>
> Ugh. The last thing we need there is more RCU complexity. My brain is still recovering
> from fixing the last time the introduction of static calls special-cases ended up subtly
> breaking tracepoints.
Well, we can go back to your approach with doing the check in the iterator.
Setting it to 0x1UL I think is what you wanted (to know it's a removed
function and not a random NULL).
I could write that up if you want.
-- Steve
On 2023-03-23 14:42, Steven Rostedt wrote:
> On Thu, 23 Mar 2023 14:37:15 -0400
> Mathieu Desnoyers <[email protected]> wrote:
>
>>> Repeated again so Mathieu doesn't have to search for it.
>>>
>>> [ Note, this version does use undefined compiler behavior (assuming that
>>> a stub function with no parameters or return, can be called by a location
>>> that thinks it has parameters but still no return value. Static calls
>>> do the same thing, so this trick is not without precedent.
>>>
>>> There's another solution that uses RCU tricks and is more complex, but
>>> can be an alternative if this solution becomes an issue.
>>>
>>> Link: https://lore.kernel.org/lkml/[email protected]/
>>> ]
>>
>> Ugh. The last thing we need there is more RCU complexity. My brain is still recovering
>> from fixing the last time the introduction of static calls special-cases ended up subtly
>> breaking tracepoints.
>
> Well, we can go back to your approach with doing the check in the iterator.
> Setting it to 0x1UL I think is what you wanted (to know it's a removed
> function and not a random NULL).
>
> I could write that up if you want.
I can write the patch and send an RFC, won't take long.
Thanks,
Mathieu
>
> -- Steve
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com