linux-kernel - Re: [PATCH v1 2/8] tracing/ftrace: guard syscall probe with preempt

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

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

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

Message-ID: <20241004104503.7a1d6b44@gandalf.local.home>
Date: Fri, 4 Oct 2024 10:45:03 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc: Masami Hiramatsu <mhiramat@...nel.org>, linux-kernel@...r.kernel.org,
 Peter Zijlstra <peterz@...radead.org>, Alexei Starovoitov <ast@...nel.org>,
 Yonghong Song <yhs@...com>, "Paul E . McKenney" <paulmck@...nel.org>, Ingo
 Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Mark
 Rutland <mark.rutland@....com>, Alexander Shishkin
 <alexander.shishkin@...ux.intel.com>, Namhyung Kim <namhyung@...nel.org>,
 Andrii Nakryiko <andrii.nakryiko@...il.com>, bpf@...r.kernel.org, Joel
 Fernandes <joel@...lfernandes.org>, linux-trace-kernel@...r.kernel.org,
 Michael Jeanson <mjeanson@...icios.com>
Subject: Re: [PATCH v1 2/8] tracing/ftrace: guard syscall probe with
 preempt_notrace

On Fri, 4 Oct 2024 10:18:59 -0400
Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:

> On 2024-10-04 15:26, Steven Rostedt wrote:
> > On Thu, 3 Oct 2024 21:33:16 -0400
> > Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> >   
> >> On 2024-10-04 03:04, Steven Rostedt wrote:  
> >>> On Thu, 3 Oct 2024 20:26:29 -0400
> >>> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> >>>
> >>>      
> >>>> static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id)
> >>>> {
> >>>>            struct trace_array *tr = data;
> >>>>            struct trace_event_file *trace_file;
> >>>>            struct syscall_trace_enter *entry;
> >>>>            struct syscall_metadata *sys_data;
> >>>>            struct trace_event_buffer fbuffer;
> >>>>            unsigned long args[6];
> >>>>            int syscall_nr;
> >>>>            int size;
> >>>>
> >>>>            syscall_nr = trace_get_syscall_nr(current, regs);
> >>>>            if (syscall_nr < 0 || syscall_nr >= NR_syscalls)
> >>>>                    return;
> >>>>
> >>>>            /* Here we're inside tp handler's rcu_read_lock_sched (__DO_TRACE) */
> >>>>            trace_file = rcu_dereference_sched(tr->enter_syscall_files[syscall_nr]);
> >>>>
> >>>> ^^^^ this function explicitly states that preempt needs to be disabled by
> >>>> tracepoints.  
> >>>
> >>> Ah, I should have known it was the syscall portion. I don't care for this
> >>> hidden dependency. I rather add a preempt disable here and not expect it to
> >>> be disabled when called.  
> >>
> >> Which is exactly what this patch is doing.  
> > 
> > I was thinking of putting the protection in the function and not the macro.  
> 
> I'm confused by your comment. The protection is added to the function here:

Ah, sorry. I'm the one confused. I was talking about this part:

> +#undef DECLARE_EVENT_SYSCALL_CLASS
> +#define DECLARE_EVENT_SYSCALL_CLASS(call, proto, args, tstruct, assign, print) \
> +__DECLARE_EVENT_CLASS(call, PARAMS(proto), PARAMS(args), PARAMS(tstruct), \
> +		      PARAMS(assign), PARAMS(print))			\
> +static notrace void							\
> +trace_event_raw_event_##call(void *__data, proto)			\
> +{									\
> +	guard(preempt_notrace)();					\
> +	do_trace_event_raw_event_##call(__data, args);			\
> +}
> +

But that's for the non-syscall case.

This is why I shouldn't review patches just before going to bed :-p

-- Steve