[PATCH 0/6] tracing: Fix removing instances while reading/writing to their files

[PATCH 0/6] tracing: Fix removing instances while reading/writing to their files

[Steven Rostedt]

It appears that the dynamic code of eventfs has caused a race window
to open up a bit more and showed that several files were not protected
by the trace array ref count. This means that a task could open one
of the files in an instance, remove the instance, and still be able to
read or write to that file. That access will then cause a "use-after-free"
bug.

Close those holes!

Also, fix a left over unused variable in the eventfs dput fix.

Steven Rostedt (Google) (6):
      tracefs/eventfs: Use dput to free the toplevel events directory
      tracing: Increase trace array ref count on enable and filter files
      tracing: Have tracing_max_latency inc the trace array ref count
      tracing: Have current_trace inc the trace array ref count
      tracing: Have option files inc the trace array ref count
      tracing: Have event inject files inc the trace array ref count

----
 fs/tracefs/event_inode.c           | 17 +++++++---
 fs/tracefs/inode.c                 |  2 +-
 fs/tracefs/internal.h              |  5 +--
 kernel/trace/trace.c               | 68 ++++++++++++++++++++++++++++++++++----
 kernel/trace/trace.h               |  2 ++
 kernel/trace/trace_events.c        |  6 ++--
 kernel/trace/trace_events_inject.c |  3 +-
 7 files changed, 85 insertions(+), 18 deletions(-)

[PATCH 1/6] tracefs/eventfs: Use dput to free the toplevel events directory

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Currently when rmdir on an instance is done, eventfs_remove_events_dir()
is called and it does a dput on the dentry and then frees the
eventfs_inode that represents the events directory.

But there's no protection against a reader reading the top level events
directory at the same time and we can get a use after free error. Instead,
use the dput() associated to the dentry to also free the eventfs_inode
associated to the events directory, as that will get called when the last
reference to the directory is released.

Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/

Cc: Ajay Kaher <akaher@vmware.com>
Fixes: 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")
Reported-by: Zheng Yejian <zhengyejian1@huawei.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/20230905183332.628d7cc0@gandalf.local.home
 - Removed left over "ei" variable (kernel test robot)

 fs/tracefs/event_inode.c | 17 ++++++++++++-----
 fs/tracefs/inode.c       |  2 +-
 fs/tracefs/internal.h    |  5 +++--
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index fa1a1679a886..609ccb5b7cfc 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
 
 /**
  * eventfs_set_ef_status_free - set the ef->status to free
+ * @ti: the tracefs_inode of the dentry
  * @dentry: dentry who's status to be freed
  *
  * eventfs_set_ef_status_free will be called if no more
  * references remain
  */
-void eventfs_set_ef_status_free(struct dentry *dentry)
+void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
 {
 	struct tracefs_inode *ti_parent;
+	struct eventfs_inode *ei;
 	struct eventfs_file *ef;
 
 	mutex_lock(&eventfs_mutex);
+
+	/* The top level events directory may be freed by this */
+	if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
+		ei = ti->private;
+		kfree(ei);
+		goto out;
+	}
+
 	ti_parent = get_tracefs(dentry->d_parent->d_inode);
 	if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
 		goto out;
@@ -510,7 +520,7 @@ struct dentry *eventfs_create_events_dir(const char *name,
 	INIT_LIST_HEAD(&ei->e_top_files);
 
 	ti = get_tracefs(inode);
-	ti->flags |= TRACEFS_EVENT_INODE;
+	ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
 	ti->private = ei;
 
 	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
@@ -806,7 +816,6 @@ void eventfs_remove(struct eventfs_file *ef)
 void eventfs_remove_events_dir(struct dentry *dentry)
 {
 	struct tracefs_inode *ti;
-	struct eventfs_inode *ei;
 
 	if (!dentry || !dentry->d_inode)
 		return;
@@ -815,8 +824,6 @@ void eventfs_remove_events_dir(struct dentry *dentry)
 	if (!ti || !(ti->flags & TRACEFS_EVENT_INODE))
 		return;
 
-	ei = ti->private;
 	d_invalidate(dentry);
 	dput(dentry);
-	kfree(ei);
 }
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 3b8dd938b1c8..891653ba9cf3 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -385,7 +385,7 @@ static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode)
 
 	ti = get_tracefs(inode);
 	if (ti && ti->flags & TRACEFS_EVENT_INODE)
-		eventfs_set_ef_status_free(dentry);
+		eventfs_set_ef_status_free(ti, dentry);
 	iput(inode);
 }
 
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 69c2b1d87c46..4f2e49e2197b 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -3,7 +3,8 @@
 #define _TRACEFS_INTERNAL_H
 
 enum {
-	TRACEFS_EVENT_INODE     = BIT(1),
+	TRACEFS_EVENT_INODE		= BIT(1),
+	TRACEFS_EVENT_TOP_INODE		= BIT(2),
 };
 
 struct tracefs_inode {
@@ -24,6 +25,6 @@ struct inode *tracefs_get_inode(struct super_block *sb);
 struct dentry *eventfs_start_creating(const char *name, struct dentry *parent);
 struct dentry *eventfs_failed_creating(struct dentry *dentry);
 struct dentry *eventfs_end_creating(struct dentry *dentry);
-void eventfs_set_ef_status_free(struct dentry *dentry);
+void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry);
 
 #endif /* _TRACEFS_INTERNAL_H */
