linux-trace-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
@ 2024-11-07 12:25 Marco Elver
  2024-11-07 12:25 ` [PATCH v2 2/2] tracing: Remove pid in task_rename tracing output Marco Elver
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Marco Elver @ 2024-11-07 12:25 UTC (permalink / raw)
  To: elver, Steven Rostedt, Kees Cook
  Cc: Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton, Oleg Nesterov,
	linux-kernel, linux-trace-kernel, Dmitry Vyukov, kasan-dev

prctl() is a complex syscall which multiplexes its functionality based
on a large set of PR_* options. Currently we count 64 such options. The
return value of unknown options is -EINVAL, and doesn't distinguish from
known options that were passed invalid args that also return -EINVAL.

To understand if programs are attempting to use prctl() options not yet
available on the running kernel, provide the task_prctl_unknown
tracepoint.

Note, this tracepoint is in an unlikely cold path, and would therefore
be suitable for continuous monitoring (e.g. via perf_event_open).

While the above is likely the simplest usecase, additionally this
tracepoint can help unlock some testing scenarios (where probing
sys_enter or sys_exit causes undesirable performance overheads):

  a. unprivileged triggering of a test module: test modules may register a
     probe to be called back on task_prctl_unknown, and pick a very large
     unknown prctl() option upon which they perform a test function for an
     unprivileged user;

  b. unprivileged triggering of an eBPF program function: similar
     as idea (a).

Example trace_pipe output:

  test-484     [000] .....   631.748104: task_prctl_unknown: comm=test option=1234 arg2=101 arg3=102 arg4=103 arg5=104

Signed-off-by: Marco Elver <elver@google.com>
---
v2:
* Remove "pid" in trace output (suggested by Steven).
---
 include/trace/events/task.h | 41 +++++++++++++++++++++++++++++++++++++
 kernel/sys.c                |  3 +++
 2 files changed, 44 insertions(+)

diff --git a/include/trace/events/task.h b/include/trace/events/task.h
index 47b527464d1a..9202cb2524c4 100644
--- a/include/trace/events/task.h
+++ b/include/trace/events/task.h
@@ -56,6 +56,47 @@ TRACE_EVENT(task_rename,
 		__entry->newcomm, __entry->oom_score_adj)
 );
 
+/**
+ * task_prctl_unknown - called on unknown prctl() option
+ * @task:	pointer to the current task
+ * @option:	option passed
+ * @arg2:	arg2 passed
+ * @arg3:	arg3 passed
+ * @arg4:	arg4 passed
+ * @arg5:	arg5 passed
+ *
+ * Called on an unknown prctl() option.
+ */
+TRACE_EVENT(task_prctl_unknown,
+
+	TP_PROTO(struct task_struct *task, int option, unsigned long arg2, unsigned long arg3,
+		 unsigned long arg4, unsigned long arg5),
+
+	TP_ARGS(task, option, arg2, arg3, arg4, arg5),
+
+	TP_STRUCT__entry(
+		__string(	comm,		task->comm	)
+		__field(	int,		option)
+		__field(	unsigned long,	arg2)
+		__field(	unsigned long,	arg3)
+		__field(	unsigned long,	arg4)
+		__field(	unsigned long,	arg5)
+	),
+
+	TP_fast_assign(
+		__assign_str(comm);
+		__entry->option = option;
+		__entry->arg2 = arg2;
+		__entry->arg3 = arg3;
+		__entry->arg4 = arg4;
+		__entry->arg5 = arg5;
+	),
+
+	TP_printk("comm=%s option=%d arg2=%ld arg3=%ld arg4=%ld arg5=%ld",
+		  __get_str(comm), __entry->option,
+		  __entry->arg2, __entry->arg3, __entry->arg4, __entry->arg5)
+);
+
 #endif
 
 /* This part must be outside protection */
diff --git a/kernel/sys.c b/kernel/sys.c
index 4da31f28fda8..dd0a71b68558 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -75,6 +75,8 @@
 #include <asm/io.h>
 #include <asm/unistd.h>
 
+#include <trace/events/task.h>
+
 #include "uid16.h"
 
 #ifndef SET_UNALIGN_CTL
@@ -2785,6 +2787,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
 		break;
 	default:
+		trace_task_prctl_unknown(me, option, arg2, arg3, arg4, arg5);
 		error = -EINVAL;
 		break;
 	}
-- 
2.47.0.199.ga7371fff76-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] tracing: Remove pid in task_rename tracing output
  2024-11-07 12:25 [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Marco Elver
@ 2024-11-07 12:25 ` Marco Elver
  2024-11-07 15:34 ` [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Steven Rostedt
  2024-11-07 15:44 ` Mathieu Desnoyers
  2 siblings, 0 replies; 11+ messages in thread
