Messages in this thread | | | Date | Mon, 12 Oct 2009 19:37:54 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH tracing/kprobes v3 4/7] tracing/kprobes: Avoid field name confliction |
| |
On Wed, Oct 07, 2009 at 06:28:07PM -0400, Masami Hiramatsu wrote: > Check whether the argument name is conflict with other field names. > > Changes in v3: > - Check strcmp() == 0 instead of !strcmp(). > > Changes in v2: > - Add common_lock_depth to reserved name list. > > Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com> > Cc: Frederic Weisbecker <fweisbec@gmail.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Mike Galbraith <efault@gmx.de> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com> > Cc: Jim Keniston <jkenisto@us.ibm.com> > Cc: Frank Ch. Eigler <fche@redhat.com> > --- > > kernel/trace/trace_kprobe.c | 65 +++++++++++++++++++++++++++++++++++-------- > 1 files changed, 53 insertions(+), 12 deletions(-) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 030f28c..e3b824a 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -38,6 +38,25 @@ > #define MAX_EVENT_NAME_LEN 64 > #define KPROBE_EVENT_SYSTEM "kprobes" > > +/* Reserved field names */ > +#define FIELD_STRING_IP "ip" > +#define FIELD_STRING_NARGS "nargs" > +#define FIELD_STRING_RETIP "ret_ip" > +#define FIELD_STRING_FUNC "func" > + > +const char *reserved_field_names[] = { > + "common_type", > + "common_flags", > + "common_preempt_count", > + "common_pid", > + "common_tgid", > + "common_lock_depth", > + FIELD_STRING_IP, > + FIELD_STRING_NARGS, > + FIELD_STRING_RETIP, > + FIELD_STRING_FUNC, > +}; > + > /* currently, trace_kprobe only supports X86. */ > > struct fetch_func { > @@ -537,6 +556,20 @@ static int parse_probe_arg(char *arg, struct fetch_func *ff, int is_return) > return ret; > } > > +/* Return 1 if name is reserved or already used by another argument */ > +static int conflict_field_name(const char *name, > + struct probe_arg *args, int narg) > +{ > + int i; > + for (i = 0; i < ARRAY_SIZE(reserved_field_names); i++) > + if (strcmp(reserved_field_names[i], name) == 0) > + return 1; > + for (i = 0; i < narg; i++) > + if (strcmp(args[i].name, name) == 0) > + return 1; > + return 0; > +} > + > static int create_trace_probe(int argc, char **argv) > { > /* > @@ -637,6 +670,12 @@ static int create_trace_probe(int argc, char **argv) > *arg++ = '\0'; > else > arg = argv[i]; > + > + if (conflict_field_name(argv[i], tp->args, i)) { > + ret = -EINVAL;
The conflict issue might not be obvious for a user desperately trying to set a kprobe. Even for other failcases, it might not be obvious (blacklisted symbols, syntax errors...)
May be should you improve the error granularity and print a KERN_DEBUG message?
Thanks.
| |