linux-kernel - Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks

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: <1273244123.22438.140.camel@gandalf.stny.rr.com>
Date:	Fri, 07 May 2010 10:55:23 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Li Zefan <lizf@...fujitsu.com>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have
 data?passed to tracepoint callbacks

On Fri, 2010-05-07 at 10:39 -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@...dmis.org) wrote:
> > 
> > 
> > "Frederic Weisbecker" <fweisbec@...il.com> wrote:
> > 
> > >On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
> [...]
> > >> 
> > >> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > >> index 78b4bd3..ee8059a 100644
> > >> --- a/include/linux/tracepoint.h
> > >> +++ b/include/linux/tracepoint.h
> > >> @@ -20,12 +20,17 @@
> > >>  struct module;
> > >>  struct tracepoint;
> > >>  
> > >> +struct tracepoint_func {
> > >> +	void *func;
> > >> +	void *data;
> > >> +};
> > >> +
> > >>  struct tracepoint {
> > >>  	const char *name;		/* Tracepoint name */
> > >>  	int state;			/* State. */
> > >>  	void (*regfunc)(void);
> > >>  	void (*unregfunc)(void);
> > >> -	void **funcs;
> > >> +	struct tracepoint_func *funcs;
> > >>  } __attribute__((aligned(32)));		/*
> > >>  					 * Aligned on 32 bytes because it is
> > >>  					 * globally visible and gcc happily
> > >> @@ -46,14 +51,18 @@ struct tracepoint {
> > >>   */
> > >>  #define __DO_TRACE(tp, proto, args)					\
> > >>  	do {								\
> > >> -		void **it_func;						\
> > >> +		struct tracepoint_func *it_func_ptr;			\
> > >> +		void *it_func;						\
> > >> +		void *__data;						\
> > >>  									\
> > >>  		rcu_read_lock_sched_notrace();				\
> > >> -		it_func = rcu_dereference_sched((tp)->funcs);		\
> > >> -		if (it_func) {						\
> > >> +		it_func_ptr = rcu_dereference_sched((tp)->funcs);	\
> > >> +		if (it_func_ptr) {					\
> > >>  			do {						\
> > >> -				((void(*)(proto))(*it_func))(args);	\
> > >> -			} while (*(++it_func));				\
> > >> +				it_func = (it_func_ptr)->func;		\
> > >> +				__data = (it_func_ptr)->data;		\
> > >> +				((void(*)(proto))(it_func))(args);	\
> > >
> > >
> > >So, we had a talk about this and we concluded that it is probably fine
> > >on every archs to push one more argument than needed in a function.
> > >
> > 
> > Yeah, I'm hoping it's fine.
> 
> How about changing the callback prototypes to match the call arguments (changing
> the type expected in register/unregister_trace, as well as an additional "check
> type" that I proposed for Ftrace) ?

This can not happen!!!! As I said before, the register is done in C,
there is no macro that will help here. We create the call back with the
macro, but the registering is in kernel/trace/trace_events.c. One
register for ___ALL___ events!!!

 Thus there is no check.

Understand this yet?


> 
> Otherwise, you basically expect here that:
> 
> void fct(void *foo, void *bar, etc etc) (N parameters expected)
> {
> 
> }
> 
> called by:
> 
> fct(foo, bar, etc etc, foobar) (N + 1 parameters)
> 
> will always work.
> 
> Can you show me where the C standard says it is safe to do so ?

No, but it seems safe in the kernel ;-)

But that said. There is another option that will conform to this, and
that is to add flags to registering tracepoints. I already wrote a patch
for this in trying to do some other work (that I threw away).


So here's the proposal.

Change struct tracepoint_func to...

struct tracepoint_func {
	void *func;
	void *data;
	unsigned int flags;
};


The flags is set when registered. If a function is registered with data,
then the flags field will be set. Then the calling of the function can
be:

	if ((it_func_ptr)->flags & TP_FL_DATA)
		((void(*)(proto, void *))(it_func)(args, __data);
	else
		((void(*)(proto))(it_func)(args);

This would comply with the C standard.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/