Skip to contentSkip to conversation

tracing: trace_remove_event_call() should fail if call/file is in use

PATCH In review Oleg Nesterov 3 participants 8 messages Jul 29, 2013 – Jul 31, 2013

Subsystems (2)
Tracing ftrace
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

v2 cover tracing: trace_remove_event_call() should fail if call/file is in use

Hello.

On top of "[PATCH v2 0/6] tracing: open/delete fixes" series.

Perhaps this series should be updated, Masami has some concerns. But it seems that you both mostly agree with these changes, so let me send the "last" patch for review.

The last change in kernel/trace/trace_events.c we need to close the problems with create/use/delete (at least those which I know ;).

Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven already has the patches.

Changes:

- remove the TRACE_EVENT_FL_REF_MASK (which we do not have) check.

We rely on the previous changes, we do not care if someone have an opened file, event_enable_write/etc can do nothing but fail after we do remove_event_file_dir().

- Add the comment and a note into the changelog to explain why we still need to check FTRACE_EVENT_FL_ENABLED.

Oleg.

 include/linux/ftrace_event.h |    2 +-
 kernel/trace/trace_events.c  |   35 +++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)
author
#1 lore raw

PATCH v2 1/1 tracing: trace_remove_event_call() should fail if call/file is in use

Change trace_remove_event_call(call) to return the error if this call is active. This is what the callers assume but can't verify outside of the tracing locks. Both trace_kprobe.c/trace_uprobe.c need the additional changes, unregister_trace_probe() should abort if trace_remove_event_call() fails.

The caller is going to free this call/file so we must ensure that nobody can use them after trace_remove_event_call() succeeds. debugfs should be fine after the previous changes and event_remove() does TRACE_REG_UNREGISTER, but still there are 2 reasons why we need the additional checks:

- There could be a perf_event(s) attached to this tp_event, so the patch checks ->perf_refcount.

- TRACE_REG_UNREGISTER can be suppressed by FTRACE_EVENT_FL_SOFT_MODE, so we simply check FTRACE_EVENT_FL_ENABLED protected by event_mutex.

Signed-off-by Oleg Nesterov <[email protected]>
---
 include/linux/ftrace_event.h |    2 +-
 kernel/trace/trace_events.c  |   35 +++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)
include/linux/ftrace_event.h +1 -1
index 4372658..f98ab06 100644
@@ -332,7 +332,7 @@ extern int trace_define_field(struct ftrace_event_call *call, const char *type,
 			      const char *name, int offset, int size,
 			      int is_signed, int filter_type);
 extern int trace_add_event_call(struct ftrace_event_call *call);
-extern void trace_remove_event_call(struct ftrace_event_call *call);
+extern int trace_remove_event_call(struct ftrace_event_call *call);
 
 #define is_signed_type(type)	(((type)(-1)) < (type)1)
kernel/trace/trace_events.c +33 -2
index 79a2743..3450496 100644
@@ -1709,16 +1709,47 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
 	destroy_preds(call);
 }
 
+static int probe_remove_event_call(struct ftrace_event_call *call)
+{
+	struct trace_array *tr;
+	struct ftrace_event_file *file;
+
+#ifdef CONFIG_PERF_EVENTS
+	if (call->perf_refcount)
+		return -EBUSY;
+#endif
+	do_for_each_event_file(tr, file) {
+		if (file->event_call != call)
+			continue;
+		/*
+		 * We can't rely on ftrace_event_enable_disable(enable => 0)
+		 * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
+		 * TRACE_REG_UNREGISTER.
+		 */
+		if (file->flags & FTRACE_EVENT_FL_ENABLED)
+			return -EBUSY;
+		break;
+	} while_for_each_event_file();
+
+	__trace_remove_event_call(call);
+
+	return 0;
+}
+
 /* Remove an event_call */