-- 
2.40.1

Re: [PATCH 1/6] tracefs/eventfs: Use dput to free the toplevel events directory

[Ajay Kaher]

> On 07-Sep-2023, at 8:17 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> !! External Email
>
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> Currently when rmdir on an instance is done, eventfs_remove_events_dir()
> is called and it does a dput on the dentry and then frees the
> eventfs_inode that represents the events directory.
>
> But there's no protection against a reader reading the top level events
> directory at the same time and we can get a use after free error. Instead,
> use the dput() associated to the dentry to also free the eventfs_inode
> associated to the events directory, as that will get called when the last
> reference to the directory is released.
>

Nice catch Steve. Changes looks good to me.

Would like to know how did you map the backtrace with
use-after-free of eventfs_inode.

Thinking if same problem could happen for sub folder/files of eventfs as
free_ef() may get call earlier then dput().

-Ajay

> Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/
>
> Cc: Ajay Kaher <akaher@vmware.com>
> Fixes: 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")
> Reported-by: Zheng Yejian <zhengyejian1@huawei.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/20230905183332.628d7cc0@gandalf.local.home
> - Removed left over "ei" variable (kernel test robot)
>
> fs/tracefs/event_inode.c | 17 ++++++++++++-----
> fs/tracefs/inode.c       |  2 +-
> fs/tracefs/internal.h    |  5 +++--
> 3 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index fa1a1679a886..609ccb5b7cfc 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
>
> /**
>  * eventfs_set_ef_status_free - set the ef->status to free
> + * @ti: the tracefs_inode of the dentry
>  * @dentry: dentry who's status to be freed
>  *
>  * eventfs_set_ef_status_free will be called if no more
>  * references remain
>  */
> -void eventfs_set_ef_status_free(struct dentry *dentry)
> +void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> {
>        struct tracefs_inode *ti_parent;
> +       struct eventfs_inode *ei;
>        struct eventfs_file *ef;
>
>        mutex_lock(&eventfs_mutex);
> +
> +       /* The top level events directory may be freed by this */
> +       if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
> +               ei = ti->private;
> +               kfree(ei);
> +               goto out;
> +       }
> +
>        ti_parent = get_tracefs(dentry->d_parent->d_inode);
>        if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
>                goto out;
> @@ -510,7 +520,7 @@ struct dentry *eventfs_create_events_dir(const char *name,
>        INIT_LIST_HEAD(&ei->e_top_files);
>
>        ti = get_tracefs(inode);
> -       ti->flags |= TRACEFS_EVENT_INODE;
> +       ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
>        ti->private = ei;
>
>        inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
> @@ -806,7 +816,6 @@ void eventfs_remove(struct eventfs_file *ef)
> void eventfs_remove_events_dir(struct dentry *dentry)
> {
>        struct tracefs_inode *ti;
> -       struct eventfs_inode *ei;
>
>        if (!dentry || !dentry->d_inode)
>                return;
> @@ -815,8 +824,6 @@ void eventfs_remove_events_dir(struct dentry *dentry)
>        if (!ti || !(ti->flags & TRACEFS_EVENT_INODE))
>                return;
>
> -       ei = ti->private;
>        d_invalidate(dentry);
>        dput(dentry);
> -       kfree(ei);
> }
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index 3b8dd938b1c8..891653ba9cf3 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -385,7 +385,7 @@ static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode)
>
>        ti = get_tracefs(inode);
>        if (ti && ti->flags & TRACEFS_EVENT_INODE)
> -               eventfs_set_ef_status_free(dentry);
> +               eventfs_set_ef_status_free(ti, dentry);
>        iput(inode);
> }
>
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index 69c2b1d87c46..4f2e49e2197b 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -3,7 +3,8 @@
> #define _TRACEFS_INTERNAL_H
>
> enum {
> -       TRACEFS_EVENT_INODE     = BIT(1),
> +       TRACEFS_EVENT_INODE             = BIT(1),
> +       TRACEFS_EVENT_TOP_INODE         = BIT(2),
> };
>
> struct tracefs_inode {
> @@ -24,6 +25,6 @@ struct inode *tracefs_get_inode(struct super_block *sb);
> struct dentry *eventfs_start_creating(const char *name, struct dentry *parent);
> struct dentry *eventfs_failed_creating(struct dentry *dentry);
> struct dentry *eventfs_end_creating(struct dentry *dentry);
> -void eventfs_set_ef_status_free(struct dentry *dentry);
> +void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry);
>
> #endif /* _TRACEFS_INTERNAL_H */
> --
> 2.40.1
>
> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.