From: Marco Elver @ 2024-11-07 12:25 UTC (permalink / raw)
  To: elver, Steven Rostedt, Kees Cook
  Cc: Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton, Oleg Nesterov,
	linux-kernel, linux-trace-kernel, Dmitry Vyukov, kasan-dev

Remove pid in task_rename tracepoint output, since that tracepoint only
deals with the current task, and is printed by default. This also saves
some space in the entry and avoids wasted padding.

Link: https://lkml.kernel.org/r/20241105120247.596a0dc9@gandalf.local.home
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Marco Elver <elver@google.com>
---
v2:
* New patch
---
 include/trace/events/task.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/task.h b/include/trace/events/task.h
index 9202cb2524c4..ee202aafa9fd 100644
--- a/include/trace/events/task.h
+++ b/include/trace/events/task.h
@@ -38,22 +38,19 @@ TRACE_EVENT(task_rename,
 	TP_ARGS(task, comm),
 
 	TP_STRUCT__entry(
-		__field(	pid_t,	pid)
 		__array(	char, oldcomm,  TASK_COMM_LEN)
 		__array(	char, newcomm,  TASK_COMM_LEN)
 		__field(	short,	oom_score_adj)
 	),
 
 	TP_fast_assign(
-		__entry->pid = task->pid;
 		memcpy(entry->oldcomm, task->comm, TASK_COMM_LEN);
 		strscpy(entry->newcomm, comm, TASK_COMM_LEN);
 		__entry->oom_score_adj = task->signal->oom_score_adj;
 	),
 
-	TP_printk("pid=%d oldcomm=%s newcomm=%s oom_score_adj=%hd",
-		__entry->pid, __entry->oldcomm,
-		__entry->newcomm, __entry->oom_score_adj)
+	TP_printk("oldcomm=%s newcomm=%s oom_score_adj=%hd",
+		  __entry->oldcomm, __entry->newcomm, __entry->oom_score_adj)
 );
 
 /**
-- 
2.47.0.199.ga7371fff76-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 12:25 [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Marco Elver
  2024-11-07 12:25 ` [PATCH v2 2/2] tracing: Remove pid in task_rename tracing output Marco Elver
@ 2024-11-07 15:34 ` Steven Rostedt
  2024-11-07 15:46   ` Marco Elver
  2024-11-07 15:44 ` Mathieu Desnoyers
  2 siblings, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2024-11-07 15:34 UTC (permalink / raw)
  To: Marco Elver
  Cc: Kees Cook, Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On Thu,  7 Nov 2024 13:25:47 +0100
Marco Elver <elver@google.com> wrote:

> +/**
> + * task_prctl_unknown - called on unknown prctl() option
> + * @task:	pointer to the current task
> + * @option:	option passed
> + * @arg2:	arg2 passed
> + * @arg3:	arg3 passed
> + * @arg4:	arg4 passed
> + * @arg5:	arg5 passed
> + *
> + * Called on an unknown prctl() option.
> + */
> +TRACE_EVENT(task_prctl_unknown,
> +
> +	TP_PROTO(struct task_struct *task, int option, unsigned long arg2, unsigned long arg3,
> +		 unsigned long arg4, unsigned long arg5),
> +
> +	TP_ARGS(task, option, arg2, arg3, arg4, arg5),
> +
> +	TP_STRUCT__entry(
> +		__string(	comm,		task->comm	)

The question is, do we really need comm? From your example, it's redundant:

  test-484     [000] .....   631.748104: task_prctl_unknown: comm=test option=1234 arg2=101 arg3=102 arg4=103 arg5=104
  ^^^^                                                            ^^^^

-- Steve


> +		__field(	int,		option)
> +		__field(	unsigned long,	arg2)
> +		__field(	unsigned long,	arg3)
> +		__field(	unsigned long,	arg4)
> +		__field(	unsigned long,	arg5)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(comm);
> +		__entry->option = option;
> +		__entry->arg2 = arg2;
> +		__entry->arg3 = arg3;
> +		__entry->arg4 = arg4;
> +		__entry->arg5 = arg5;
> +	),
> +
> +	TP_printk("comm=%s option=%d arg2=%ld arg3=%ld arg4=%ld arg5=%ld",
> +		  __get_str(comm), __entry->option,
> +		  __entry->arg2, __entry->arg3, __entry->arg4, __entry->arg5)
> +);
> +

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 12:25 [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Marco Elver
  2024-11-07 12:25 ` [PATCH v2 2/2] tracing: Remove pid in task_rename tracing output Marco Elver
  2024-11-07 15:34 ` [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Steven Rostedt
@ 2024-11-07 15:44 ` Mathieu Desnoyers
  2024-11-07 15:46   ` Marco Elver
  2 siblings, 1 reply; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-11-07 15:44 UTC (permalink / raw)
  To: Marco Elver, Steven Rostedt, Kees Cook
  Cc: Masami Hiramatsu, Andrew Morton, Oleg Nesterov, linux-kernel,
	linux-trace-kernel, Dmitry Vyukov, kasan-dev

On 2024-11-07 07:25, Marco Elver wrote:
> prctl() is a complex syscall which multiplexes its functionality based
> on a large set of PR_* options. Currently we count 64 such options. The
> return value of unknown options is -EINVAL, and doesn't distinguish from
> known options that were passed invalid args that also return -EINVAL.
> 
> To understand if programs are attempting to use prctl() options not yet
> available on the running kernel, provide the task_prctl_unknown
> tracepoint.
> 
> Note, this tracepoint is in an unlikely cold path, and would therefore
> be suitable for continuous monitoring (e.g. via perf_event_open).
> 
> While the above is likely the simplest usecase, additionally this
> tracepoint can help unlock some testing scenarios (where probing
> sys_enter or sys_exit causes undesirable performance overheads):
> 
>    a. unprivileged triggering of a test module: test modules may register a
>       probe to be called back on task_prctl_unknown, and pick a very large
>       unknown prctl() option upon which they perform a test function for an
>       unprivileged user;
> 
>    b. unprivileged triggering of an eBPF program function: similar
>       as idea (a).
> 
> Example trace_pipe output:
> 
>    test-484     [000] .....   631.748104: task_prctl_unknown: comm=test option=1234 arg2=101 arg3=102 arg4=103 arg5=104
> 

My concern is that we start adding tons of special-case
tracepoints to the implementation of system calls which
are redundant with the sys_enter/exit tracepoints.

Why favor this approach rather than hooking on sys_enter/exit ?

Thanks,

Mathieu

> Signed-off-by: Marco Elver <elver@google.com>
> ---
> v2:
> * Remove "pid" in trace output (suggested by Steven).
> ---
>   include/trace/events/task.h | 41 +++++++++++++++++++++++++++++++++++++
>   kernel/sys.c                |  3 +++
>   2 files changed, 44 insertions(+)
> 
> diff --git a/include/trace/events/task.h b/include/trace/events/task.h
> index 47b527464d1a..9202cb2524c4 100644
> --- a/include/trace/events/task.h
> +++ b/include/trace/events/task.h
> @@ -56,6 +56,47 @@ TRACE_EVENT(task_rename,
>   		__entry->newcomm, __entry->oom_score_adj)
>   );
>   
> +/**
> + * task_prctl_unknown - called on unknown prctl() option
> + * @task:	pointer to the current task
> + * @option:	option passed
> + * @arg2:	arg2 passed
> + * @arg3:	arg3 passed
> + * @arg4:	arg4 passed
> + * @arg5:	arg5 passed
> + *
> + * Called on an unknown prctl() option.
> + */
> +TRACE_EVENT(task_prctl_unknown,
> +
> +	TP_PROTO(struct task_struct *task, int option, unsigned long arg2, unsigned long arg3,
> +		 unsigned long arg4, unsigned long arg5),
> +
> +	TP_ARGS(task, option, arg2, arg3, arg4, arg5),
> +
> +	TP_STRUCT__entry(
> +		__string(	comm,		task->comm	)
> +		__field(	int,		option)
> +		__field(	unsigned long,	arg2)
> +		__field(	unsigned long,	arg3)
> +		__field(	unsigned long,	arg4)
> +		__field(	unsigned long,	arg5)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(comm);
> +		__entry->option = option;
> +		__entry->arg2 = arg2;
> +		__entry->arg3 = arg3;
> +		__entry->arg4 = arg4;
> +		__entry->arg5 = arg5;
> +	),
> +
> +	TP_printk("comm=%s option=%d arg2=%ld arg3=%ld arg4=%ld arg5=%ld",
> +		  __get_str(comm), __entry->option,
> +		  __entry->arg2, __entry->arg3, __entry->arg4, __entry->arg5)
> +);
> +
>   #endif
>   
>   /* This part must be outside protection */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 4da31f28fda8..dd0a71b68558 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -75,6 +75,8 @@
>   #include <asm/io.h>
>   #include <asm/unistd.h>
>   
> +#include <trace/events/task.h>
> +
>   #include "uid16.h"
>   
>   #ifndef SET_UNALIGN_CTL
> @@ -2785,6 +2787,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>   		error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
>   		break;
>   	default:
> +		trace_task_prctl_unknown(me, option, arg2, arg3, arg4, arg5);
>   		error = -EINVAL;
>   		break;
>   	}

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 15:44 ` Mathieu Desnoyers
@ 2024-11-07 15:46   ` Marco Elver
  2024-11-07 15:52     ` Steven Rostedt
  2024-11-07 15:52     ` Mathieu Desnoyers
  0 siblings, 2 replies; 11+ messages in thread