-void trace_remove_event_call(struct ftrace_event_call *call)
+int trace_remove_event_call(struct ftrace_event_call *call)
 {
+	int ret;
+
 	mutex_lock(&trace_types_lock);
 	mutex_lock(&event_mutex);
 	down_write(&trace_event_sem);
-	__trace_remove_event_call(call);
+	ret = probe_remove_event_call(call);
 	up_write(&trace_event_sem);
 	mutex_unlock(&event_mutex);
 	mutex_unlock(&trace_types_lock);
+
+	return ret;
 }
 
 #define for_each_event(event, start, end)			\
Signature · 1 line
1.5.5.1
Reviewed-by on v2 1/1
#2 lore raw
Quoted 12 lines · Oleg Nesterov

Change trace_remove_event_call(call) to return the error if this call is active. This is what the callers assume but can't verify outside of the tracing locks. Both trace_kprobe.c/trace_uprobe.c need the additional changes, unregister_trace_probe() should abort if trace_remove_event_call() fails.

The caller is going to free this call/file so we must ensure that nobody can use them after trace_remove_event_call() succeeds. debugfs should be fine after the previous changes and event_remove() does TRACE_REG_UNREGISTER, but still there are 2 reasons why we need the additional checks:

Quoted 6 lines

- There could be a perf_event(s) attached to this tp_event, so the patch checks ->perf_refcount.

- TRACE_REG_UNREGISTER can be suppressed by FTRACE_EVENT_FL_SOFT_MODE, so we simply check FTRACE_EVENT_FL_ENABLED protected by event_mutex.

This looks good to me :)

Reviewed-by Masami Hiramatsu <[email protected]>

Thanks! I'll update trace_kprobes.c too.

Quoted 74 lines
Signed-off-by Oleg Nesterov <[email protected]>
---
 include/linux/ftrace_event.h |    2 +-
 kernel/trace/trace_events.c  |   35 +++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)
include/linux/ftrace_event.h +1 -1
index 4372658..f98ab06 100644
@@ -332,7 +332,7 @@ extern int trace_define_field(struct ftrace_event_call *call, const char *type,
 			      const char *name, int offset, int size,
 			      int is_signed, int filter_type);
 extern int trace_add_event_call(struct ftrace_event_call *call);
-extern void trace_remove_event_call(struct ftrace_event_call *call);
+extern int trace_remove_event_call(struct ftrace_event_call *call);
 
 #define is_signed_type(type)	(((type)(-1)) < (type)1)
kernel/trace/trace_events.c +33 -2
index 79a2743..3450496 100644
@@ -1709,16 +1709,47 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
 	destroy_preds(call);
 }
 
+static int probe_remove_event_call(struct ftrace_event_call *call)
+{
+	struct trace_array *tr;
+	struct ftrace_event_file *file;
+
+#ifdef CONFIG_PERF_EVENTS
+	if (call->perf_refcount)
+		return -EBUSY;
+#endif
+	do_for_each_event_file(tr, file) {
+		if (file->event_call != call)
+			continue;
+		/*
+		 * We can't rely on ftrace_event_enable_disable(enable => 0)
+		 * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
+		 * TRACE_REG_UNREGISTER.
+		 */
+		if (file->flags & FTRACE_EVENT_FL_ENABLED)
+			return -EBUSY;
+		break;
+	} while_for_each_event_file();
+
+	__trace_remove_event_call(call);
+
+	return 0;
+}
+
 /* Remove an event_call */
-void trace_remove_event_call(struct ftrace_event_call *call)
+int trace_remove_event_call(struct ftrace_event_call *call)
 {
+	int ret;
+
 	mutex_lock(&trace_types_lock);
 	mutex_lock(&event_mutex);
 	down_write(&trace_event_sem);
-	__trace_remove_event_call(call);
+	ret = probe_remove_event_call(call);
 	up_write(&trace_event_sem);
 	mutex_unlock(&event_mutex);
 	mutex_unlock(&trace_types_lock);
+
+	return ret;
 }
 
 #define for_each_event(event, start, end)			\
