linux-kernel - Re: [PATCH] tracing: Don't display trigger file for events that don't have register func

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] [day] [month] [year] [list]

Message-ID: <2087997675.42526586.1462274976826.JavaMail.zimbra@redhat.com>
Date:	Tue, 3 May 2016 07:29:36 -0400 (EDT)
From:	Chunyu Hu <chuhu@...hat.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tracing: Don't display trigger file for events that
 don't have register func



----- Original Message -----
> From: "Steven Rostedt" <rostedt@...dmis.org>
> To: "Chunyu Hu" <chuhu@...hat.com>
> Cc: linux-kernel@...r.kernel.org
> Sent: Tuesday, May 3, 2016 10:42:44 AM
> Subject: Re: [PATCH] tracing: Don't display trigger file for events that don't have register func
> 
> On Mon,  2 May 2016 22:14:14 +0800
> Chunyu Hu <chuhu@...hat.com> wrote:
> 
> > Currently register function of the event will be called
> > through the 'reg'  field of event class directly without
> > any check when seting up triggers.
> > 
> > Triggers for events that don't support register through
> > debug fs (events under events/ftrace are for perf to
> 
> Actually, they were created for trace-cmd. I'm not even sure if perf
> uses the ftrace event formats.

Thanks for correcting me. I misunderstood it.
 
> 
> > read event format, and most of them don't have regisgter
> > function except events/ftrace/function.) can't be enabled
> > at all, and an oops will be hit when setting up trigger
> > for those events, so just not showing them is an easy way
> > to avoid the oops.
> > 
> > Signed-off-by: Chunyu Hu <chuhu@...hat.com>
> > ---
> >  kernel/trace/trace_events.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index da1eeb6..9fb99fd 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -2138,9 +2138,10 @@ event_create_dir(struct dentry *parent, struct
> > trace_event_file *file)
> >  	trace_create_file("filter", 0644, file->dir, file,
> >  			  &ftrace_event_filter_fops);
> >  
> > -	trace_create_file("trigger", 0644, file->dir, file,
> > -			  &event_trigger_fops);
> > -
> > +	if (call->class->reg) {
> > +		trace_create_file("trigger", 0644, file->dir, file,
> > +				  &event_trigger_fops);
> > +	}
> 
> As you stated, reg is there for function tracing, and is not a good
> value to use as a check. Use the following check instead:
> 
> 	if (!(call->flags & TRACE_EVENT_FL_IGNORE_ENABLE))
> 		trace_create_file("trigger", 0644, file->dir, file,
> 				  &event_trigger_fops);

Thanks. Agree, will prepare v2 for this.

It looks like that checking both is safest way, like other parts
did in trace_events.c. but i didn't see other events using 
TRACE_EVENT_FL_IGNORE_ENABLE, ftrace event sub system is the only user now.
and didn't see other events that don't have register func.
so i think it's safe to use only this flag to check it.

> And add a comment that states that only event directories that can be
> enabled should have triggers.

Ok.

> -- Steve
> 
> 
> 
> >  #ifdef CONFIG_HIST_TRIGGERS
> >  	trace_create_file("hist", 0444, file->dir, file,
> >  			  &event_hist_fops);
> 
> 

-- 
Regards,
Chunyu Hu