linux-kernel - Re: [PATCH] Tracepoint: Make skb tracepoint use TRACE

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: <20090409152924.GB5716@hmsreliant.think-freely.org>
Date:	Thu, 9 Apr 2009 11:29:24 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Ingo Molnar <mingo@...e.hu>, Zhaolei <zhaolei@...fujitsu.com>,
	"David S. Miller" <davem@...emloft.net>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Tom Zanussi <tzanussi@...il.com>, linux-kernel@...r.kernel.org,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH] Tracepoint: Make skb tracepoint use TRACE_EVENT macro

On Thu, Apr 09, 2009 at 11:05:08AM -0400, Steven Rostedt wrote:
> 
> On Thu, 9 Apr 2009, Neil Horman wrote:
> > 
> > I think this seems like a reasonable idea, as long as the sites that register a
> > hook for the tracepoint don't need to be updated (and it looks like they dont).
> > I do have one nit though: include/trace/skb.h has been emptied and now includes
> > include/trace/skb_event_types.h instead.  For the sake of neatness I'd say we
> > should either not make that change, and just dump the contents of
> > skb_event_types.h into trace/skb.h, or alternatively, remove trace/skb.h
> > entirely, and have files that need trace/skb.h include skb_event_types.h
> > instead.  Do that and it has my ACK.
> 
> Actually that trace/skb.h change is needed. It is not emptied, it still 
> has:
> 
>  #include <linux/skbuff.h>
>  #include <linux/tracepoint.h>
> 
> before the include of include/trace/skb_event_types.h
> 
> For ftrace to do its magic with the TRACE_EVENT macro, the file that 
> contains the TRACE_EVENT can not include tracepoint.h.  ftrace runs the 
> skb_event_types.h through a series of redefines of the TRACE_EVENT to 
> automate the creation of the directory and files in 
> debugfs/tracing/events/*.
> 
> We've already changed most of the files in include/trace/ to this format.
> 
> I hope we can still have your Ack without making your requested update.
> 
> Thanks,
> 
> -- Steve
> 
> 

That strikes me as overly complex and fragile.  One would think that ftrace
could include a first pass to scrub out that include prior to processing.
Having a utility reliant on the format of kernel header files, especially the
placement of includes is begging for somthing to break.

I'll ack it since the change I'm asking for is just cosmetic, and since any
reversions in this area are going to break ftrace in userspace rather than oops
the kernel, but I would really like to see ftrace hardened in such a way that
we don't need to be extra careful of include formatting when adding additional
tracepoints.

Acked-by: Neil Horman <nhorman@...driver.com>

--
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/