Signature · 4 lines
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
maintainer
#3 lore raw
In reply to Oleg Nesterov
Quoted 11 lines · Oleg Nesterov
kernel/trace/trace_events.c +10 -0
+static int probe_remove_event_call(struct ftrace_event_call *call)
+{
+	struct trace_array *tr;
+	struct ftrace_event_file *file;
+
+#ifdef CONFIG_PERF_EVENTS
+	if (call->perf_refcount)
+		return -EBUSY;
+#endif
+	do_for_each_event_file(tr, file) {

kernel/trace/trace_events.c static void __trace_remove_event_call(struct ftrace_event_call *call) · line 1731

Quoted 10 lines
kernel/trace/trace_events.c +10 -0
+		if (file->event_call != call)
+			continue;
+		/*
+		 * We can't rely on ftrace_event_enable_disable(enable => 0)
+		 * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
+		 * TRACE_REG_UNREGISTER.
+		 */
+		if (file->flags & FTRACE_EVENT_FL_ENABLED)
+			return -EBUSY;
+		break;

I'm going to modify the patch to include a comment here about the break being used to go to the next trace_array and not leaving the loop.

-- Steve

Quoted 26 lines
kernel/trace/trace_events.c +13 -2
+	} while_for_each_event_file();
+
+	__trace_remove_event_call(call);
+
+	return 0;
+}
+
 /* Remove an event_call */
-void trace_remove_event_call(struct ftrace_event_call *call)
+int trace_remove_event_call(struct ftrace_event_call *call)
 {
+	int ret;
+
 	mutex_lock(&trace_types_lock);
 	mutex_lock(&event_mutex);
 	down_write(&trace_event_sem);
-	__trace_remove_event_call(call);
+	ret = probe_remove_event_call(call);
 	up_write(&trace_event_sem);
 	mutex_unlock(&event_mutex);
 	mutex_unlock(&trace_types_lock);
+
+	return ret;
 }
 
 #define for_each_event(event, start, end)			\
maintainer
#5 lore raw
In reply to Oleg Nesterov
Quoted 8 lines · Oleg Nesterov

Hello.

On top of "[PATCH v2 0/6] tracing: open/delete fixes" series.

Perhaps this series should be updated, Masami has some concerns. But it seems that you both mostly agree with these changes, so let me send the "last" patch for review.

Quoted 5 lines

The last change in kernel/trace/trace_events.c we need to close the problems with create/use/delete (at least those which I know ;).

Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven already has the patches.

Can you refresh my memory. My INBOX is quite full. What were the subjects for these patches? This fix has lots of different threads, not to mention versions.

Thanks,

-- Steve

Quoted 18 lines

Changes:

- remove the TRACE_EVENT_FL_REF_MASK (which we do not have) check.

We rely on the previous changes, we do not care if someone have an opened file, event_enable_write/etc can do nothing but fail after we do remove_event_file_dir().

- Add the comment and a note into the changelog to explain why we still need to check FTRACE_EVENT_FL_ENABLED.

Oleg.

 include/linux/ftrace_event.h |    2 +-
 kernel/trace/trace_events.c  |   35 +++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)
