Re: [RFC][PATCH 5/5] tracing/filters: Provide support for char * pointers

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Aug 06, 2009 at 09:35:39AM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Mon, Aug 03, 2009 at 02:58:15PM +0800, Li Zefan wrote:
> >> Frederic Weisbecker wrote:
> >>> Provide support for char * pointers in the filtering framework.
> >>> Usually, char * entries are dangerous in traces because the string
> >>> can be released whereas a pointer to it can still wait to be read from
> >>> the ring buffer. But sometimes we can assume it's safe, like in case
> >>> of RO data (eg: __file__ or __line__, used in bkl trace event). If
> >>> these RO data are in a module and so is the call to the trace event,
> >>> then it's safe, because the ring buffer will be flushed once this
> >>> module get unloaded.
> >>>
> >> The problem is we don't distinguish dangerous char * from
> >> safe char *... They are both defined as:
> >> 	__field(char *, str)
> >>
> >> So for those dangerous ones, a string filter still can be applied,
> >> which will dereference those pointers.
> > 
> > Yeah, but only reviewing can distinguish them. It depends on the
> > context.
> > IMO, a __builtin_constant check would be wrong. I don't remember who
> > posted recently tracepoints with char * types that were safe although he
> > didn't use string constants.
> > 
> 
> IMO it's really bad to rely on review to prevent wrong use of
> an API..
> 
> Other developers won't know this restriction, and not all tracepoint
> patches go through -tip tree, and not all trace_event source files
> are in include/trace/events/.
> 
> How about add __field_type()? So we can define:
> 
> 	__field_type(char *, str, FILTER_PTR_STR)
> 
> the advantage is he who wrote the code really knows this field is safe
> to be used in filtering as a string.
> 
> I had some patches that does similar job. I can rewrite and post them.


Ah good idea. That may even be useful for further typedef'ed types which
filter process match existing supported types.

Just one neat however: __field_type looks too much ambiguous. __field() is
already here to define a typed field. This seems confusing.

Why not __field_ext() for "extended"? We may probably add more flags
than FILTER_PTR_STR in the future to define options for filtering or even
for larger scope.

I then wait for your patches to be posted and I'll integrate them in the
current queue.

Thanks a lot!