(2013/06/17 2:21), Oleg Nesterov wrote:
> enable_trace_probe() and disable_trace_probe() should not worry about
> serialization, the caller (perf_trace_init or __ftrace_set_clr_event)
> holds event_mutex.
>
> They are also called by kprobe_trace_self_tests_init(), but this __init
> function can't race with itself or trace_events.c
Right,
For safety, we should comment this at the caller side, because
those calls are the reason why I have introduced this lock.
Thank you,
> And note that this code depended on event_mutex even before 41a7dd420c
> which introduced probe_enable_lock. In fact it assumes that the caller
> kprobe_register() can never race with itself. Otherwise, say, tp->flags
> manipulations are racy.
>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
> ---
> kernel/trace/trace_kprobe.c | 33 ++++++++++-----------------------
> 1 files changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index c0af476..5a73de0 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -183,16 +183,15 @@ static struct trace_probe *find_trace_probe(const char
> *event,
> return NULL;
> }
>
> +/*
> + * 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;
> + struct ftrace_event_file **file = rcu_dereference_raw(tp->files);
> int ret = 0;
>
> - /*
> - * Since all tp->files updater is protected by probe_enable_lock,
> - * we don't need to lock an rcu_read_lock.
> - */
> - file = rcu_dereference_raw(tp->files);
> if (file)
> while (*(file++))
> ret++;
> @@ -200,8 +199,6 @@ static int trace_probe_nr_files(struct trace_probe *tp)
> return ret;
> }
>
> -static DEFINE_MUTEX(probe_enable_lock);
> -
> /*
> * Enable trace_probe
> * if the file is NULL, enable "perf" handler, or enable "trace" handler.
> @@ -211,8 +208,6 @@ enable_trace_probe(struct trace_probe *tp, struct
> ftrace_event_file *file)
> {
> int ret = 0;
>
> - mutex_lock(&probe_enable_lock);
> -
> if (file) {
> struct ftrace_event_file **new, **old;
> int n = trace_probe_nr_files(tp);
> @@ -223,7 +218,7 @@ enable_trace_probe(struct trace_probe *tp, struct
> ftrace_event_file *file)
> GFP_KERNEL);
> if (!new) {
> ret = -ENOMEM;
> - goto out_unlock;
> + goto out;
> }
> memcpy(new, old, n * sizeof(struct ftrace_event_file *));
> new[n] = file;
> @@ -247,10 +242,7 @@ enable_trace_probe(struct trace_probe *tp, struct
> ftrace_event_file *file)
> else
> ret = enable_kprobe(&tp->rp.kp);
> }
> -
> - out_unlock:
> - mutex_unlock(&probe_enable_lock);
> -
> + out:
> return ret;
> }
>
> @@ -283,8 +275,6 @@ disable_trace_probe(struct trace_probe *tp, struct
> ftrace_event_file *file)
> {
> int ret = 0;
>
> - mutex_lock(&probe_enable_lock);
> -
> if (file) {
> struct ftrace_event_file **new, **old;
> int n = trace_probe_nr_files(tp);
> @@ -293,7 +283,7 @@ disable_trace_probe(struct trace_probe *tp, struct
> ftrace_event_file *file)
> old = rcu_dereference_raw(tp->files);
> if (n == 0 || trace_probe_file_index(tp, file) < 0) {
> ret = -EINVAL;
> - goto out_unlock;
> + goto out;
> }
>
> if (n == 1) { /* Remove the last file */
> @@ -304,7 +294,7 @@ disable_trace_probe(struct trace_probe *tp, struct
> ftrace_event_file *file)
> GFP_KERNEL);
> if (!new) {
> ret = -ENOMEM;
> - goto out_unlock;
> + goto out;
> }
>
> /* This copy & check loop copies the NULL stopper too */
> @@ -327,10 +317,7 @@ disable_trace_probe(struct trace_probe *tp, struct
> ftrace_event_file *file)
> else
> disable_kprobe(&tp->rp.kp);
> }
> -
> - out_unlock:
> - mutex_unlock(&probe_enable_lock);
> -
> + out:
> return ret;
> }
>
>
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