Re: [PATCH 1/6] tracefs/eventfs: Use dput to free the toplevel events directory

[Steven Rostedt]

On Thu, 7 Sep 2023 17:48:21 +0000
Ajay Kaher <akaher@vmware.com> wrote:

> Nice catch Steve. Changes looks good to me.

I'll add your Reviewed-by then?

> 
> Would like to know how did you map the backtrace with
> use-after-free of eventfs_inode.
> 
> Thinking if same problem could happen for sub folder/files of eventfs as
> free_ef() may get call earlier then dput().

I can also post the output. Here's the full dump:

 [  175.090834] ==================================================================
 [  175.092850] BUG: KASAN: slab-use-after-free in eventfs_root_lookup+0x88/0x1b0
 [  175.094635] Read of size 8 at addr ffff888120130ca0 by task ftracetest/1201
 [  175.096375] 
 [  175.096775] CPU: 4 PID: 1201 Comm: ftracetest Not tainted 6.5.0-test-10737-g469e0a8194e7 #13
 [  175.098727] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
 [  175.100877] Call Trace:
 [  175.101452]  <TASK>
 [  175.101955]  dump_stack_lvl+0x57/0x90
 [  175.102798]  print_report+0xcf/0x670
 [  175.103617]  ? __pfx_ring_buffer_record_off+0x10/0x10
 [  175.104752]  ? _raw_spin_lock_irqsave+0x2b/0x70
 [  175.105724]  ? __virt_addr_valid+0xd9/0x160
 [  175.106626]  kasan_report+0xd4/0x110
 [  175.107401]  ? eventfs_root_lookup+0x88/0x1b0
 [  175.108357]  ? eventfs_root_lookup+0x88/0x1b0
 [  175.109266]  eventfs_root_lookup+0x88/0x1b0
 [  175.110117]  ? eventfs_root_lookup+0x33/0x1b0
 [  175.111013]  __lookup_slow+0x194/0x2a0
 [  175.111785]  ? __pfx___lookup_slow+0x10/0x10
 [  175.112660]  ? down_read+0x11c/0x330
 [  175.113381]  walk_component+0x166/0x220
 [  175.114131]  link_path_walk.part.0.constprop.0+0x3a3/0x5a0
 [  175.115185]  ? seqcount_lockdep_reader_access+0x82/0x90
 [  175.116199]  ? __pfx_link_path_walk.part.0.constprop.0+0x10/0x10
 [  175.117353]  path_openat+0x143/0x11f0
 [  175.118073]  ? __lock_acquire+0xa1a/0x3220
 [  175.118874]  ? __pfx_path_openat+0x10/0x10
 [  175.119668]  ? __pfx___lock_acquire+0x10/0x10
 [  175.120535]  do_filp_open+0x166/0x290
 [  175.121259]  ? __pfx_do_filp_open+0x10/0x10
 [  175.122070]  ? lock_is_held_type+0xce/0x120
 [  175.122899]  ? preempt_count_sub+0xb7/0x100
 [  175.123714]  ? _raw_spin_unlock+0x29/0x50
 [  175.124504]  ? alloc_fd+0x1a0/0x320
 [  175.125202]  do_sys_openat2+0x126/0x160
 [  175.125955]  ? rcu_is_watching+0x34/0x60
 [  175.126728]  ? __pfx_do_sys_openat2+0x10/0x10
 [  175.127579]  ? __might_resched+0x2cf/0x3b0
 [  175.128391]  ? __fget_light+0xdf/0x100
 [  175.129138]  __x64_sys_openat+0xcd/0x140
 [  175.129906]  ? __pfx___x64_sys_openat+0x10/0x10
 [  175.133418]  ? syscall_enter_from_user_mode+0x22/0x90
 [  175.136987]  ? lockdep_hardirqs_on+0x7d/0x100
 [  175.140421]  do_syscall_64+0x3b/0xc0
 [  175.143605]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
 [  175.147005] RIP: 0033:0x7f1dceef5e51
 [  175.150173] Code: 75 57 89 f0 25 00 00 41 00 3d 00 00 41 00 74 49 80 3d 9a 27 0e 00 00 74 6d 89 da 48 89 ee bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 93 00 00 00 48 8b 54 24 28 64 48 2b 14 25
 [  175.158706] RSP: 002b:00007fff2cddf380 EFLAGS: 00000202 ORIG_RAX: 0000000000000101
 [  175.162676] RAX: ffffffffffffffda RBX: 0000000000000241 RCX: 00007f1dceef5e51
 [  175.166579] RDX: 0000000000000241 RSI: 000055d7520677d0 RDI: 00000000ffffff9c
 [  175.170442] RBP: 000055d7520677d0 R08: 000000000000001e R09: 0000000000000001
 [  175.174238] R10: 00000000000001b6 R11: 0000000000000202 R12: 0000000000000000
 [  175.177960] R13: 0000000000000003 R14: 000055d752035678 R15: 000055d752067788
 [  175.181738]  </TASK>
 [  175.184665] 
 [  175.187443] Allocated by task 1200:
 [  175.190554]  kasan_save_stack+0x2f/0x50
 [  175.193712]  kasan_set_track+0x21/0x30
 [  175.196836]  __kasan_kmalloc+0x8b/0x90
 [  175.199939]  eventfs_create_events_dir+0x54/0x220
 [  175.203234]  create_event_toplevel_files+0x42/0x130
 [  175.206609]  event_trace_add_tracer+0x33/0x180
 [  175.209934]  trace_array_create_dir+0x52/0xf0
 [  175.213222]  trace_array_create+0x361/0x410
 [  175.216461]  instance_mkdir+0x6b/0xb0
 [  175.219592]  tracefs_syscall_mkdir+0x57/0x80
 [  175.222852]  vfs_mkdir+0x275/0x380
 [  175.225949]  do_mkdirat+0x1da/0x210
 [  175.229048]  __x64_sys_mkdir+0x74/0xa0
 [  175.232161]  do_syscall_64+0x3b/0xc0
 [  175.235211]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
 [  175.238519] 
 [  175.241210] Freed by task 1251:
 [  175.244190]  kasan_save_stack+0x2f/0x50
 [  175.247293]  kasan_set_track+0x21/0x30
 [  175.250375]  kasan_save_free_info+0x27/0x40
 [  175.253517]  __kasan_slab_free+0x106/0x180
 [  175.256637]  __kmem_cache_free+0x149/0x2e0
 [  175.259749]  event_trace_del_tracer+0xcb/0x120
 [  175.262940]  __remove_instance+0x16a/0x340
 [  175.266080]  instance_rmdir+0x77/0xa0
 [  175.269139]  tracefs_syscall_rmdir+0x77/0xc0
 [  175.272323]  vfs_rmdir+0xed/0x2d0
 [  175.275321]  do_rmdir+0x235/0x280
 [  175.278327]  __x64_sys_rmdir+0x5f/0x90
 [  175.281395]  do_syscall_64+0x3b/0xc0
 [  175.284433]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
 [  175.287725] 
 [  175.290402] The buggy address belongs to the object at ffff888120130ca0
 [  175.290402]  which belongs to the cache kmalloc-16 of size 16
 [  175.297419] The buggy address is located 0 bytes inside of
 [  175.297419]  freed 16-byte region [ffff888120130ca0, ffff888120130cb0)
 [  175.304477] 
 [  175.307155] The buggy address belongs to the physical page:
 [  175.310514] page:000000004dbddbb0 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x120130
 [  175.314581] flags: 0x17ffffc0000800(slab|node=0|zone=2|lastcpupid=0x1fffff)
 [  175.318254] page_type: 0xffffffff()
 [  175.321321] raw: 0017ffffc0000800 ffff8881000423c0 dead000000000122 0000000000000000
 [  175.325190] raw: 0000000000000000 0000000000800080 00000001ffffffff 0000000000000000
 [  175.329080] page dumped because: kasan: bad access detected
 [  175.332608] 
 [  175.335350] Memory state around the buggy address:
 [  175.338752]  ffff888120130b80: 00 00 fc fc 00 05 fc fc 00 00 fc fc 00 02 fc fc
 [  175.342664]  ffff888120130c00: 00 07 fc fc 00 00 fc fc 00 00 fc fc fa fb fc fc
 [  175.346537] >ffff888120130c80: 00 00 fc fc fa fb fc fc 00 00 fc fc 00 00 fc fc
 [  175.350391]                                ^
 [  175.353753]  ffff888120130d00: 00 00 fc fc 00 00 fc fc 00 00 fc fc fa fb fc fc
 [  175.357707]  ffff888120130d80: 00 00 fc fc 00 00 fc fc 00 00 fc fc 00 00 fc fc
 [  175.361621] ==================================================================


