| 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
| ||
|
Message-ID: <51BEAAA1.20806@hitachi.com>
Date: Mon, 17 Jun 2013 15:20:17 +0900
From: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Frederic Weisbecker <fweisbec@...il.com>,
Ingo Molnar <mingo@...hat.com>,
Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
"zhangwei(Jovi)" <jovi.zhangwei@...wei.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] tracing/kprobes: Turn trace_probe->files into
list_head
(2013/06/17 2:21), Oleg Nesterov wrote:
> I think that "ftrace_event_file *trace_probe[]" complicates the
> code for no reason, turn it into list_head to simplify the code.
> enable_trace_probe() no longer needs synchronize_sched().
Looks cleaner :)
>
> This needs the extra sizeof(list_head) memory for every attached
> ftrace_event_file, hopefully not a problem in this case.
I think it's no problem, because the number depends on the instances
and it could not be so much. :)
Thanks!
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
> kernel/trace/trace_kprobe.c | 138 ++++++++++++-------------------------------
> 1 files changed, 37 insertions(+), 101 deletions(-)
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5a73de0..b95f683 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -35,12 +35,17 @@ struct trace_probe {
> const char *symbol; /* symbol name */
> struct ftrace_event_class class;
> struct ftrace_event_call call;
> - struct ftrace_event_file * __rcu *files;
> + struct list_head files;
> ssize_t size; /* trace entry size */
> unsigned int nr_args;
> struct probe_arg args[];
> };
>
> +struct event_file_link {
> + struct ftrace_event_file *file;
> + struct list_head list;
> +};
> +
> #define SIZEOF_TRACE_PROBE(n) \
> (offsetof(struct trace_probe, args) + \
> (sizeof(struct probe_arg) * (n)))
> @@ -150,6 +155,7 @@ static struct trace_probe *alloc_trace_probe(const char *group,
> goto error;
>
> INIT_LIST_HEAD(&tp->list);
> + INIT_LIST_HEAD(&tp->files);
> return tp;
> error:
> kfree(tp->call.name);
> @@ -184,22 +190,6 @@ static struct trace_probe *find_trace_probe(const char *event,
> }
>
> /*
> - * This and enable_trace_probe/disable_trace_probe rely on event_mutex
> - * held by the caller, __ftrace_set_clr_event().
> - */
> -static int trace_probe_nr_files(struct trace_probe *tp)
> -{
> - struct ftrace_event_file **file = rcu_dereference_raw(tp->files);
> - int ret = 0;
> -
> - if (file)
> - while (*(file++))
> - ret++;
> -
> - return ret;
> -}
> -
> -/*
> * Enable trace_probe
> * if the file is NULL, enable "perf" handler, or enable "trace" handler.
> */
> @@ -209,29 +199,18 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
> int ret = 0;
>
> if (file) {
> - struct ftrace_event_file **new, **old;
> - int n = trace_probe_nr_files(tp);
> -
> - old = rcu_dereference_raw(tp->files);
> - /* 1 is for new one and 1 is for stopper */
> - new = kzalloc((n + 2) * sizeof(struct ftrace_event_file *),
> - GFP_KERNEL);
> - if (!new) {
> + struct event_file_link *link;
> +
> + link = kmalloc(sizeof(*link), GFP_KERNEL);
> + if (!link) {
> ret = -ENOMEM;
> goto out;
> }
> - memcpy(new, old, n * sizeof(struct ftrace_event_file *));
> - new[n] = file;
> - /* The last one keeps a NULL */
>
> - rcu_assign_pointer(tp->files, new);
> - tp->flags |= TP_FLAG_TRACE;
> + link->file = file;
> + list_add_rcu(&link->list, &tp->files);
>
> - if (old) {
> - /* Make sure the probe is done with old files */
> - synchronize_sched();
> - kfree(old);
> - }
> + tp->flags |= TP_FLAG_TRACE;
> } else
> tp->flags |= TP_FLAG_PROFILE;
>
> @@ -246,24 +225,16 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
> return ret;
> }
>
> -static int
> -trace_probe_file_index(struct trace_probe *tp, struct ftrace_event_file *file)
> +static struct event_file_link *
> +find_event_file_link(struct trace_probe *tp, struct ftrace_event_file *file)
> {
> - struct ftrace_event_file **files;
> - int i;
> + struct event_file_link *link;
>
> - /*
> - * Since all tp->files updater is protected by probe_enable_lock,
> - * we don't need to lock an rcu_read_lock.
> - */
> - files = rcu_dereference_raw(tp->files);
> - if (files) {
> - for (i = 0; files[i]; i++)
> - if (files[i] == file)
> - return i;
> - }
> + list_for_each_entry(link, &tp->files, list)
> + if (link->file == file)
> + return link;
>
> - return -1;
> + return NULL;
> }
>
> /*
> @@ -276,38 +247,23 @@ disable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
> int ret = 0;
>
> if (file) {
> - struct ftrace_event_file **new, **old;
> - int n = trace_probe_nr_files(tp);
> - int i, j;
> + struct event_file_link *link;
>
> - old = rcu_dereference_raw(tp->files);
> - if (n == 0 || trace_probe_file_index(tp, file) < 0) {
> + link = find_event_file_link(tp, file);
> + if (!link) {
> ret = -EINVAL;
> goto out;
> }
>
> - if (n == 1) { /* Remove the last file */
> - tp->flags &= ~TP_FLAG_TRACE;
> - new = NULL;
> - } else {
> - new = kzalloc(n * sizeof(struct ftrace_event_file *),
> - GFP_KERNEL);
> - if (!new) {
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - /* This copy & check loop copies the NULL stopper too */
> - for (i = 0, j = 0; j < n && i < n + 1; i++)
> - if (old[i] != file)
> - new[j++] = old[i];
> - }
> + list_del_rcu(&link->list);
> + /* synchronize with kprobe_trace_func/kretprobe_trace_func */
> + synchronize_sched();
> + kfree(link);
>
> - rcu_assign_pointer(tp->files, new);
> + if (!list_empty(&tp->files))
> + goto out;
>
> - /* Make sure the probe is done with old files */
> - synchronize_sched();
> - kfree(old);
> + tp->flags &= ~TP_FLAG_TRACE;
> } else
> tp->flags &= ~TP_FLAG_PROFILE;
>
> @@ -872,20 +828,10 @@ __kprobe_trace_func(struct trace_probe *tp, struct pt_regs *regs,
> static __kprobes void
> kprobe_trace_func(struct trace_probe *tp, struct pt_regs *regs)
> {
> - /*
> - * Note: preempt is already disabled around the kprobe handler.
> - * However, we still need an smp_read_barrier_depends() corresponding
> - * to smp_wmb() in rcu_assign_pointer() to access the pointer.
> - */
> - struct ftrace_event_file **file = rcu_dereference_raw(tp->files);
> -
> - if (unlikely(!file))
> - return;
> + struct event_file_link *link;
>
> - while (*file) {
> - __kprobe_trace_func(tp, regs, *file);
> - file++;
> - }
> + list_for_each_entry_rcu(link, &tp->files, list)
> + __kprobe_trace_func(tp, regs, link->file);
> }
>
> /* Kretprobe handler */
> @@ -932,20 +878,10 @@ static __kprobes void
> kretprobe_trace_func(struct trace_probe *tp, struct kretprobe_instance *ri,
> struct pt_regs *regs)
> {
> - /*
> - * Note: preempt is already disabled around the kprobe handler.
> - * However, we still need an smp_read_barrier_depends() corresponding
> - * to smp_wmb() in rcu_assign_pointer() to access the pointer.
> - */
> - struct ftrace_event_file **file = rcu_dereference_raw(tp->files);
> -
> - if (unlikely(!file))
> - return;
> + struct event_file_link *link;
>
> - while (*file) {
> - __kretprobe_trace_func(tp, ri, regs, *file);
> - file++;
> - }
> + list_for_each_entry_rcu(link, &tp->files, list)
> + __kretprobe_trace_func(tp, ri, regs, link->file);
> }
>
> /* Event entry printers */
>
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com
--
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/
Powered by blists - more mailing lists