From: Marco Elver @ 2024-11-07 15:46 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Steven Rostedt, Kees Cook, Masami Hiramatsu, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On Thu, 7 Nov 2024 at 16:45, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2024-11-07 07:25, Marco Elver wrote:
> > prctl() is a complex syscall which multiplexes its functionality based
> > on a large set of PR_* options. Currently we count 64 such options. The
> > return value of unknown options is -EINVAL, and doesn't distinguish from
> > known options that were passed invalid args that also return -EINVAL.
> >
> > To understand if programs are attempting to use prctl() options not yet
> > available on the running kernel, provide the task_prctl_unknown
> > tracepoint.
> >
> > Note, this tracepoint is in an unlikely cold path, and would therefore
> > be suitable for continuous monitoring (e.g. via perf_event_open).
> >
> > While the above is likely the simplest usecase, additionally this
> > tracepoint can help unlock some testing scenarios (where probing
> > sys_enter or sys_exit causes undesirable performance overheads):
> >
> >    a. unprivileged triggering of a test module: test modules may register a
> >       probe to be called back on task_prctl_unknown, and pick a very large
> >       unknown prctl() option upon which they perform a test function for an
> >       unprivileged user;
> >
> >    b. unprivileged triggering of an eBPF program function: similar
> >       as idea (a).
> >
> > Example trace_pipe output:
> >
> >    test-484     [000] .....   631.748104: task_prctl_unknown: comm=test option=1234 arg2=101 arg3=102 arg4=103 arg5=104
> >
>
> My concern is that we start adding tons of special-case
> tracepoints to the implementation of system calls which
> are redundant with the sys_enter/exit tracepoints.
>
> Why favor this approach rather than hooking on sys_enter/exit ?