maintainer
#4 lore raw
In reply to Steven Rostedt
Quoted 17 lines · Steven Rostedt
Quoted 16 lines · Oleg Nesterov
kernel/trace/trace_events.c +15 -0
+static int probe_remove_event_call(struct ftrace_event_call *call)
+{
+	struct trace_array *tr;
+	struct ftrace_event_file *file;
+
+#ifdef CONFIG_PERF_EVENTS
+	if (call->perf_refcount)
+		return -EBUSY;
+#endif
+	do_for_each_event_file(tr, file) {
+		if (file->event_call != call)
+			continue;
+		/*
+		 * We can't rely on ftrace_event_enable_disable(enable => 0)
+		 * we are going to do, FTRACE_EVENT_FL_SOFT_MODE can suppress
Quoted 9 lines

kernel/trace/trace_events.c static void __trace_remove_event_call(struct ftrace_event_call *call) · line 1731

Quoted 5 lines
kernel/trace/trace_events.c +5 -0
+		 * TRACE_REG_UNREGISTER.
+		 */
+		if (file->flags & FTRACE_EVENT_FL_ENABLED)
+			return -EBUSY;
+		break;

I'm going to modify the patch to include a comment here about the break being used to go to the next trace_array and not leaving the loop.

Screw it, I'm going to just add a separate patch to include it.

-- Steve

author Acked-by Masami Hiramatsu
#6 lore raw
In reply to Steven Rostedt
Quoted 8 lines · Steven Rostedt
Quoted 3 lines · Oleg Nesterov

Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven already has the patches.

Can you refresh my memory. My INBOX is quite full. What were the subjects for these patches?

Please see the attached mbox. And notice who is the author ;) I also added the ack from Masami.

But if you only need the subjects:

[RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open [RFC][PATCH 4/4] tracing/uprobes: Fail to unregister if probe event files are open

Oleg.

Quoted 1 line

From [email protected] Thu Jul 4 05:40:42 2013

Return-Path: [email protected]
Received: from zmta04.collab.prod.int.phx2.redhat.com (LHLO
zmta04.collab.prod.int.phx2.redhat.com) (10.5.81.11) by zmail13.collab.prod.int.phx2.redhat.com with LMTP; Wed, 3 Jul 2013 23:40:42 -0400 (EDT)
Received: from zmta04.collab.prod.int.phx2.redhat.com (localhost [127.0.0.1])

	by zmta04.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id AC30BD1C4E
	for <[email protected]>; Wed,  3 Jul 2013 23:40:42 -0400 (EDT)
Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25])
	by zmta04.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id A6B98D1C4B
	for <[email protected]>; Wed,  3 Jul 2013 23:40:42 -0400 (EDT)
Received: from mx1.redhat.com (ext-mx14.extmail.prod.ext.phx2.redhat.com [10.5.110.19])
	by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r643efGV017597
	for <[email protected]>; Wed, 3 Jul 2013 23:40:42 -0400
Received: from hrndva-omtalb.mail.rr.com (hrndva-omtalb.mail.rr.com [71.74.56.122])
	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r643edMW011522
	for <[email protected]>; Wed, 3 Jul 2013 23:40:40 -0400

X-Authority-Analysis: v=2.0 cv=Du3UCRD+ c=1 sm=0 a=Sro2XwOs0tJUSHxCKfOySw==:17 a=Drc5e87SC40A:10 a=Ciwy3NGCPMMA:10 a=odXrFLFfAhoA:10 a=5SG0PmZfjMsA:10 a=bbbx4UPp9XUA:10 a=meVymXHHAAAA:8 a=KGjhK52YXX0A:10 a=BdHU2U2mihYA:10 a=Q00xHw_dJstCAtue8jQA:9 a=jeBq3FmKZ4MA:10 a=Sro2XwOs0tJUSHxCKfOySw==:117
X-Cloudmark-Score: 0
X-Authenticated-User:
X-Originating-IP: 67.255.60.225
Received: from [67.255.60.225] ([67.255.60.225:50276] helo=gandalf.local.home)

	by hrndva-oedge02.mail.rr.com (envelope-from <[email protected]>)
	(ecelerity 2.2.3.46 r()) with ESMTP
	id 37/75-21735-7BEE4D15; Thu, 04 Jul 2013 03:40:39 +0000
Received: from rostedt by gandalf.local.home with local (Exim 4.80)
	(envelope-from <[email protected]>)
	id 1UuaPO-0007TA-T2; Wed, 03 Jul 2013 23:40:38 -0400

