FAILED: patch "[PATCH] tracing: Have trace_event_file have ref counters" failed to apply to 5.4-stable tree

PATCH [email protected] 2 participants 8 messages Nov 6, 2023 – Nov 24, 2023

Subsystems (2)

Stable branch Tracing

lore

Share & tools

lore

[email protected]

author maintainer

2023-11-06 11:40 UTC #0 lore raw

The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-5.4.y
git checkout FETCH_HEAD
git cherry-pick -x bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2023110614-natural-tweak-9ee4@gregkh' --subject-prefix 'PATCH 5.4.y' HEAD^..

Possible dependencies:

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Google)" <[email protected]>
Date: Tue, 31 Oct 2023 12:24:53 -0400
Subject: [PATCH] tracing: Have trace_event_file have ref counters

The following can crash the kernel:

 # cd /sys/kernel/tracing
 # echo 'p:sched schedule' > kprobe_events
 # exec 5>>events/kprobes/sched/enable
 # > kprobe_events
 # exec 5>&-

The above commands:

1. Change directory to the tracefs directory
2. Create a kprobe event (doesn't matter what one)
3. Open bash file descriptor 5 on the enable file of the kprobe event
4. Delete the kprobe event (removes the files too)
5. Close the bash file descriptor 5

The above causes a crash!

 BUG: kernel NULL pointer dereference, address: 0000000000000028
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 0 P4D 0
 Oops: 0000 [#1] PREEMPT SMP PTI
 CPU: 6 PID: 877 Comm: bash Not tainted 6.5.0-rc4-test-00008-g2c6b6b1029d4-dirty #186
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
 RIP: 0010:tracing_release_file_tr+0xc/0x50

What happens here is that the kprobe event creates a trace_event_file "file" descriptor that represents the file in tracefs to the event. It maintains state of the event (is it enabled for the given instance?). Opening the "enable" file gets a reference to the event "file" descriptor via the open file descriptor. When the kprobe event is deleted, the file is also deleted from the tracefs system which also frees the event "file" descriptor.

But as the tracefs file is still opened by user space, it will not be totally removed until the final dput() is called on it. But this is not true with the event "file" descriptor that is already freed. If the user does a write to or simply closes the file descriptor it will reference the event "file" descriptor that was just freed, causing a use-after-free bug.

To solve this, add a ref count to the event "file" descriptor as well as a new flag called "FREED". The "file" will not be freed until the last reference is released. But the FREE flag will be set when the event is removed to prevent any more modifications to that event from happening, even if there's still a reference to the event "file" descriptor.

Link https://lore.kernel.org/linux-trace-kernel/[email protected]/ open here

Link https://lore.kernel.org/linux-trace-kernel/[email protected] open here

Cc [email protected]

Cc Mark Rutland <[email protected]>

Fixes f5ca233e2e66d ("tracing: Increase trace array ref count on enable and filter files")

Reported-by Beau Belgrave <[email protected]>

Tested-by Beau Belgrave <[email protected]>

Reviewed-by Masami Hiramatsu (Google) <[email protected]>

Signed-off-by Steven Rostedt (Google) <[email protected]>

include/linux/trace_events.h +4 -0

index 12207dc6722d..696f8dc4aa53 100644
@@ -492,6 +492,7 @@ enum {
 	EVENT_FILE_FL_TRIGGER_COND_BIT,
 	EVENT_FILE_FL_PID_FILTER_BIT,
 	EVENT_FILE_FL_WAS_ENABLED_BIT,
+	EVENT_FILE_FL_FREED_BIT,
 };
 
 extern struct trace_event_file *trace_get_event_file(const char *instance,

@@ -630,6 +631,7 @@ extern int __kprobe_event_add_fields(struct dynevent_cmd *cmd, ...);
  *  TRIGGER_COND  - When set, one or more triggers has an associated filter
  *  PID_FILTER    - When set, the event is filtered based on pid
  *  WAS_ENABLED   - Set when enabled to know to clear trace on module removal
+ *  FREED         - File descriptor is freed, all fields should be considered invalid
  */
 enum {
 	EVENT_FILE_FL_ENABLED		= (1 << EVENT_FILE_FL_ENABLED_BIT),

@@ -643,6 +645,7 @@ enum {
 	EVENT_FILE_FL_TRIGGER_COND	= (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
 	EVENT_FILE_FL_PID_FILTER	= (1 << EVENT_FILE_FL_PID_FILTER_BIT),
 	EVENT_FILE_FL_WAS_ENABLED	= (1 << EVENT_FILE_FL_WAS_ENABLED_BIT),
+	EVENT_FILE_FL_FREED		= (1 << EVENT_FILE_FL_FREED_BIT),
 };
 
 struct trace_event_file {

@@ -671,6 +674,7 @@ struct trace_event_file {
 	 * caching and such. Which is mostly OK ;-)
 	 */
 	unsigned long		flags;
+	atomic_t		ref;	/* ref count for opened files */
 	atomic_t		sm_ref;	/* soft-mode reference counter */
 	atomic_t		tm_ref;	/* trigger-mode reference counter */
 };

kernel/trace/trace.c +15 -0

index 2539cfc20a97..9aebf904ff97 100644
@@ -4978,6 +4978,20 @@ int tracing_open_file_tr(struct inode *inode, struct file *filp)
 	if (ret)
 		return ret;
 
+	mutex_lock(&event_mutex);
+
+	/* Fail if the file is marked for removal */
+	if (file->flags & EVENT_FILE_FL_FREED) {
+		trace_array_put(file->tr);
+		ret = -ENODEV;
+	} else {
+		event_file_get(file);
+	}
+
+	mutex_unlock(&event_mutex);
+	if (ret)
+		return ret;
+
 	filp->private_data = inode->i_private;
 
 	return 0;

@@ -4988,6 +5002,7 @@ int tracing_release_file_tr(struct inode *inode, struct file *filp)
 	struct trace_event_file *file = inode->i_private;
 
 	trace_array_put(file->tr);
+	event_file_put(file);
 
 	return 0;
 }

kernel/trace/trace.h +3 -0

index 0e1405abf4f7..b7f4ea25a194 100644
@@ -1669,6 +1669,9 @@ extern void event_trigger_unregister(struct event_command *cmd_ops,
 				     char *glob,
 				     struct event_trigger_data *trigger_data);
 
+extern void event_file_get(struct trace_event_file *file);
+extern void event_file_put(struct trace_event_file *file);
+
 /**
  * struct event_trigger_ops - callbacks for trace event triggers
  *

kernel/trace/trace_events.c +27 -4

index f9e3e24d8796..f29e815ca5b2 100644
@@ -990,13 +990,35 @@ static void remove_subsystem(struct trace_subsystem_dir *dir)
 	}
 }
 
+void event_file_get(struct trace_event_file *file)
+{
+	atomic_inc(&file->ref);
+}
+
+void event_file_put(struct trace_event_file *file)
+{
+	if (WARN_ON_ONCE(!atomic_read(&file->ref))) {
+		if (file->flags & EVENT_FILE_FL_FREED)
+			kmem_cache_free(file_cachep, file);
+		return;
+	}
+
+	if (atomic_dec_and_test(&file->ref)) {
+		/* Count should only go to zero when it is freed */
+		if (WARN_ON_ONCE(!(file->flags & EVENT_FILE_FL_FREED)))
+			return;
+		kmem_cache_free(file_cachep, file);
+	}
+}
+
 static void remove_event_file_dir(struct trace_event_file *file)
 {
 	eventfs_remove_dir(file->ei);
 	list_del(&file->list);
 	remove_subsystem(file->system);
 	free_event_filter(file->filter);
-	kmem_cache_free(file_cachep, file);
+	file->flags |= EVENT_FILE_FL_FREED;
+	event_file_put(file);
 }
 
 /*

@@ -1369,7 +1391,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		flags = file->flags;
 	mutex_unlock(&event_mutex);
 
-	if (!file)
+	if (!file || flags & EVENT_FILE_FL_FREED)
 		return -ENODEV;
 
 	if (flags & EVENT_FILE_FL_ENABLED &&

@@ -1403,7 +1425,7 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 		ret = -ENODEV;
 		mutex_lock(&event_mutex);
 		file = event_file_data(filp);
-		if (likely(file)) {
+		if (likely(file && !(file->flags & EVENT_FILE_FL_FREED))) {
 			ret = tracing_update_buffers(file->tr);
 			if (ret < 0) {
 				mutex_unlock(&event_mutex);

@@ -1683,7 +1705,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 
 	mutex_lock(&event_mutex);
 	file = event_file_data(filp);
-	if (file)
+	if (file && !(file->flags & EVENT_FILE_FL_FREED))
 		print_event_filter(file, s);
 	mutex_unlock(&event_mutex);

@@ -2902,6 +2924,7 @@ trace_create_new_event(struct trace_event_call *call,
 	atomic_set(&file->tm_ref, 0);
 	INIT_LIST_HEAD(&file->triggers);
 	list_add(&file->list, &tr->events);
+	event_file_get(file);
 
 	return file;
 }

kernel/trace/trace_events_filter.c +3 -0

index 33264e510d16..0c611b281a5b 100644
@@ -2349,6 +2349,9 @@ int apply_event_filter(struct trace_event_file *file, char *filter_string)
 	struct event_filter *filter = NULL;
 	int err;
 
+	if (file->flags & EVENT_FILE_FL_FREED)
+		return -ENODEV;
+
 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_disable(file);
 		filter = event_filter(file);

Steven Rostedt

maintainer Tested-by Beau Belgrave Reviewed-by Masami Hiramatsu (Google)

2023-11-06 19:48 UTC #1 lore raw

[ This should work for v5.4 ]

From: "Steven Rostedt (Google)" <[email protected]>
Subject: [PATCH] tracing: Have trace_event_file have ref counters

commit bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 upstream

The following can crash the kernel:

 # cd /sys/kernel/tracing
 # echo 'p:sched schedule' > kprobe_events
 # exec 5>>events/kprobes/sched/enable
 # > kprobe_events
 # exec 5>&-

The above commands:

The above causes a crash!

 BUG: kernel NULL pointer dereference, address: 0000000000000028
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 0 P4D 0
 Oops: 0000 [#1] PREEMPT SMP PTI
 CPU: 6 PID: 877 Comm: bash Not tainted 6.5.0-rc4-test-00008-g2c6b6b1029d4-dirty #186
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
 RIP: 0010:tracing_release_file_tr+0xc/0x50

Link https://lore.kernel.org/linux-trace-kernel/[email protected]/ open here

Link https://lore.kernel.org/linux-trace-kernel/[email protected] open here

Cc [email protected]

Cc Mark Rutland <[email protected]>

Fixes f5ca233e2e66d ("tracing: Increase trace array ref count on enable and filter files")

Reported-by Beau Belgrave <[email protected]>

Tested-by Beau Belgrave <[email protected]>

Reviewed-by Masami Hiramatsu (Google) <[email protected]>

Signed-off-by Steven Rostedt (Google) <[email protected]>

---
 include/linux/trace_events.h       |    4 +++
 kernel/trace/trace.c               |   15 ++++++++++++++
 kernel/trace/trace.h               |    3 ++
 kernel/trace/trace_events.c        |   38 +++++++++++++++++++++++++------------
 kernel/trace/trace_events_filter.c |    3 ++
 5 files changed, 51 insertions(+), 12 deletions(-)

include/linux/trace_events.h +4 -0

Index: linux-trace.git/include/linux/trace_events.h
===================================================================
@@ -341,6 +341,7 @@ enum {
 	EVENT_FILE_FL_TRIGGER_COND_BIT,
 	EVENT_FILE_FL_PID_FILTER_BIT,
 	EVENT_FILE_FL_WAS_ENABLED_BIT,
+	EVENT_FILE_FL_FREED_BIT,
 };
 
 /*

@@ -357,6 +358,7 @@ enum {
  *  TRIGGER_COND  - When set, one or more triggers has an associated filter
  *  PID_FILTER    - When set, the event is filtered based on pid
  *  WAS_ENABLED   - Set when enabled to know to clear trace on module removal
+ *  FREED         - File descriptor is freed, all fields should be considered invalid
  */
 enum {
 	EVENT_FILE_FL_ENABLED		= (1 << EVENT_FILE_FL_ENABLED_BIT),

@@ -370,6 +372,7 @@ enum {
 	EVENT_FILE_FL_TRIGGER_COND	= (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
 	EVENT_FILE_FL_PID_FILTER	= (1 << EVENT_FILE_FL_PID_FILTER_BIT),
 	EVENT_FILE_FL_WAS_ENABLED	= (1 << EVENT_FILE_FL_WAS_ENABLED_BIT),
+	EVENT_FILE_FL_FREED		= (1 << EVENT_FILE_FL_FREED_BIT),
 };
 
 struct trace_event_file {

@@ -398,6 +401,7 @@ struct trace_event_file {
 	 * caching and such. Which is mostly OK ;-)
 	 */
 	unsigned long		flags;
+	atomic_t		ref;	/* ref count for opened files */
 	atomic_t		sm_ref;	/* soft-mode reference counter */
 	atomic_t		tm_ref;	/* trigger-mode reference counter */
 };

kernel/trace/trace.c +15 -0

Index: linux-trace.git/kernel/trace/trace.c
===================================================================
@@ -4257,6 +4257,20 @@ int tracing_open_file_tr(struct inode *i
 	if (ret)
 		return ret;
 
+	mutex_lock(&event_mutex);
+
+	/* Fail if the file is marked for removal */
+	if (file->flags & EVENT_FILE_FL_FREED) {
+		trace_array_put(file->tr);
+		ret = -ENODEV;
+	} else {
+		event_file_get(file);
+	}
+
+	mutex_unlock(&event_mutex);
+	if (ret)
+		return ret;
+
 	filp->private_data = inode->i_private;
 
 	return 0;

@@ -4267,6 +4281,7 @@ int tracing_release_file_tr(struct inode
 	struct trace_event_file *file = inode->i_private;
 
 	trace_array_put(file->tr);
+	event_file_put(file);
 
 	return 0;
 }

kernel/trace/trace.h +3 -0

Index: linux-trace.git/kernel/trace/trace.h
===================================================================
@@ -1696,6 +1696,9 @@ extern int register_event_command(struct
 extern int unregister_event_command(struct event_command *cmd);
 extern int register_trigger_hist_enable_disable_cmds(void);
 
+extern void event_file_get(struct trace_event_file *file);
+extern void event_file_put(struct trace_event_file *file);
+
 /**
  * struct event_trigger_ops - callbacks for trace event triggers
  *

kernel/trace/trace_events.c +26 ~~-12~~

Index: linux-trace.git/kernel/trace/trace_events.c
===================================================================
@@ -698,21 +698,34 @@ static void remove_subsystem(struct trac
 	}
 }
 
+void event_file_get(struct trace_event_file *file)
+{
+	atomic_inc(&file->ref);
+}
+
+void event_file_put(struct trace_event_file *file)
+{
+	if (WARN_ON_ONCE(!atomic_read(&file->ref))) {
+		if (file->flags & EVENT_FILE_FL_FREED)
+			kmem_cache_free(file_cachep, file);
+		return;
+	}
+
+	if (atomic_dec_and_test(&file->ref)) {
+		/* Count should only go to zero when it is freed */
+		if (WARN_ON_ONCE(!(file->flags & EVENT_FILE_FL_FREED)))
+			return;
+		kmem_cache_free(file_cachep, file);
+	}
+}
+
 static void remove_event_file_dir(struct trace_event_file *file)
 {
 	struct dentry *dir = file->dir;
 	struct dentry *child;
 
-	if (dir) {
-		spin_lock(&dir->d_lock);	/* probably unneeded */
-		list_for_each_entry(child, &dir->d_subdirs, d_child) {
-			if (d_really_is_positive(child))	/* probably unneeded */
-				d_inode(child)->i_private = NULL;
-		}
-		spin_unlock(&dir->d_lock);
-
+	if (dir)
 		tracefs_remove_recursive(dir);
-	}
 
 	list_del(&file->list);
 	remove_subsystem(file->system);

@@ -1033,7 +1046,7 @@ event_enable_read(struct file *filp, cha
 		flags = file->flags;
 	mutex_unlock(&event_mutex);
 
-	if (!file)
+	if (!file || flags & EVENT_FILE_FL_FREED)
 		return -ENODEV;
 
 	if (flags & EVENT_FILE_FL_ENABLED &&

@@ -1071,7 +1084,7 @@ event_enable_write(struct file *filp, co
 		ret = -ENODEV;
 		mutex_lock(&event_mutex);
 		file = event_file_data(filp);
-		if (likely(file))
+		if (likely(file && !(file->flags & EVENT_FILE_FL_FREED)))
 			ret = ftrace_event_enable_disable(file, val);
 		mutex_unlock(&event_mutex);
 		break;

@@ -1340,7 +1353,7 @@ event_filter_read(struct file *filp, cha
 
 	mutex_lock(&event_mutex);
 	file = event_file_data(filp);
-	if (file)
+	if (file && !(file->flags & EVENT_FILE_FL_FREED))
 		print_event_filter(file, s);
 	mutex_unlock(&event_mutex);

@@ -2264,6 +2277,7 @@ trace_create_new_event(struct trace_even
 	atomic_set(&file->tm_ref, 0);
 	INIT_LIST_HEAD(&file->triggers);
 	list_add(&file->list, &tr->events);
+	event_file_get(file);
 
 	return file;
 }

kernel/trace/trace_events_filter.c +3 -0

Index: linux-trace.git/kernel/trace/trace_events_filter.c
===================================================================
@@ -1800,6 +1800,9 @@ int apply_event_filter(struct trace_even
 	struct event_filter *filter = NULL;
 	int err;
 
+	if (file->flags & EVENT_FILE_FL_FREED)
+		return -ENODEV;
+
 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_disable(file);
 		filter = event_filter(file);

Greg KH

author maintainer

2023-11-15 11:58 UTC #2 lore raw

Quoted 7 lines · Steven Rostedt

[ This should work for v5.4 ]
From: "Steven Rostedt (Google)" <[email protected]>
Subject: [PATCH] tracing: Have trace_event_file have ref counters
commit bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 upstream

All now queued up, thanks.

greg k-h

Greg KH

author maintainer

2023-11-15 12:04 UTC #3 lore raw

Quoted 10 lines · Greg KH

Quoted 7 lines · Steven Rostedt
[ This should work for v5.4 ]
From: "Steven Rostedt (Google)" <[email protected]>
Subject: [PATCH] tracing: Have trace_event_file have ref counters
commit bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 upstream
All now queued up, thanks.

No, wait, all of these break the build with this error:

kernel/trace/trace_events.c: In function ‘remove_event_file_dir’:
kernel/trace/trace_events.c:1015:24: error: unused variable ‘child’ [-Werror=unused-variable]
1015 | struct dentry *child;
| ^~~~~

So I'm going to drop them now :(

greg k-h

Steven Rostedt

maintainer

2023-11-15 17:33 UTC #4 lore raw

Quoted 12 lines · On Wed, 15 Nov 2023 07:04:42 -0500 Greg KH <[email protected]> wrote:

Quoted 10 lines · Greg KH
Quoted 7 lines · Steven Rostedt
[ This should work for v5.4 ]
From: "Steven Rostedt (Google)" <[email protected]>
Subject: [PATCH] tracing: Have trace_event_file have ref counters
commit bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 upstream
All now queued up, thanks.

Quoted 9 lines

No, wait, all of these break the build with this error:
kernel/trace/trace_events.c: In function ‘remove_event_file_dir’:
kernel/trace/trace_events.c:1015:24: error: unused variable ‘child’ [-Werror=unused-variable]
1015 | struct dentry *child;
| ^~~~~
So I'm going to drop them now :(

Ah, this patch I didn't run through all my tests, like I did with the 6.6 patches, so I didn't test with fail on warnings. The patch deleted the following code:

static void remove_event_file_dir(struct trace_event_file *file) {
struct dentry *dir = file->dir;
struct dentry *child;

- if (dir) {
- spin_lock(&dir->d_lock); /* probably unneeded */
- list_for_each_entry(child, &dir->d_subdirs, d_child) {
- if (d_really_is_positive(child)) /* probably unneeded */
- d_inode(child)->i_private = NULL;
- }
- spin_unlock(&dir->d_lock);
-
+ if (dir)
tracefs_remove_recursive(dir);
- }

list_del(&file->list);

The extra check that that utilized that child variable is no longer needed, and I forgot to delete the declaration of the child variable.

Did you just want to delete that, or do you want me to create a new patch?

-- Steve

Greg KH

author maintainer

2023-11-15 17:37 UTC #5 lore raw

Quoted 48 lines · Steven Rostedt

Quoted 12 lines · On Wed, 15 Nov 2023 07:04:42 -0500 Greg KH <[email protected]> wrote:
Quoted 10 lines · Greg KH
Quoted 7 lines · Steven Rostedt
[ This should work for v5.4 ]
From: "Steven Rostedt (Google)" <[email protected]>
Subject: [PATCH] tracing: Have trace_event_file have ref counters
commit bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 upstream
All now queued up, thanks.
Quoted 9 lines
No, wait, all of these break the build with this error:
kernel/trace/trace_events.c: In function ‘remove_event_file_dir’:
kernel/trace/trace_events.c:1015:24: error: unused variable ‘child’ [-Werror=unused-variable]
1015 | struct dentry *child;
| ^~~~~
So I'm going to drop them now :(
Ah, this patch I didn't run through all my tests, like I did with the 6.6 patches, so I didn't test with fail on warnings. The patch deleted the following code:
static void remove_event_file_dir(struct trace_event_file *file) {
kernel/trace/trace_events.c +1 -9
 	struct dentry *dir = file->dir;
 	struct dentry *child;
 
-	if (dir) {
-		spin_lock(&dir->d_lock);	/* probably unneeded */
-		list_for_each_entry(child, &dir->d_subdirs, d_child) {
-			if (d_really_is_positive(child))	/* probably unneeded */
-				d_inode(child)->i_private = NULL;
-		}
-		spin_unlock(&dir->d_lock);
-
+	if (dir)
 		tracefs_remove_recursive(dir);
-	}
 
 	list_del(&file->list);

Quoted 5 lines

The extra check that that utilized that child variable is no longer needed, and I forgot to delete the declaration of the child variable.
Did you just want to delete that, or do you want me to create a new patch?

I need all new patches, sorry, these are long gone from my queue.

greg k-h

Steven Rostedt

maintainer Tested-by Beau Belgrave Reviewed-by Masami Hiramatsu (Google)

2023-11-16 16:24 UTC #6 lore raw

In reply to [email protected]

From: "Steven Rostedt (Google)" <[email protected]>
Date: Tue, 31 Oct 2023 12:24:53 -0400
Subject: [PATCH] tracing: Have trace_event_file have ref counters

commit bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 upstream.

The following can crash the kernel:

 # cd /sys/kernel/tracing
 # echo 'p:sched schedule' > kprobe_events
 # exec 5>>events/kprobes/sched/enable
 # > kprobe_events
 # exec 5>&-

The above commands:

The above causes a crash!

 BUG: kernel NULL pointer dereference, address: 0000000000000028
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 0 P4D 0
 Oops: 0000 [#1] PREEMPT SMP PTI
 CPU: 6 PID: 877 Comm: bash Not tainted 6.5.0-rc4-test-00008-g2c6b6b1029d4-dirty #186
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
 RIP: 0010:tracing_release_file_tr+0xc/0x50

Link https://lore.kernel.org/linux-trace-kernel/[email protected]/ open here

Link https://lore.kernel.org/linux-trace-kernel/[email protected] open here

Cc [email protected]

Cc Mark Rutland <[email protected]>

Fixes f5ca233e2e66d ("tracing: Increase trace array ref count on enable and filter files")

Reported-by Beau Belgrave <[email protected]>

Tested-by Beau Belgrave <[email protected]>

Reviewed-by Masami Hiramatsu (Google) <[email protected]>

Signed-off-by Steven Rostedt (Google) <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 include/linux/trace_events.h       |  4 +++
 kernel/trace/trace.c               | 15 ++++++++++++
 kernel/trace/trace.h               |  3 +++
 kernel/trace/trace_events.c        | 39 ++++++++++++++++++++----------
 kernel/trace/trace_events_filter.c |  3 +++
 5 files changed, 51 insertions(+), 13 deletions(-)

include/linux/trace_events.h +4 -0

index a8e9d1a04f82..b8b87e7ba93f 100644
@@ -341,6 +341,7 @@ enum {
 	EVENT_FILE_FL_TRIGGER_COND_BIT,
 	EVENT_FILE_FL_PID_FILTER_BIT,
 	EVENT_FILE_FL_WAS_ENABLED_BIT,
+	EVENT_FILE_FL_FREED_BIT,
 };
 
 /*

@@ -357,6 +358,7 @@ enum {
  *  TRIGGER_COND  - When set, one or more triggers has an associated filter
  *  PID_FILTER    - When set, the event is filtered based on pid
  *  WAS_ENABLED   - Set when enabled to know to clear trace on module removal
+ *  FREED         - File descriptor is freed, all fields should be considered invalid
  */
 enum {
 	EVENT_FILE_FL_ENABLED		= (1 << EVENT_FILE_FL_ENABLED_BIT),

@@ -370,6 +372,7 @@ enum {
 	EVENT_FILE_FL_TRIGGER_COND	= (1 << EVENT_FILE_FL_TRIGGER_COND_BIT),
 	EVENT_FILE_FL_PID_FILTER	= (1 << EVENT_FILE_FL_PID_FILTER_BIT),
 	EVENT_FILE_FL_WAS_ENABLED	= (1 << EVENT_FILE_FL_WAS_ENABLED_BIT),
+	EVENT_FILE_FL_FREED		= (1 << EVENT_FILE_FL_FREED_BIT),
 };
 
 struct trace_event_file {

@@ -398,6 +401,7 @@ struct trace_event_file {
 	 * caching and such. Which is mostly OK ;-)
 	 */
 	unsigned long		flags;
+	atomic_t		ref;	/* ref count for opened files */
 	atomic_t		sm_ref;	/* soft-mode reference counter */
 	atomic_t		tm_ref;	/* trigger-mode reference counter */
 };

kernel/trace/trace.c +15 -0

index 85ad403006a2..a15dffe60722 100644
@@ -4257,6 +4257,20 @@ int tracing_open_file_tr(struct inode *inode, struct file *filp)
 	if (ret)
 		return ret;
 
+	mutex_lock(&event_mutex);
+
+	/* Fail if the file is marked for removal */
+	if (file->flags & EVENT_FILE_FL_FREED) {
+		trace_array_put(file->tr);
+		ret = -ENODEV;
+	} else {
+		event_file_get(file);
+	}
+
+	mutex_unlock(&event_mutex);
+	if (ret)
+		return ret;
+
 	filp->private_data = inode->i_private;
 
 	return 0;

@@ -4267,6 +4281,7 @@ int tracing_release_file_tr(struct inode *inode, struct file *filp)
 	struct trace_event_file *file = inode->i_private;
 
 	trace_array_put(file->tr);
+	event_file_put(file);
 
 	return 0;
 }

kernel/trace/trace.h +3 -0

index f1f54111b856..40644e06536c 100644
@@ -1696,6 +1696,9 @@ extern int register_event_command(struct event_command *cmd);
 extern int unregister_event_command(struct event_command *cmd);
 extern int register_trigger_hist_enable_disable_cmds(void);
 
+extern void event_file_get(struct trace_event_file *file);
+extern void event_file_put(struct trace_event_file *file);
+
 /**
  * struct event_trigger_ops - callbacks for trace event triggers
  *

kernel/trace/trace_events.c +26 ~~-13~~

index 4f42dd088079..958789fe4cef 100644
@@ -698,21 +698,33 @@ static void remove_subsystem(struct trace_subsystem_dir *dir)
 	}
 }
 
+void event_file_get(struct trace_event_file *file)
+{
+	atomic_inc(&file->ref);
+}
+
+void event_file_put(struct trace_event_file *file)
+{
+	if (WARN_ON_ONCE(!atomic_read(&file->ref))) {
+		if (file->flags & EVENT_FILE_FL_FREED)
+			kmem_cache_free(file_cachep, file);
+		return;
+	}
+
+	if (atomic_dec_and_test(&file->ref)) {
+		/* Count should only go to zero when it is freed */
+		if (WARN_ON_ONCE(!(file->flags & EVENT_FILE_FL_FREED)))
+			return;
+		kmem_cache_free(file_cachep, file);
+	}
+}
+
 static void remove_event_file_dir(struct trace_event_file *file)
 {
 	struct dentry *dir = file->dir;
-	struct dentry *child;
-
-	if (dir) {
-		spin_lock(&dir->d_lock);	/* probably unneeded */
-		list_for_each_entry(child, &dir->d_subdirs, d_child) {
-			if (d_really_is_positive(child))	/* probably unneeded */
-				d_inode(child)->i_private = NULL;
-		}
-		spin_unlock(&dir->d_lock);
 
+	if (dir)
 		tracefs_remove_recursive(dir);
-	}
 
 	list_del(&file->list);
 	remove_subsystem(file->system);

@@ -1033,7 +1045,7 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
 		flags = file->flags;
 	mutex_unlock(&event_mutex);
 
-	if (!file)
+	if (!file || flags & EVENT_FILE_FL_FREED)
 		return -ENODEV;
 
 	if (flags & EVENT_FILE_FL_ENABLED &&

@@ -1071,7 +1083,7 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 		ret = -ENODEV;
 		mutex_lock(&event_mutex);
 		file = event_file_data(filp);
-		if (likely(file))
+		if (likely(file && !(file->flags & EVENT_FILE_FL_FREED)))
 			ret = ftrace_event_enable_disable(file, val);
 		mutex_unlock(&event_mutex);
 		break;

@@ -1340,7 +1352,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 
 	mutex_lock(&event_mutex);
 	file = event_file_data(filp);
-	if (file)
+	if (file && !(file->flags & EVENT_FILE_FL_FREED))
 		print_event_filter(file, s);
 	mutex_unlock(&event_mutex);

@@ -2264,6 +2276,7 @@ trace_create_new_event(struct trace_event_call *call,
 	atomic_set(&file->tm_ref, 0);
 	INIT_LIST_HEAD(&file->triggers);
 	list_add(&file->list, &tr->events);
+	event_file_get(file);
 
 	return file;
 }

kernel/trace/trace_events_filter.c +3 -0

index bf44f6bbd0c3..bad8cf24837e 100644
@@ -1800,6 +1800,9 @@ int apply_event_filter(struct trace_event_file *file, char *filter_string)
 	struct event_filter *filter = NULL;
 	int err;
 
+	if (file->flags & EVENT_FILE_FL_FREED)
+		return -ENODEV;
+
 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_disable(file);
 		filter = event_filter(file);

Signature · 1 line

2.42.0

Greg KH

author maintainer

2023-11-24 16:06 UTC #7 lore raw

Quoted 5 lines · Steven Rostedt

From: "Steven Rostedt (Google)" <[email protected]>
Date: Tue, 31 Oct 2023 12:24:53 -0400
Subject: [PATCH] tracing: Have trace_event_file have ref counters
commit bb32500fb9b78215e4ef6ee8b4345c5f5d7eafb4 upstream.

All now queued up, thanks for the backports.

greg k-h