In the "Freed by task" above, the event_trace_del_tracer+0xcb is the tail
call from the kfree(ei) in eventfs_remove_events_dir(). It's a tail call
so that screws up the unwind in ORC, as that address returned the next line
after the call to eventfs_remove_events_dir().

I can add this to the commit change log. And also I'll update the change
log of the other patch that shows its KASAN dump.

-- Steve

Re: [PATCH 1/6] tracefs/eventfs: Use dput to free the toplevel events directory

[Steven Rostedt]

On Thu, 7 Sep 2023 17:48:21 +0000
Ajay Kaher <akaher@vmware.com> wrote:

> Thinking if same problem could happen for sub folder/files of eventfs as
> free_ef() may get call earlier then dput().

Actually, they are not freed at all! Another patch coming.

-- Steve

Re: [PATCH 1/6] tracefs/eventfs: Use dput to free the toplevel events directory

[Masami Hiramatsu]

Hi, 

On Wed, 06 Sep 2023 22:47:11 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Currently when rmdir on an instance is done, eventfs_remove_events_dir()
> is called and it does a dput on the dentry and then frees the
> eventfs_inode that represents the events directory.
> 
> But there's no protection against a reader reading the top level events
> directory at the same time and we can get a use after free error. Instead,
> use the dput() associated to the dentry to also free the eventfs_inode
> associated to the events directory, as that will get called when the last
> reference to the directory is released.
> 
> Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/
> 
> Cc: Ajay Kaher <akaher@vmware.com>
> Fixes: 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")
> Reported-by: Zheng Yejian <zhengyejian1@huawei.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v1: https://lore.kernel.org/linux-trace-kernel/20230905183332.628d7cc0@gandalf.local.home
>  - Removed left over "ei" variable (kernel test robot)
> 
>  fs/tracefs/event_inode.c | 17 ++++++++++++-----
>  fs/tracefs/inode.c       |  2 +-
>  fs/tracefs/internal.h    |  5 +++--
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index fa1a1679a886..609ccb5b7cfc 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
>  
>  /**
>   * eventfs_set_ef_status_free - set the ef->status to free
> + * @ti: the tracefs_inode of the dentry
>   * @dentry: dentry who's status to be freed
>   *
>   * eventfs_set_ef_status_free will be called if no more
>   * references remain
>   */
> -void eventfs_set_ef_status_free(struct dentry *dentry)
> +void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
>  {
>  	struct tracefs_inode *ti_parent;
> +	struct eventfs_inode *ei;
>  	struct eventfs_file *ef;
>  
>  	mutex_lock(&eventfs_mutex);
> +
> +	/* The top level events directory may be freed by this */
> +	if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
> +		ei = ti->private;
> +		kfree(ei);

Don't we need to clear 'ti->private' here to avoid accessing
(or double free) ti->private somewhare?

> +		goto out;
> +	}
> +
>  	ti_parent = get_tracefs(dentry->d_parent->d_inode);
>  	if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
>  		goto out;

