From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <systemtap-return-13880-listarch-systemtap=sources.redhat.com@sourceware.org>
Received: (qmail 7433 invoked by alias); 12 Oct 2009 10:11:00 -0000
Received: (qmail 7422 invoked by uid 22791); 12 Oct 2009 10:10:58 -0000
X-SWARE-Spam-Status: No, hits=-0.1 required=5.0 	tests=AWL,BAYES_00,SARE_MSGID_LONG40,SPF_PASS
X-Spam-Check-By: sourceware.org
Received: from mail-fx0-f226.google.com (HELO mail-fx0-f226.google.com) (209.85.220.226)     by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 12 Oct 2009 10:10:54 +0000
Received: by fxm26 with SMTP id 26so26790857fxm.23         for <systemtap@sources.redhat.com>; Mon, 12 Oct 2009 03:10:52 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.223.143.15 with SMTP id s15mr1453527fau.77.1255342251884; Mon,  	12 Oct 2009 03:10:51 -0700 (PDT)
In-Reply-To: <20091007222807.1684.26880.stgit@dhcp-100-2-132.bos.redhat.com>
References: <20091007222733.1684.32035.stgit@dhcp-100-2-132.bos.redhat.com> 	 <20091007222807.1684.26880.stgit@dhcp-100-2-132.bos.redhat.com>
Date: Mon, 12 Oct 2009 10:11:00 -0000
Message-ID: <c62985530910120310m386e9289v29e71fee1992476b@mail.gmail.com>
Subject: Re: [PATCH tracing/kprobes v3 4/7] tracing/kprobes: Avoid field name  	confliction
From: =?ISO-8859-1?Q?Fr=E9d=E9ric_Weisbecker?= <fweisbec@gmail.com>
To: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>, Ingo Molnar <mingo@elte.hu>,  	lkml <linux-kernel@vger.kernel.org>, systemtap <systemtap@sources.redhat.com>,  	DLE <dle-develop@lists.sourceforge.net>, Thomas Gleixner <tglx@linutronix.de>,  	Arnaldo Carvalho de Melo <acme@redhat.com>, Mike Galbraith <efault@gmx.de>, Paul Mackerras <paulus@samba.org>,  	Peter Zijlstra <a.p.zijlstra@chello.nl>, Christoph Hellwig <hch@infradead.org>,  	Ananth N Mavinakayanahalli <ananth@in.ibm.com>, Jim Keniston <jkenisto@us.ibm.com>,  	"Frank Ch. Eigler" <fche@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <systemtap.sourceware.org>
List-Subscribe: <mailto:systemtap-subscribe@sourceware.org>
List-Post: <mailto:systemtap@sourceware.org>
List-Help: <mailto:systemtap-help@sourceware.org>, <http://sourceware.org/lists.html#faqs>
Sender: systemtap-owner@sourceware.org
X-SW-Source: 2009-q4/txt/msg00121.txt.bz2

2009/10/8 Masami Hiramatsu <mhiramat@redhat.com>:
> Check whether the argument name is conflict with other field names.
>
> Changes in v3:
> =A0- Check strcmp() =3D=3D 0 instead of !strcmp().
>
> Changes in v2:
> =A0- 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>
> ---
>
> =A0kernel/trace/trace_kprobe.c | =A0 65 +++++++++++++++++++++++++++++++++=
++--------
> =A01 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 @@
> =A0#define MAX_EVENT_NAME_LEN 64
> =A0#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"


If it might conflict, then we should minimize the possibilities for
that to happen.

What if we prefix these fields with kprobed_ ?

kprobed_ip
kprobed_nargs
kprobed_ret_ip
kprobed_func

We are lucky there in that kprobe functions in the kernel can't be kprobed
so it's safe wrt the conflict in the same namespace.

And we can further schrink the kprobed prefixes in userspace post processin=
g.

(If you agree with the above, that can be done incrementally).

Thanks.


