linux-kernel - Re: [PATCH 3/8] ftrace: Add enable/disable ftrace

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]

Date:	Wed, 21 Dec 2011 17:43:04 +0100
From:	Jiri Olsa <jolsa@...hat.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	fweisbec@...il.com, mingo@...hat.com, paulus@...ba.org,
	acme@...stprotocols.net, a.p.zijlstra@...llo.nl,
	linux-kernel@...r.kernel.org, aarapov@...hat.com
Subject: Re: [PATCH 3/8] ftrace: Add enable/disable ftrace_ops control
 interface

On Wed, Dec 21, 2011 at 11:01:33AM -0500, Steven Rostedt wrote:
> On Wed, 2011-12-21 at 12:48 +0100, Jiri Olsa wrote:

SNIP

> > +/**
> > + * ftrace_function_enable - enable controlled ftrace_ops on given cpu
> > + *
> > + * This function enables tracing on given cpu by decreasing
> > + * the per cpu control variable.
> > + * It must be called with preemption disabled and only on
> > + * ftrace_ops registered with FTRACE_OPS_FL_CONTROL.
> > + */
> > +static inline void ftrace_function_enable(struct ftrace_ops *ops, int cpu)
> > +{
> > +	atomic_t *disabled;
> > +
> > +	if (WARN_ON_ONCE(!(ops->flags & FTRACE_OPS_FL_CONTROL)) ||
> > +			 !preempt_count())
> 
> The WARN_ON_ONCE() should also include the !preempt_count().
> 

ouch, that was initial intention.. need eye doctor ;)

> 
> > +		return;
> > +
> > +	disabled = per_cpu_ptr(ops->disabled, cpu);
> > +	atomic_dec(disabled);
> > +}
> > +

SNIP

> > +
> > +static int control_ops_is_disabled(struct ftrace_ops *ops)
> > +{
> > +	atomic_t *disabled = this_cpu_ptr(ops->disabled);
> 
> Again, the use of "this_cpu_ptr" is wrong. Gah! We should nuke all of
> that crap.

will nuke this one..

> 
> 

> >  static int __register_ftrace_function(struct ftrace_ops *ops)
> >  {
> >  	if (ftrace_disabled)
> > @@ -268,15 +324,19 @@ static int __register_ftrace_function(struct ftrace_ops *ops)
> >  	if (WARN_ON(ops->flags & FTRACE_OPS_FL_ENABLED))
> >  		return -EBUSY;
> >  
> > +	if ((ops->flags & FL_GLOBAL_CONTROL) == FL_GLOBAL_CONTROL)
> 
> No biggy, but I usually find:
> 
> 	if (ops->flags & FL_GLOBAL_CONTROL)
> 
> more readable. With what you have, I looked at that condition three
> times to figure out what was different between what was '&'d with the
> flags and what was being equal too. Usually the ((flags & X) == Y) is
> done to check if a subset of bits are set within a mask of bits.

Well, thats what I need to do here. Bail out if both bits are set,
since we dont support both global and control flags set at the same
time.. I'll add some comment to it.

thanks,
jirka
--
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/