Message-Id: <[email protected]>
User-Agent: quilt/0.60-1
Date: Wed, 03 Jul 2013 23:33:50 -0400
From: Steven Rostedt <[email protected]>
To: [email protected]
Cc: Oleg Nesterov <[email protected]>,
Masami Hiramatsu <[email protected]>,
"zhangwei(Jovi)" <[email protected]>,
Jiri Olsa <[email protected]>, Peter Zijlstra <[email protected]>, Arnaldo Carvalho de Melo <[email protected]>,
Srikar Dronamraju <[email protected]>,
Frederic Weisbecker <[email protected]>,
Ingo Molnar <[email protected]>,
Andrew Morton <[email protected]>
Subject: [RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open
References: <[email protected]>
Content-Disposition: inline; filename=0003-tracing-kprobes-Fail-to-unregister-if-probe-event-fi.patch
X-RedHat-Spam-Score: -2.009 (BAYES_00,DCC_REPUT_13_19,RCVD_IN_DNSWL_NONE,URIBL_BLOCKED)
X-Scanned-By: MIMEDefang 2.68 on 10.5.11.25
X-Scanned-By: MIMEDefang 2.68 on 10.5.110.19
Status: RO
Content-Length: 3008
Lines: 100

From: "Steven Rostedt (Red Hat)" <[email protected]>

When one of the event files is opened, we need to prevent them from being removed. Modules do with with the module owner set (automated from the VFS layer). The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened (the others are not that big of a deal, as they only reference the event calls which still exist when an instance disappears). But kprobes and uprobes do not have any protection.

 # cd /sys/kernel/debug/tracing
 # echo 'p:sigprocmask sigprocmask' > kprobe_events || exit -1
 # enable_probe() {
    sleep 10
    echo 1
 }
 # file=events/kprobes/sigprocmask/enable
 # enable_probe > $file &
Quoted 1 line

kprobe_events

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Trying to create the probe again fails:

 # echo 'p:sigprocmask sigprocmask' > kprobe_events
 # cat kprobe_events
p:kprobes/sigprocmask sigprocmask
 # ls events/kprobes/
enable  filter

Have the unregister probe fail when the event files are open, in use are used by perf.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_kprobe.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
kernel/trace/trace_kprobe.c +15 -6
index 7ed6976..ffcaf42 100644
@@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
 }
 
 static int register_probe_event(struct trace_probe *tp);
-static void unregister_probe_event(struct trace_probe *tp);
+static int unregister_probe_event(struct trace_probe *tp);
 
 static DEFINE_MUTEX(probe_lock);
 static LIST_HEAD(probe_list);

@@ -340,9 +340,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	if (trace_probe_is_enabled(tp))
 		return -EBUSY;
 
+	/* Will fail if probe is being used by ftrace or perf */
+	if (unregister_probe_event(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
-	unregister_probe_event(tp);
 
 	return 0;
 }

@@ -621,7 +624,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}
 

@@ -1242,11 +1247,15 @@ static int register_probe_event(struct trace_probe *tp)
 	return ret;
 }
 
-static void unregister_probe_event(struct trace_probe *tp)
+static int unregister_probe_event(struct trace_probe *tp)
 {
+	int ret;
+
 	/* tp->event is unregistered in trace_remove_event_call() */
-	trace_remove_event_call(&tp->call);
-	kfree(tp->call.print_fmt);
+	ret = trace_remove_event_call(&tp->call);
+	if (!ret)
+		kfree(tp->call.print_fmt);
+	return ret;
 }
 
 /* Make a debugfs interface for controlling probe points */
Signature · 1 line
1.7.10.4
Quoted 1 line

From [email protected] Tue Jul 30 10:15:51 2013

Return-Path: [email protected]
Received: from zmta05.collab.prod.int.phx2.redhat.com (LHLO
zmta05.collab.prod.int.phx2.redhat.com) (10.5.81.12) by zmail13.collab.prod.int.phx2.redhat.com with LMTP; Tue, 30 Jul 2013 04:15:51 -0400 (EDT)
Received: from zmta05.collab.prod.int.phx2.redhat.com (localhost [127.0.0.1])

	by zmta05.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id C7B26F28DE;
	Tue, 30 Jul 2013 04:15:51 -0400 (EDT)
Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24])
	by zmta05.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id 6B231F2A08;
	Tue, 30 Jul 2013 04:15:51 -0400 (EDT)