> +
> +const char *reserved_field_names[] =3D {
> + =A0 =A0 =A0 "common_type",
> + =A0 =A0 =A0 "common_flags",
> + =A0 =A0 =A0 "common_preempt_count",
> + =A0 =A0 =A0 "common_pid",
> + =A0 =A0 =A0 "common_tgid",
> + =A0 =A0 =A0 "common_lock_depth",
> + =A0 =A0 =A0 FIELD_STRING_IP,
> + =A0 =A0 =A0 FIELD_STRING_NARGS,
> + =A0 =A0 =A0 FIELD_STRING_RETIP,
> + =A0 =A0 =A0 FIELD_STRING_FUNC,
> +};
> +
> =A0/* currently, trace_kprobe only supports X86. */
>
> =A0struct fetch_func {
> @@ -537,6 +556,20 @@ static int parse_probe_arg(char *arg, struct fetch_f=
unc *ff, int is_return)
> =A0 =A0 =A0 =A0return ret;
> =A0}
>
> +/* Return 1 if name is reserved or already used by another argument */
> +static int conflict_field_name(const char *name,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct probe=
_arg *args, int narg)
> +{
> + =A0 =A0 =A0 int i;
> + =A0 =A0 =A0 for (i =3D 0; i < ARRAY_SIZE(reserved_field_names); i++)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (strcmp(reserved_field_names[i], name) =
=3D=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 1;
> + =A0 =A0 =A0 for (i =3D 0; i < narg; i++)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (strcmp(args[i].name, name) =3D=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 1;
> + =A0 =A0 =A0 return 0;
> +}
> +
> =A0static int create_trace_probe(int argc, char **argv)
> =A0{
> =A0 =A0 =A0 =A0/*
> @@ -637,6 +670,12 @@ static int create_trace_probe(int argc, char **argv)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*arg++ =3D '\0';
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0arg =3D argv[i];
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (conflict_field_name(argv[i], tp->args, =
i)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -EINVAL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto error;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tp->args[i].name =3D kstrdup(argv[i], GFP_=
KERNEL);
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Parse fetch argument */
> @@ -1039,8 +1078,8 @@ static int kprobe_event_define_fields(struct ftrace=
_event_call *event_call)
> =A0 =A0 =A0 =A0if (!ret)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return ret;
>
> - =A0 =A0 =A0 DEFINE_FIELD(unsigned long, ip, "ip", 0);
> - =A0 =A0 =A0 DEFINE_FIELD(int, nargs, "nargs", 1);
> + =A0 =A0 =A0 DEFINE_FIELD(unsigned long, ip, FIELD_STRING_IP, 0);
> + =A0 =A0 =A0 DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
> =A0 =A0 =A0 =A0/* Set argument names as fields */
> =A0 =A0 =A0 =A0for (i =3D 0; i < tp->nr_args; i++)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0DEFINE_FIELD(unsigned long, args[i], tp->a=
rgs[i].name, 0);
> @@ -1057,9 +1096,9 @@ static int kretprobe_event_define_fields(struct ftr=
ace_event_call *event_call)
> =A0 =A0 =A0 =A0if (!ret)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return ret;
>
> - =A0 =A0 =A0 DEFINE_FIELD(unsigned long, func, "func", 0);
> - =A0 =A0 =A0 DEFINE_FIELD(unsigned long, ret_ip, "ret_ip", 0);
> - =A0 =A0 =A0 DEFINE_FIELD(int, nargs, "nargs", 1);
> + =A0 =A0 =A0 DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
> + =A0 =A0 =A0 DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
> + =A0 =A0 =A0 DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
> =A0 =A0 =A0 =A0/* Set argument names as fields */
> =A0 =A0 =A0 =A0for (i =3D 0; i < tp->nr_args; i++)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0DEFINE_FIELD(unsigned long, args[i], tp->a=
rgs[i].name, 0);
> @@ -1108,15 +1147,16 @@ static int kprobe_event_show_format(struct ftrace=
_event_call *call,
> =A0 =A0 =A0 =A0int ret, i;
> =A0 =A0 =A0 =A0struct trace_probe *tp =3D (struct trace_probe *)call->dat=
a;
>
> - =A0 =A0 =A0 SHOW_FIELD(unsigned long, ip, "ip");
> - =A0 =A0 =A0 SHOW_FIELD(int, nargs, "nargs");
> + =A0 =A0 =A0 SHOW_FIELD(unsigned long, ip, FIELD_STRING_IP);
> + =A0 =A0 =A0 SHOW_FIELD(int, nargs, FIELD_STRING_NARGS);
>
> =A0 =A0 =A0 =A0/* Show fields */
> =A0 =A0 =A0 =A0for (i =3D 0; i < tp->nr_args; i++)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0SHOW_FIELD(unsigned long, args[i], tp->arg=
s[i].name);
> =A0 =A0 =A0 =A0trace_seq_puts(s, "\n");
>
> - =A0 =A0 =A0 return __probe_event_show_format(s, tp, "(%lx)", "REC->ip");
> + =A0 =A0 =A0 return __probe_event_show_format(s, tp, "(%lx)",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0"REC->" FIELD_STRING_IP);
> =A0}
>
> =A0static int kretprobe_event_show_format(struct ftrace_event_call *call,
> @@ -1126,9 +1166,9 @@ static int kretprobe_event_show_format(struct ftrac=
e_event_call *call,
> =A0 =A0 =A0 =A0int ret, i;
> =A0 =A0 =A0 =A0struct trace_probe *tp =3D (struct trace_probe *)call->dat=
a;
>
> - =A0 =A0 =A0 SHOW_FIELD(unsigned long, func, "func");
> - =A0 =A0 =A0 SHOW_FIELD(unsigned long, ret_ip, "ret_ip");
> - =A0 =A0 =A0 SHOW_FIELD(int, nargs, "nargs");
> + =A0 =A0 =A0 SHOW_FIELD(unsigned long, func, FIELD_STRING_FUNC);
> + =A0 =A0 =A0 SHOW_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP);
> + =A0 =A0 =A0 SHOW_FIELD(int, nargs, FIELD_STRING_NARGS);
>
> =A0 =A0 =A0 =A0/* Show fields */
> =A0 =A0 =A0 =A0for (i =3D 0; i < tp->nr_args; i++)
> @@ -1136,7 +1176,8 @@ static int kretprobe_event_show_format(struct ftrac=
e_event_call *call,
> =A0 =A0 =A0 =A0trace_seq_puts(s, "\n");
>
> =A0 =A0 =A0 =A0return __probe_event_show_format(s, tp, "(%lx <- %lx)",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 "REC->func, REC->ret_ip");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0"REC->" FIELD_STRING_FUNC
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0", REC->" FIELD_STRING_RETIP);
> =A0}
>
> =A0#ifdef CONFIG_EVENT_PROFILE
>
>
> --
> Masami Hiramatsu
>
> Software Engineer
> Hitachi Computer Products (America), Inc.
> Software Solutions Division
>
> e-mail: mhiramat@redhat.com
>