| Message ID | 20240430142327.7bff81ba@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | eventfs/tracing: Add callback for release of an eventfs_inode | expand |
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 >
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
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; }