Received: from mx1.redhat.com (ext-mx11.extmail.prod.ext.phx2.redhat.com [10.5.110.16])
	by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r6U8Fo7F030049;
	Tue, 30 Jul 2013 04:15:50 -0400
Received: from mail7.hitachi.co.jp (mail7.hitachi.co.jp [133.145.228.42])
	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6U8FkxR015000;
	Tue, 30 Jul 2013 04:15:47 -0400
Received: from mlsv3.hitachi.co.jp (unknown [133.144.234.166])
	by mail7.hitachi.co.jp (Postfix) with ESMTP id 181D137AC6;
	Tue, 30 Jul 2013 17:15:46 +0900 (JST)
Received: from mfilter05.hitachi.co.jp by mlsv3.hitachi.co.jp (8.13.1/8.13.1) id r6U8Fke3005277; Tue, 30 Jul 2013 17:15:46 +0900
Received: from vshuts04.hitachi.co.jp (vshuts04.hitachi.co.jp [10.201.6.86])
	by mfilter05.hitachi.co.jp (Switch-3.3.4/Switch-3.3.4) with ESMTP id r6U8FfkM003254;
	Tue, 30 Jul 2013 17:15:45 +0900
Received: from gmml27.itg.hitachi.co.jp (unknown [158.213.165.130])
	by vshuts04.hitachi.co.jp (Postfix) with ESMTP id 91D9F140058;
	Tue, 30 Jul 2013 17:15:44 +0900 (JST)

Received: from [10.198.208.96] by gmml27.itg.hitachi.co.jp (AIX5.2/8.11.6p2/8.11.0) id r6U8Fix5128402; Tue, 30 Jul 2013 17:15:44 +0900
Message-ID: <[email protected]>
Date: Tue, 30 Jul 2013 17:15:42 +0900
From: Masami Hiramatsu <[email protected]>
Organization: Hitachi, Ltd., Japan
User-Agent: Mozilla/5.0 (Windows NT 5.2; rv:13.0) Gecko/20120614 Thunderbird/13.0.1
MIME-Version: 1.0
To: Steven Rostedt <[email protected]>
Cc: [email protected], Oleg Nesterov <[email protected]>,
"zhangwei(Jovi)" <[email protected]>,
Jiri Olsa <[email protected]>, Peter Zijlstra <[email protected]>,
Arnaldo Carvalho de Melo <[email protected]>,
Srikar Dronamraju <[email protected]>,
Frederic Weisbecker <[email protected]>,
Ingo Molnar <[email protected]>,
Andrew Morton <[email protected]>
Subject: Re: [RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe
event files are open
References: <[email protected]> <[email protected]>
In-Reply-To: <[email protected]>
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit
X-RedHat-Spam-Score: -2.6 (BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS,URIBL_BLOCKED)
X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24
X-Scanned-By: MIMEDefang 2.68 on 10.5.110.16
Status: RO
Content-Length: 3609
Lines: 116

Quoted 25 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

When one of the event files is opened, we need to prevent them from being removed. Modules do with with the module owner set (automated from the VFS layer). The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened (the others are not that big of a deal, as they only reference the event calls which still exist when an instance disappears). But kprobes and uprobes do not have any protection.

 # cd /sys/kernel/debug/tracing
 # echo 'p:sigprocmask sigprocmask' > kprobe_events || exit -1
 # enable_probe() {
    sleep 10
    echo 1
 }
 # file=events/kprobes/sigprocmask/enable
 # enable_probe > $file &
Quoted 1 line

kprobe_events

The above will corrupt the kprobe system, as the write to the enable file will happen after the kprobe was deleted.

Trying to create the probe again fails:

Quoted 9 lines
 # echo 'p:sigprocmask sigprocmask' > kprobe_events
 # cat kprobe_events