Here, I guess this "!(ti_parent->flags & TRACEFS_EVENT_INODE)" means this
inode is TRACEFS_EVENT_TOP_INODE, so this check may not be needed,
is this correct?

Thank you,

> @@ -510,7 +520,7 @@ struct dentry *eventfs_create_events_dir(const char *name,
>  	INIT_LIST_HEAD(&ei->e_top_files);
>  
>  	ti = get_tracefs(inode);
> -	ti->flags |= TRACEFS_EVENT_INODE;
> +	ti->flags |= TRACEFS_EVENT_INODE | TRACEFS_EVENT_TOP_INODE;
>  	ti->private = ei;
>  
>  	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
> @@ -806,7 +816,6 @@ void eventfs_remove(struct eventfs_file *ef)
>  void eventfs_remove_events_dir(struct dentry *dentry)
>  {
>  	struct tracefs_inode *ti;
> -	struct eventfs_inode *ei;
>  
>  	if (!dentry || !dentry->d_inode)
>  		return;
> @@ -815,8 +824,6 @@ void eventfs_remove_events_dir(struct dentry *dentry)
>  	if (!ti || !(ti->flags & TRACEFS_EVENT_INODE))
>  		return;
>  
> -	ei = ti->private;
>  	d_invalidate(dentry);
>  	dput(dentry);
> -	kfree(ei);
>  }
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index 3b8dd938b1c8..891653ba9cf3 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -385,7 +385,7 @@ static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode)
>  
>  	ti = get_tracefs(inode);
>  	if (ti && ti->flags & TRACEFS_EVENT_INODE)
> -		eventfs_set_ef_status_free(dentry);
> +		eventfs_set_ef_status_free(ti, dentry);
>  	iput(inode);
>  }
>  
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index 69c2b1d87c46..4f2e49e2197b 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -3,7 +3,8 @@
>  #define _TRACEFS_INTERNAL_H
>  
>  enum {
> -	TRACEFS_EVENT_INODE     = BIT(1),
> +	TRACEFS_EVENT_INODE		= BIT(1),
> +	TRACEFS_EVENT_TOP_INODE		= BIT(2),
>  };
>  
>  struct tracefs_inode {
> @@ -24,6 +25,6 @@ struct inode *tracefs_get_inode(struct super_block *sb);
>  struct dentry *eventfs_start_creating(const char *name, struct dentry *parent);
>  struct dentry *eventfs_failed_creating(struct dentry *dentry);
>  struct dentry *eventfs_end_creating(struct dentry *dentry);
> -void eventfs_set_ef_status_free(struct dentry *dentry);
> +void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry);
>  
>  #endif /* _TRACEFS_INTERNAL_H */
> -- 
> 2.40.1


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/6] tracefs/eventfs: Use dput to free the toplevel events directory

[Steven Rostedt]

