On Tue, 30 Apr 2024 14:23:27 -0400
Steven Rostedt <rost...@goodmis.org> wrote:
> From: "Steven Rostedt (Google)" <rost...@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) <mhira...@kernel.org>
Thank you,
> Link:
> https://lore.kernel.org/linux-trace-kernel/20240426073410.17154-1-tze-nan...@mediatek.com/
>
> Cc: sta...@vger.kernel.org
> Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use
> eventfs_inode")
> Reported-by: Tze-nan wu <tze-nan...@mediatek.com>
> Signed-off-by: Steven Rostedt (Google) <rost...@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) <mhira...@kernel.org>