p:kprobes/sigprocmask sigprocmask
 # ls events/kprobes/
enable  filter

Have the unregister probe fail when the event files are open, in use are used by perf.

Now, since the commit a232e270d makes sure that disable_trace_probe() waits until all running handlers are done, this patch has no problem.

Acked-by Masami Hiramatsu <[email protected]>

Thank you!

Quoted 63 lines
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_kprobe.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
kernel/trace/trace_kprobe.c +15 -6
index 7ed6976..ffcaf42 100644
@@ -95,7 +95,7 @@ static __kprobes bool trace_probe_is_on_module(struct trace_probe *tp)
 }
 
 static int register_probe_event(struct trace_probe *tp);
-static void unregister_probe_event(struct trace_probe *tp);
+static int unregister_probe_event(struct trace_probe *tp);
 
 static DEFINE_MUTEX(probe_lock);
 static LIST_HEAD(probe_list);

@@ -340,9 +340,12 @@ static int unregister_trace_probe(struct trace_probe *tp)
 	if (trace_probe_is_enabled(tp))
 		return -EBUSY;
 
+	/* Will fail if probe is being used by ftrace or perf */
+	if (unregister_probe_event(tp))
+		return -EBUSY;
+
 	__unregister_trace_probe(tp);
 	list_del(&tp->list);
-	unregister_probe_event(tp);
 
 	return 0;
 }

@@ -621,7 +624,9 @@ static int release_all_trace_probes(void)
 	/* TODO: Use batch unregistration */
 	while (!list_empty(&probe_list)) {
 		tp = list_entry(probe_list.next, struct trace_probe, list);
-		unregister_trace_probe(tp);
+		ret = unregister_trace_probe(tp);
+		if (ret)
+			goto end;
 		free_trace_probe(tp);
 	}
 

@@ -1242,11 +1247,15 @@ static int register_probe_event(struct trace_probe *tp)
 	return ret;
 }
 
-static void unregister_probe_event(struct trace_probe *tp)
+static int unregister_probe_event(struct trace_probe *tp)
 {
+	int ret;
+
 	/* tp->event is unregistered in trace_remove_event_call() */
-	trace_remove_event_call(&tp->call);
-	kfree(tp->call.print_fmt);
+	ret = trace_remove_event_call(&tp->call);
+	if (!ret)
+		kfree(tp->call.print_fmt);
+	return ret;
 }
 
 /* Make a debugfs interface for controlling probe points */
Signature · 4 lines
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
Quoted 1 line

From [email protected] Thu Jul 4 05:40:42 2013

Return-Path: [email protected]
Received: from zmta05.collab.prod.int.phx2.redhat.com (LHLO
zmta05.collab.prod.int.phx2.redhat.com) (10.5.81.12) by zmail13.collab.prod.int.phx2.redhat.com with LMTP; Wed, 3 Jul 2013 23:40:42 -0400 (EDT)
Received: from zmta05.collab.prod.int.phx2.redhat.com (localhost [127.0.0.1])

	by zmta05.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id AFC03F2D19;
	Wed,  3 Jul 2013 23:40:42 -0400 (EDT)
Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22])
	by zmta05.collab.prod.int.phx2.redhat.com (Postfix) with ESMTP id A6FF3F2D17;
	Wed,  3 Jul 2013 23:40:42 -0400 (EDT)
Received: from mx1.redhat.com (ext-mx13.extmail.prod.ext.phx2.redhat.com [10.5.110.18])
	by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r643egZa017120;
	Wed, 3 Jul 2013 23:40:42 -0400
Received: from hrndva-omtalb.mail.rr.com (hrndva-omtalb.mail.rr.com [71.74.56.122])
	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r643edx3020216;
	Wed, 3 Jul 2013 23:40:40 -0400