On Fri, 8 Sep 2023 16:45:53 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Hi, 
> 
> On Wed, 06 Sep 2023 22:47:11 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > Currently when rmdir on an instance is done, eventfs_remove_events_dir()
> > is called and it does a dput on the dentry and then frees the
> > eventfs_inode that represents the events directory.
> > 
> > But there's no protection against a reader reading the top level events
> > directory at the same time and we can get a use after free error. Instead,
> > use the dput() associated to the dentry to also free the eventfs_inode
> > associated to the events directory, as that will get called when the last
> > reference to the directory is released.
> > 
> > Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/
> > 
> > Cc: Ajay Kaher <akaher@vmware.com>
> > Fixes: 5bdcd5f5331a2 eventfs: ("Implement removal of meta data from eventfs")
> > Reported-by: Zheng Yejian <zhengyejian1@huawei.com>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> > Changes since v1: https://lore.kernel.org/linux-trace-kernel/20230905183332.628d7cc0@gandalf.local.home
> >  - Removed left over "ei" variable (kernel test robot)
> > 
> >  fs/tracefs/event_inode.c | 17 ++++++++++++-----
> >  fs/tracefs/inode.c       |  2 +-
> >  fs/tracefs/internal.h    |  5 +++--
> >  3 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> > index fa1a1679a886..609ccb5b7cfc 100644
> > --- a/fs/tracefs/event_inode.c
> > +++ b/fs/tracefs/event_inode.c
> > @@ -185,17 +185,27 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
> >  
> >  /**
> >   * eventfs_set_ef_status_free - set the ef->status to free
> > + * @ti: the tracefs_inode of the dentry
> >   * @dentry: dentry who's status to be freed
> >   *
> >   * eventfs_set_ef_status_free will be called if no more
> >   * references remain
> >   */
> > -void eventfs_set_ef_status_free(struct dentry *dentry)
> > +void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> >  {
> >  	struct tracefs_inode *ti_parent;
> > +	struct eventfs_inode *ei;
> >  	struct eventfs_file *ef;
> >  
> >  	mutex_lock(&eventfs_mutex);
> > +
> > +	/* The top level events directory may be freed by this */
> > +	if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
> > +		ei = ti->private;
> > +		kfree(ei);  
> 
> Don't we need to clear 'ti->private' here to avoid accessing
> (or double free) ti->private somewhare?

I don't think it's needed but I did add it to

  https://lore.kernel.org/linux-trace-kernel/20230907175859.6fedbaa2@gandalf.local.home/

Which you reviewed.

> 
> > +		goto out;
> > +	}
> > +
> >  	ti_parent = get_tracefs(dentry->d_parent->d_inode);
> >  	if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
> >  		goto out;  
> 
> Here, I guess this "!(ti_parent->flags & TRACEFS_EVENT_INODE)" means this
> inode is TRACEFS_EVENT_TOP_INODE, so this check may not be needed,
> is this correct?

The check isn't needed but I like to keep it because it will break things
badly if it is every called on something that is not an EVENT_INODE.

We could add a WARN() here if not, but this code is not critical if it is
called without it set.

-- Steve

[PATCH 2/6] tracing: Increase trace array ref count on enable and filter files

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

When the trace event enable and filter files are opened, increment the
trace array ref counter, otherwise they can be accessed when the trace
array is being deleted. The ref counter keeps the trace array from being
deleted while those files are opened.

Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/

