* [PATCH] eventfs/tracing: Add callback for release of an eventfs_inode
@ 2024-04-30 18:23 Steven Rostedt
2024-05-01 14:56 ` Masami Hiramatsu
0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2024-04-30 18:23 UTC (permalink / raw
To: LKML, Linux Trace Kernel; +Cc: Masami Hiramatsu, Mathieu Desnoyers, Tze-nan wu
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
Synthetic events create and destroy tracefs files when they are created
and removed. The tracing subsystem has its own file descriptor
representing the state of the events attached to the tracefs files.
There's a race between the eventfs files and this file descriptor of the
tracing system where the following can cause an issue:
With two scripts 'A' and 'B' doing:
Script 'A':
echo "hello int aaa" > /sys/kernel/tracing/synthetic_events
while :
do
echo 0 > /sys/kernel/tracing/events/synthetic/hello/enable
done
Script 'B':
echo > /sys/kernel/tracing/synthetic_events
Script 'A' creates a synthetic event "hello" and then just writes zero
into its enable file.
Script 'B' removes all synthetic events (including the newly created
"hello" event).
What happens is that the opening of the "enable" file has:
{
struct trace_event_file *file = inode->i_private;
int ret;
ret = tracing_check_open_get_tr(file->tr);
[..]
But deleting the events frees the "file" descriptor, and a "use after
free" happens with the dereference at "file->tr".
The file descriptor does have a reference counter, but there needs to be a
way to decrement it from the eventfs when the eventfs_inode is removed
that represents this file descriptor.
Add an optional "release" callback to the eventfs_entry array structure,
that gets called when the eventfs file is about to be removed. This allows
for the creating on the eventfs file to increment the tracing file
descriptor ref counter. When the eventfs file is deleted, it can call the
release function that will call the put function for the tracing file
descriptor.
This will protect the tracing file from being freed while a eventfs file
that references it is being opened.
Link: https://lore.kernel.org/linux-trace-kernel/20240426073410.17154-1-Tze-nan.Wu@mediatek.com/
Cc: stable@vger.kernel.org
Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Reported-by: Tze-nan wu <Tze-nan.Wu@mediatek.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
fs/tracefs/event_inode.c | 7 +++++++
include/linux/tracefs.h | 3 +++
kernel/trace/trace_events.c | 12 ++++++++++++
3 files changed, 22 insertions(+)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 894c6ca1e500..dc97c19f9e0a 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -84,10 +84,17 @@ enum {
static void release_ei(struct kref *ref)
{
struct eventfs_inode *ei = container_of(ref, struct eventfs_inode, kref);
+ const struct eventfs_entry *entry;
struct eventfs_root_inode *rei;
WARN_ON_ONCE(!ei->is_freed);
+ for (int i = 0; i < ei->nr_entries; i++) {
+ entry = &ei->entries[i];
+ if (entry->release)
+ entry->release(entry->name, ei->data);
+ }
+
kfree(ei->entry_attrs);
kfree_const(ei->name);
if (ei->is_events) {
diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
index 7a5fe17b6bf9..d03f74658716 100644
--- a/include/linux/tracefs.h
+++ b/include/linux/tracefs.h
@@ -62,6 +62,8 @@ struct eventfs_file;
typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
const struct file_operations **fops);
+typedef void (*eventfs_release)(const char *name, void *data);
+
/**
* struct eventfs_entry - dynamically created eventfs file call back handler
* @name: Then name of the dynamic file in an eventfs directory
@@ -72,6 +74,7 @@ typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
struct eventfs_entry {
const char *name;
eventfs_callback callback;
+ eventfs_release release;
};
struct eventfs_inode;
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 52f75c36bbca..6ef29eba90ce 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2552,6 +2552,14 @@ static int event_callback(const char *name, umode_t *mode, void **data,
return 0;
}
+/* The file is incremented on creation and freeing the enable file decrements it */
+static void event_release(const char *name, void *data)
+{
+ struct trace_event_file *file = data;
+
+ event_file_put(file);
+}
+
static int
event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
{
@@ -2566,6 +2574,7 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
{
.name = "enable",
.callback = event_callback,
+ .release = event_release,
},
{
.name = "filter",
@@ -2634,6 +2643,9 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
return ret;
}
+ /* Gets decremented on freeing of the "enable" file */
+ event_file_get(file);
+
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] eventfs/tracing: Add callback for release of an eventfs_inode
2024-04-30 18:23 [PATCH] eventfs/tracing: Add callback for release of an eventfs_inode Steven Rostedt
@ 2024-05-01 14:56 ` Masami Hiramatsu
2024-05-02 13:04 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Masami Hiramatsu @ 2024-05-01 14:56 UTC (permalink / raw
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mathieu Desnoyers,
Tze-nan wu
On Tue, 30 Apr 2024 14:23:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> Synthetic events create and destroy tracefs files when they are created
> and removed. The tracing subsystem has its own file descriptor
> representing the state of the events attached to the tracefs files.
> There's a race between the eventfs files and this file descriptor of the
> tracing system where the following can cause an issue:
>
> With two scripts 'A' and 'B' doing:
>
> Script 'A':
> echo "hello int aaa" > /sys/kernel/tracing/synthetic_events
> while :
> do
> echo 0 > /sys/kernel/tracing/events/synthetic/hello/enable
> done
>
> Script 'B':
> echo > /sys/kernel/tracing/synthetic_events
>
> Script 'A' creates a synthetic event "hello" and then just writes zero
> into its enable file.
>
> Script 'B' removes all synthetic events (including the newly created
> "hello" event).
>
> What happens is that the opening of the "enable" file has:
>
> {
> struct trace_event_file *file = inode->i_private;
> int ret;
>
> ret = tracing_check_open_get_tr(file->tr);
> [..]
>
> But deleting the events frees the "file" descriptor, and a "use after
> free" happens with the dereference at "file->tr".
>
> The file descriptor does have a reference counter, but there needs to be a
> way to decrement it from the eventfs when the eventfs_inode is removed
> that represents this file descriptor.
>
> Add an optional "release" callback to the eventfs_entry array structure,
> that gets called when the eventfs file is about to be removed. This allows
> for the creating on the eventfs file to increment the tracing file
> descriptor ref counter. When the eventfs file is deleted, it can call the
> release function that will call the put function for the tracing file
> descriptor.
>
> This will protect the tracing file from being freed while a eventfs file
> that references it is being opened.
>
Looks good to me.
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thank you,
> Link: https://lore.kernel.org/linux-trace-kernel/20240426073410.17154-1-Tze-nan.Wu@mediatek.com/
>
> Cc: stable@vger.kernel.org
> Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
> Reported-by: Tze-nan wu <Tze-nan.Wu@mediatek.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> fs/tracefs/event_inode.c | 7 +++++++
> include/linux/tracefs.h | 3 +++
> kernel/trace/trace_events.c | 12 ++++++++++++
> 3 files changed, 22 insertions(+)
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 894c6ca1e500..dc97c19f9e0a 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -84,10 +84,17 @@ enum {
> static void release_ei(struct kref *ref)
> {
> struct eventfs_inode *ei = container_of(ref, struct eventfs_inode, kref);
> + const struct eventfs_entry *entry;
> struct eventfs_root_inode *rei;
>
> WARN_ON_ONCE(!ei->is_freed);
>
> + for (int i = 0; i < ei->nr_entries; i++) {
> + entry = &ei->entries[i];
> + if (entry->release)
> + entry->release(entry->name, ei->data);
> + }
> +
> kfree(ei->entry_attrs);
> kfree_const(ei->name);
> if (ei->is_events) {
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index 7a5fe17b6bf9..d03f74658716 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -62,6 +62,8 @@ struct eventfs_file;
> typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
> const struct file_operations **fops);
>
> +typedef void (*eventfs_release)(const char *name, void *data);
> +
> /**
> * struct eventfs_entry - dynamically created eventfs file call back handler
> * @name: Then name of the dynamic file in an eventfs directory
> @@ -72,6 +74,7 @@ typedef int (*eventfs_callback)(const char *name, umode_t *mode, void **data,
> struct eventfs_entry {
> const char *name;
> eventfs_callback callback;
> + eventfs_release release;
> };
>
> struct eventfs_inode;
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 52f75c36bbca..6ef29eba90ce 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2552,6 +2552,14 @@ static int event_callback(const char *name, umode_t *mode, void **data,
> return 0;
> }
>
> +/* The file is incremented on creation and freeing the enable file decrements it */
> +static void event_release(const char *name, void *data)
> +{
> + struct trace_event_file *file = data;
> +
> + event_file_put(file);
> +}
> +
> static int
> event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
> {
> @@ -2566,6 +2574,7 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
> {
> .name = "enable",
> .callback = event_callback,
> + .release = event_release,
> },
> {
> .name = "filter",
> @@ -2634,6 +2643,9 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
> return ret;
> }
>
> + /* Gets decremented on freeing of the "enable" file */
> + event_file_get(file);
> +
> return 0;
> }
>
> --
> 2.43.0
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] eventfs/tracing: Add callback for release of an eventfs_inode
2024-05-01 14:56 ` Masami Hiramatsu
@ 2024-05-02 13:04 ` Steven Rostedt
0 siblings, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2024-05-02 13:04 UTC (permalink / raw
To: Masami Hiramatsu (Google)
Cc: LKML, Linux Trace Kernel, Mathieu Desnoyers, Tze-nan wu
On Wed, 1 May 2024 23:56:26 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> Looks good to me.
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thanks Masami,
Although Tze-nan pointed out a issue with this patch.
I just published v2, can you review that one too?
Thanks,
-- Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-02 13:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30 18:23 [PATCH] eventfs/tracing: Add callback for release of an eventfs_inode Steven Rostedt
2024-05-01 14:56 ` Masami Hiramatsu
2024-05-02 13:04 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).