X-Authority-Analysis: v=2.0 cv=Odoa/2vY c=1 sm=0 a=Sro2XwOs0tJUSHxCKfOySw==:17 a=Drc5e87SC40A:10 a=Ciwy3NGCPMMA:10 a=kwCRlE8uItcA:10 a=5SG0PmZfjMsA:10 a=bbbx4UPp9XUA:10 a=meVymXHHAAAA:8 a=KGjhK52YXX0A:10 a=e-0I92L2Hf8A:10 a=ngVV-1ifIcZ1sTZkOpAA:9 a=jeBq3FmKZ4MA:10 a=Sro2XwOs0tJUSHxCKfOySw==:117
X-Cloudmark-Score: 0
X-Authenticated-User:
X-Originating-IP: 67.255.60.225
Received: from [67.255.60.225] ([67.255.60.225:50364] helo=gandalf.local.home)

	by hrndva-oedge03.mail.rr.com (envelope-from <[email protected]>)
	(ecelerity 2.2.3.46 r()) with ESMTP
	id AF/AC-07405-7BEE4D15; Thu, 04 Jul 2013 03:40:39 +0000
Received: from rostedt by gandalf.local.home with local (Exim 4.80)
	(envelope-from <[email protected]>)
	id 1UuaPP-0007Tl-2D; Wed, 03 Jul 2013 23:40:39 -0400

Message-Id: <[email protected]>
User-Agent: quilt/0.60-1
Date: Wed, 03 Jul 2013 23:33:51 -0400
From: Steven Rostedt <[email protected]>
To: [email protected]
Cc: Oleg Nesterov <[email protected]>,
Masami Hiramatsu <[email protected]>,
"zhangwei(Jovi)" <[email protected]>,
Jiri Olsa <[email protected]>, Peter Zijlstra <[email protected]>, Arnaldo Carvalho de Melo <[email protected]>,
Srikar Dronamraju <[email protected]>,
Frederic Weisbecker <[email protected]>,
Ingo Molnar <[email protected]>,
Andrew Morton <[email protected]>
Subject: [RFC][PATCH 4/4] tracing/uprobes: Fail to unregister if probe event files are open
References: <[email protected]>
Content-Disposition: inline; filename=0004-tracing-uprobes-Fail-to-unregister-if-probe-event-fi.patch
X-RedHat-Spam-Score: -1.909 (BAYES_00,RCVD_IN_DNSWL_NONE,URIBL_BLOCKED)
X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22
X-Scanned-By: MIMEDefang 2.68 on 10.5.110.18
Status: RO
Content-Length: 4112
Lines: 148

From: "Steven Rostedt (Red Hat)" <[email protected]>

When one of the event files is opened, we need to prevent them from being removed. Modules do with with the module owner set (automated from the VFS layer). The ftrace buffer instances have a ref count added to the trace_array when the enabled file is opened (the others are not that big of a deal, as they only reference the event calls which still exist when an instance disappears). But kprobes and uprobes do not have any protection.

Have the unregister probe fail when the event files are open, in use are used by perf.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/trace_uprobe.c |   48 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 12 deletions(-)
kernel/trace/trace_uprobe.c +36 -12

114 lines not on this page · raw message

Signature · 1 line
1.7.10.4
maintainer
#7 lore raw
Quoted 11 lines · Oleg Nesterov
Quoted 8 lines · Steven Rostedt
Quoted 3 lines · Oleg Nesterov

Now we are ready to change trace_kprobe.c and trace_uprobe.c, Steven already has the patches.

Can you refresh my memory. My INBOX is quite full. What were the subjects for these patches?

Please see the attached mbox. And notice who is the author ;)

Heh, who is that guy?

Quoted 1 line

I also added the ack from Masami.

Thanks, but I'll take in from the email as that's how my scripts work (I cc'd myself)

-- Steve

Quoted 7 lines

But if you only need the subjects:

[RFC][PATCH 3/4] tracing/kprobes: Fail to unregister if probe event files are open [RFC][PATCH 4/4] tracing/uprobes: Fail to unregister if probe event files are open

Oleg.