It's __extremely__ expensive when deployed at scale. See note in
commit description above.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 15:34 ` [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Steven Rostedt
@ 2024-11-07 15:46   ` Marco Elver
  0 siblings, 0 replies; 11+ messages in thread
From: Marco Elver @ 2024-11-07 15:46 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Kees Cook, Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On Thu, 7 Nov 2024 at 16:34, Steven Rostedt <rostedt@goodmis.org> wrote:
...
> > +     TP_PROTO(struct task_struct *task, int option, unsigned long arg2, unsigned long arg3,
> > +              unsigned long arg4, unsigned long arg5),
> > +
> > +     TP_ARGS(task, option, arg2, arg3, arg4, arg5),
> > +
> > +     TP_STRUCT__entry(
> > +             __string(       comm,           task->comm      )
>
> The question is, do we really need comm? From your example, it's redundant:
>
>   test-484     [000] .....   631.748104: task_prctl_unknown: comm=test option=1234 arg2=101 arg3=102 arg4=103 arg5=104
>   ^^^^                                                            ^^^^

Ack, let's remove it. I will also remove the "task" argument.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 15:46   ` Marco Elver
@ 2024-11-07 15:52     ` Steven Rostedt
  2024-11-07 15:52     ` Mathieu Desnoyers
  1 sibling, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2024-11-07 15:52 UTC (permalink / raw)
  To: Marco Elver
  Cc: Mathieu Desnoyers, Kees Cook, Masami Hiramatsu, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On Thu, 7 Nov 2024 16:46:47 +0100
Marco Elver <elver@google.com> wrote:

> > My concern is that we start adding tons of special-case
> > tracepoints to the implementation of system calls which
> > are redundant with the sys_enter/exit tracepoints.
> >
> > Why favor this approach rather than hooking on sys_enter/exit ?  
> 
> It's __extremely__ expensive when deployed at scale. See note in
> commit description above.

Agreed. The sys_enter/exit trace events make all syscalls go the slow path,
which can be quite expensive.

-- Steve

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 15:46   ` Marco Elver
  2024-11-07 15:52     ` Steven Rostedt
@ 2024-11-07 15:52     ` Mathieu Desnoyers
  2024-11-07 15:57       ` Marco Elver
  2024-11-07 16:04       ` Steven Rostedt
  1 sibling, 2 replies; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-11-07 15:52 UTC (permalink / raw)
  To: Marco Elver
  Cc: Steven Rostedt, Kees Cook, Masami Hiramatsu, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On 2024-11-07 10:46, Marco Elver wrote:
> On Thu, 7 Nov 2024 at 16:45, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> On 2024-11-07 07:25, Marco Elver wrote:
>>> prctl() is a complex syscall which multiplexes its functionality based
>>> on a large set of PR_* options. Currently we count 64 such options. The
>>> return value of unknown options is -EINVAL, and doesn't distinguish from
>>> known options that were passed invalid args that also return -EINVAL.
>>>
>>> To understand if programs are attempting to use prctl() options not yet
>>> available on the running kernel, provide the task_prctl_unknown
>>> tracepoint.
>>>
>>> Note, this tracepoint is in an unlikely cold path, and would therefore
>>> be suitable for continuous monitoring (e.g. via perf_event_open).
>>>
>>> While the above is likely the simplest usecase, additionally this
>>> tracepoint can help unlock some testing scenarios (where probing
>>> sys_enter or sys_exit causes undesirable performance overheads):
>>>
>>>     a. unprivileged triggering of a test module: test modules may register a
>>>        probe to be called back on task_prctl_unknown, and pick a very large
>>>        unknown prctl() option upon which they perform a test function for an
>>>        unprivileged user;
>>>
>>>     b. unprivileged triggering of an eBPF program function: similar
>>>        as idea (a).
>>>
>>> Example trace_pipe output:
>>>
>>>     test-484     [000] .....   631.748104: task_prctl_unknown: comm=test option=1234 arg2=101 arg3=102 arg4=103 arg5=104
>>>
>>
>> My concern is that we start adding tons of special-case
>> tracepoints to the implementation of system calls which
>> are redundant with the sys_enter/exit tracepoints.
>>
>> Why favor this approach rather than hooking on sys_enter/exit ?
> 
> It's __extremely__ expensive when deployed at scale. See note in
> commit description above.

I suspect you base the overhead analysis on the x86-64 implementation
of sys_enter/exit tracepoint and especially the overhead caused by
the SYSCALL_WORK_SYSCALL_TRACEPOINT thread flag, am I correct ?

If that is causing a too large overhead, we should investigate if
those can be improved instead of adding tracepoints in the
implementation of system calls.

Thanks,

Mathieu


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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 15:52     ` Mathieu Desnoyers
@ 2024-11-07 15:57       ` Marco Elver
  2024-11-07 16:04       ` Steven Rostedt
  1 sibling, 0 replies; 11+ messages in thread
From: Marco Elver @ 2024-11-07 15:57 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Steven Rostedt, Kees Cook, Masami Hiramatsu, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On Thu, 7 Nov 2024 at 16:54, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> On 2024-11-07 10:46, Marco Elver wrote:
> > On Thu, 7 Nov 2024 at 16:45, Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> >>
> >> On 2024-11-07 07:25, Marco Elver wrote:
> >>> prctl() is a complex syscall which multiplexes its functionality based
> >>> on a large set of PR_* options. Currently we count 64 such options. The
> >>> return value of unknown options is -EINVAL, and doesn't distinguish from
> >>> known options that were passed invalid args that also return -EINVAL.
> >>>
> >>> To understand if programs are attempting to use prctl() options not yet
> >>> available on the running kernel, provide the task_prctl_unknown
> >>> tracepoint.
> >>>
> >>> Note, this tracepoint is in an unlikely cold path, and would therefore
> >>> be suitable for continuous monitoring (e.g. via perf_event_open).
> >>>
> >>> While the above is likely the simplest usecase, additionally this
> >>> tracepoint can help unlock some testing scenarios (where probing
> >>> sys_enter or sys_exit causes undesirable performance overheads):
> >>>
> >>>     a. unprivileged triggering of a test module: test modules may register a
> >>>        probe to be called back on task_prctl_unknown, and pick a very large
> >>>        unknown prctl() option upon which they perform a test function for an
> >>>        unprivileged user;
> >>>
> >>>     b. unprivileged triggering of an eBPF program function: similar
> >>>        as idea (a).
> >>>
> >>> Example trace_pipe output:
> >>>
> >>>     test-484     [000] .....   631.748104: task_prctl_unknown: comm=test option=1234 arg2=101 arg3=102 arg4=103 arg5=104
> >>>
> >>
> >> My concern is that we start adding tons of special-case
> >> tracepoints to the implementation of system calls which
> >> are redundant with the sys_enter/exit tracepoints.
> >>
> >> Why favor this approach rather than hooking on sys_enter/exit ?
> >
> > It's __extremely__ expensive when deployed at scale. See note in
> > commit description above.
>
> I suspect you base the overhead analysis on the x86-64 implementation
> of sys_enter/exit tracepoint and especially the overhead caused by
> the SYSCALL_WORK_SYSCALL_TRACEPOINT thread flag, am I correct ?
>
> If that is causing a too large overhead, we should investigate if
> those can be improved instead of adding tracepoints in the
> implementation of system calls.

Doing that may be generally useful, but even if you improve it
somehow, there's always some additional bit of work needed on
sys_enter/exit as soon as a tracepoint is attached. Even if that's
just a few cycles, it's too much (for me at least).

Also: if you just hook sys_enter/exit, you don't know if the prctl was
handled or not by inspecting the return code (-EINVAL). I want the
kernel to tell me if it handled the prctl() or not, and I also think
it's very bad design to copy-paste the prctl() option checking of the
running kernel in a sys_enter/exit hook. This doesn't scale in terms
of performance nor maintainability.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 15:52     ` Mathieu Desnoyers
  2024-11-07 15:57       ` Marco Elver
@ 2024-11-07 16:04       ` Steven Rostedt
  2024-11-07 16:36         ` Mathieu Desnoyers
  1 sibling, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2024-11-07 16:04 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Marco Elver, Kees Cook, Masami Hiramatsu, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On Thu, 7 Nov 2024 10:52:37 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> I suspect you base the overhead analysis on the x86-64 implementation
> of sys_enter/exit tracepoint and especially the overhead caused by
> the SYSCALL_WORK_SYSCALL_TRACEPOINT thread flag, am I correct ?
> 
> If that is causing a too large overhead, we should investigate if
> those can be improved instead of adding tracepoints in the
> implementation of system calls.

That would be great to get better, but the reason I'm not against this
patch is because prctl() is not a normal system call. It's basically an
ioctl() for Linux, and very vague. It's basically the garbage system call
when you don't know what to do. It's even being proposed for the sframe
work.

I understand your sentiment and agree. I don't want any random system call
to get a tracepoint attached to it. But here I'd make an exception.

-- Steve


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint
  2024-11-07 16:04       ` Steven Rostedt
@ 2024-11-07 16:36         ` Mathieu Desnoyers
  0 siblings, 0 replies; 11+ messages in thread
From: Mathieu Desnoyers @ 2024-11-07 16:36 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Marco Elver, Kees Cook, Masami Hiramatsu, Andrew Morton,
	Oleg Nesterov, linux-kernel, linux-trace-kernel, Dmitry Vyukov,
	kasan-dev

On 2024-11-07 11:04, Steven Rostedt wrote:
> On Thu, 7 Nov 2024 10:52:37 -0500
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
>> I suspect you base the overhead analysis on the x86-64 implementation
>> of sys_enter/exit tracepoint and especially the overhead caused by
>> the SYSCALL_WORK_SYSCALL_TRACEPOINT thread flag, am I correct ?
>>
>> If that is causing a too large overhead, we should investigate if
>> those can be improved instead of adding tracepoints in the
>> implementation of system calls.
> 
> That would be great to get better, but the reason I'm not against this
> patch is because prctl() is not a normal system call. It's basically an
> ioctl() for Linux, and very vague. It's basically the garbage system call
> when you don't know what to do. It's even being proposed for the sframe
> work.
> 
> I understand your sentiment and agree. I don't want any random system call
> to get a tracepoint attached to it. But here I'd make an exception.

Should we document this as an "instrumentation good practice" then ?

     When the system call is a multiplexor such as ioctl(2) and prctl(2),
     then instrumenting it with tracepoints within each of the "op" case
     makes sense for overall maintainability.

     For non-multiplexor system calls, using the existing sys_enter/exit
     tracepoints should be favored.

This opens the following question for non-multiplexors system calls:
considering that the overhead of the current sys_enter/exit
instrumentation is deemed to large to use in production, perhaps
we should consider a few alternatives, namely:

A) Modify SYSCALL_DEFINE so it emits a function wrapper with tracepoints
    for each system call enter/exit, except for multiplexors, or

B) Add the plumbing required to allow system call tracing to be
    activated for specific system calls only, more fine-grained than
    the current system-wide for_each_process_thread()
    SYSCALL_WORK_SYSCALL_TRACEPOINT thread flag big hammer.

Another scenario to consider is system calls that have iovec arguments.
Should we add tracepoint within the iovec iteration, or should it target
the entire iovec as input/output at system call enter/exit ?

Thanks,

Mathieu

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-11-07 16:37 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-07 12:25 [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Marco Elver
2024-11-07 12:25 ` [PATCH v2 2/2] tracing: Remove pid in task_rename tracing output Marco Elver
2024-11-07 15:34 ` [PATCH v2 1/2] tracing: Add task_prctl_unknown tracepoint Steven Rostedt
2024-11-07 15:46   ` Marco Elver
2024-11-07 15:44 ` Mathieu Desnoyers
2024-11-07 15:46   ` Marco Elver
2024-11-07 15:52     ` Steven Rostedt
2024-11-07 15:52     ` Mathieu Desnoyers
2024-11-07 15:57       ` Marco Elver
2024-11-07 16:04       ` Steven Rostedt
2024-11-07 16:36         ` Mathieu Desnoyers

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