Cc: stable@vger.kernel.org
Fixes: 8530dec63e7b4 ("tracing: Add tracing_check_open_get_tr()")
Reported-by: Zheng Yejian <zhengyejian1@huawei.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c        | 27 +++++++++++++++++++++++++++
 kernel/trace/trace.h        |  2 ++
 kernel/trace/trace_events.c |  6 ++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 35783a7baf15..0827037ee3b8 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4973,6 +4973,33 @@ int tracing_open_generic_tr(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+/*
+ * The private pointer of the inode is the trace_event_file.
+ * Update the tr ref count associated to it.
+ */
+int tracing_open_file_tr(struct inode *inode, struct file *filp)
+{
+	struct trace_event_file *file = inode->i_private;
+	int ret;
+
+	ret = tracing_check_open_get_tr(file->tr);
+	if (ret)
+		return ret;
+
+	filp->private_data = inode->i_private;
+
+	return 0;
+}
+
+int tracing_release_file_tr(struct inode *inode, struct file *filp)
+{
+	struct trace_event_file *file = inode->i_private;
+
+	trace_array_put(file->tr);
+
+	return 0;
+}
+
 static int tracing_mark_open(struct inode *inode, struct file *filp)
 {
 	stream_open(inode, filp);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 5669dd1f90d9..77debe53f07c 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -610,6 +610,8 @@ void tracing_reset_all_online_cpus(void);
 void tracing_reset_all_online_cpus_unlocked(void);
 int tracing_open_generic(struct inode *inode, struct file *filp);
 int tracing_open_generic_tr(struct inode *inode, struct file *filp);
+int tracing_open_file_tr(struct inode *inode, struct file *filp);
+int tracing_release_file_tr(struct inode *inode, struct file *filp);
 bool tracing_is_disabled(void);
 bool tracer_tracing_is_on(struct trace_array *tr);
 void tracer_tracing_on(struct trace_array *tr);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index ed367d713be0..2af92177b765 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2103,9 +2103,10 @@ static const struct file_operations ftrace_set_event_notrace_pid_fops = {
 };
 
 static const struct file_operations ftrace_enable_fops = {
-	.open = tracing_open_generic,
+	.open = tracing_open_file_tr,
 	.read = event_enable_read,
 	.write = event_enable_write,
+	.release = tracing_release_file_tr,
 	.llseek = default_llseek,
 };
 
@@ -2122,9 +2123,10 @@ static const struct file_operations ftrace_event_id_fops = {
 };
 
 static const struct file_operations ftrace_event_filter_fops = {
-	.open = tracing_open_generic,
+	.open = tracing_open_file_tr,
 	.read = event_filter_read,
 	.write = event_filter_write,
+	.release = tracing_release_file_tr,
 	.llseek = default_llseek,
 };
 
-- 
2.40.1

[PATCH 3/6] tracing: Have tracing_max_latency inc the trace array ref count

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The tracing_max_latency file points to the trace_array max_latency field.
For an instance, if the file is opened and the instance is deleted,
reading or writing to the file will cause a use after free.

Up the ref count of the trace_array when tracing_max_latency is opened.

Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/

Cc: stable@vger.kernel.org
Fixes: 8530dec63e7b4 ("tracing: Add tracing_check_open_get_tr()")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0827037ee3b8..c8b8b4c6feaf 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1772,7 +1772,7 @@ static void trace_create_maxlat_file(struct trace_array *tr,
 	init_irq_work(&tr->fsnotify_irqwork, latency_fsnotify_workfn_irq);
 	tr->d_max_latency = trace_create_file("tracing_max_latency",
 					      TRACE_MODE_WRITE,
-					      d_tracer, &tr->max_latency,
+					      d_tracer, tr,
 					      &tracing_max_lat_fops);
 }
 
@@ -1805,7 +1805,7 @@ void latency_fsnotify(struct trace_array *tr)
 
 #define trace_create_maxlat_file(tr, d_tracer)				\
 	trace_create_file("tracing_max_latency", TRACE_MODE_WRITE,	\
-			  d_tracer, &tr->max_latency, &tracing_max_lat_fops)
+			  d_tracer, tr, &tracing_max_lat_fops)
 
 #endif
 
@@ -6717,14 +6717,18 @@ static ssize_t
 tracing_max_lat_read(struct file *filp, char __user *ubuf,
 		     size_t cnt, loff_t *ppos)
 {
-	return tracing_nsecs_read(filp->private_data, ubuf, cnt, ppos);
+	struct trace_array *tr = filp->private_data;
+
+	return tracing_nsecs_read(&tr->max_latency, ubuf, cnt, ppos);
 }
 
 static ssize_t
 tracing_max_lat_write(struct file *filp, const char __user *ubuf,
 		      size_t cnt, loff_t *ppos)
 {
-	return tracing_nsecs_write(filp->private_data, ubuf, cnt, ppos);
+	struct trace_array *tr = filp->private_data;
+
+	return tracing_nsecs_write(&tr->max_latency, ubuf, cnt, ppos);
 }
 
 #endif
@@ -7778,10 +7782,11 @@ static const struct file_operations tracing_thresh_fops = {
 
 #ifdef CONFIG_TRACER_MAX_TRACE
 static const struct file_operations tracing_max_lat_fops = {
-	.open		= tracing_open_generic,
+	.open		= tracing_open_generic_tr,
 	.read		= tracing_max_lat_read,
 	.write		= tracing_max_lat_write,
 	.llseek		= generic_file_llseek,
+	.release	= tracing_release_generic_tr,
 };
 #endif
 
-- 
2.40.1

[PATCH 4/6] tracing: Have current_trace inc the trace array ref count

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The current_trace updates the trace array tracer. For an instance, if the
file is opened and the instance is deleted, reading or writing to the file
will cause a use after free.

Up the ref count of the trace array when current_trace is opened.

Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/

Cc: stable@vger.kernel.org
Fixes: 8530dec63e7b4 ("tracing: Add tracing_check_open_get_tr()")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c8b8b4c6feaf..b82df33d20ff 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -7791,10 +7791,11 @@ static const struct file_operations tracing_max_lat_fops = {
 #endif
 
 static const struct file_operations set_tracer_fops = {
-	.open		= tracing_open_generic,
+	.open		= tracing_open_generic_tr,
 	.read		= tracing_set_trace_read,
 	.write		= tracing_set_trace_write,
 	.llseek		= generic_file_llseek,
+	.release	= tracing_release_generic_tr,
 };
 
 static const struct file_operations tracing_pipe_fops = {
-- 
2.40.1

[PATCH 5/6] tracing: Have option files inc the trace array ref count

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The option files update the options for a given trace array. For an
instance, if the file is opened and the instance is deleted, reading or
writing to the file will cause a use after free.

Up the ref count of the trace_array when an option file is opened.

Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/

Cc: stable@vger.kernel.org
Fixes: 8530dec63e7b4 ("tracing: Add tracing_check_open_get_tr()")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b82df33d20ff..0608ad20cf30 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8988,12 +8988,33 @@ trace_options_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	return cnt;
 }
 
+static int tracing_open_options(struct inode *inode, struct file *filp)
+{
+	struct trace_option_dentry *topt = inode->i_private;
+	int ret;
+
+	ret = tracing_check_open_get_tr(topt->tr);
+	if (ret)
+		return ret;
+
+	filp->private_data = inode->i_private;
+	return 0;
+}
+
+static int tracing_release_options(struct inode *inode, struct file *file)
+{
+	struct trace_option_dentry *topt = file->private_data;
+
+	trace_array_put(topt->tr);
+	return 0;
+}
 
 static const struct file_operations trace_options_fops = {
-	.open = tracing_open_generic,
+	.open = tracing_open_options,
 	.read = trace_options_read,
 	.write = trace_options_write,
 	.llseek	= generic_file_llseek,
+	.release = tracing_release_options,
 };
 
 /*
-- 
2.40.1

[PATCH 6/6] tracing: Have event inject files inc the trace array ref count

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The event inject files add events for a specific trace array. For an
instance, if the file is opened and the instance is deleted, reading or
writing to the file will cause a use after free.

Up the ref count of the trace_array when a event inject file is opened.

Link: https://lore.kernel.org/all/1cb3aee2-19af-c472-e265-05176fe9bd84@huawei.com/

Cc: stable@vger.kernel.org
Fixes: 6c3edaf9fd6a ("tracing: Introduce trace event injection")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_inject.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_inject.c b/kernel/trace/trace_events_inject.c
index abe805d471eb..8650562bdaa9 100644
--- a/kernel/trace/trace_events_inject.c
+++ b/kernel/trace/trace_events_inject.c
@@ -328,7 +328,8 @@ event_inject_read(struct file *file, char __user *buf, size_t size,
 }
 
 const struct file_operations event_inject_fops = {
-	.open = tracing_open_generic,
+	.open = tracing_open_file_tr,
 	.read = event_inject_read,
 	.write = event_inject_write,
+	.release = tracing_release_file_tr,
 };
-- 
2.40.1

Re: [PATCH 0/6] tracing: Fix removing instances while reading/writing to their files

[Naresh Kamboju]

On Thu, 7 Sept 2023 at 08:17, Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> It appears that the dynamic code of eventfs has caused a race window
> to open up a bit more and showed that several files were not protected
> by the trace array ref count. This means that a task could open one
> of the files in an instance, remove the instance, and still be able to
> read or write to that file. That access will then cause a "use-after-free"
> bug.
>
> Close those holes!
>
> Also, fix a left over unused variable in the eventfs dput fix.

This patch set applied on top of Linux next master branch and
tested selftests ftrace tests [1]. The test run to complete and not
found any crashes.

Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

>
> Steven Rostedt (Google) (6):
>       tracefs/eventfs: Use dput to free the toplevel events directory
>       tracing: Increase trace array ref count on enable and filter files
>       tracing: Have tracing_max_latency inc the trace array ref count
>       tracing: Have current_trace inc the trace array ref count
>       tracing: Have option files inc the trace array ref count
>       tracing: Have event inject files inc the trace array ref count
>
> ----
>  fs/tracefs/event_inode.c           | 17 +++++++---
>  fs/tracefs/inode.c                 |  2 +-
>  fs/tracefs/internal.h              |  5 +--
>  kernel/trace/trace.c               | 68 ++++++++++++++++++++++++++++++++++----
>  kernel/trace/trace.h               |  2 ++
>  kernel/trace/trace_events.c        |  6 ++--
>  kernel/trace/trace_events_inject.c |  3 +-
>  7 files changed, 85 insertions(+), 18 deletions(-)

[1] https://tuxapi.tuxsuite.com/v1/groups/linaro/projects/naresh/tests/2V4IYQhAHrFXjwQvHLc2w3mKhXs

--
Linaro LKFT
https://lkft.linaro.org

Re: [PATCH 0/6] tracing: Fix removing instances while reading/writing to their files

[Steven Rostedt]

On Thu, 7 Sep 2023 18:54:46 +0530
Naresh Kamboju <naresh.kamboju@linaro.org> wrote:

> This patch set applied on top of Linux next master branch and
> tested selftests ftrace tests [1]. The test run to complete and not
> found any crashes.
> 
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

Thanks Naresh!

-- Steve