[PATCH v2] eventfs: Use children field for rcu head and add memory barriers

[PATCH v2] eventfs: Use children field for rcu head and add memory barriers

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

When an eventfs inode is freed, it sets ei->is_freed and then uses its
ei->list to add it to the srcu link list as the list field is a union with
the rcu list head. As the ei->list is used to iterate over an SRCU
protected list without taking the eventfs_mutex, there's nothing stopping
the iteration over that list to see the ei->rcu instead of the ei->list
and it will read a corrupt target.

To fix this, change the union of the rcu list head with the children list.
On freeing the eventfs inode, set the is_free and execute a smp_wmb()
before adding the eventfs inode to the SRCU list.

On iteration of the ei->children list, at the start, execute a smp_rmb()
and then read the is_freed of the ei to see if the children list is still
valid. If is_freed is set, then the ei_child read is not valid and the
loop should exit immediately.

Cc: stable@vger.kernel.org
Fixes: 704f960dbee2f ("eventfs: Read ei->entries before ei->children in eventfs_iterate()")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260806022719.375354-1-shuangpeng.kernel%40gmail.com
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
Changes since v1: https://patch.msgid.link/20260807170408.2d324df5@gandalf.local.home

- Fixed placement of smp_wmb() and is_free (Reported by Sashiko)

 fs/tracefs/event_inode.c | 24 ++++++++++++++++++++++++
 fs/tracefs/internal.h    |  4 ++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 39c7a34531e8..7d2431f2bb08 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -124,7 +124,17 @@ static inline void put_ei(struct eventfs_inode *ei)
 static inline void free_ei(struct eventfs_inode *ei)
 {
 	if (ei) {
+		/* The ei should have no children if it is being freed. */
+		WARN_ON_ONCE(!list_empty(&ei->children));
 		ei->is_freed = 1;
+		/*
+		 * The SRCU iteration has a smp_rmb() to make sure it
+		 * sees a child (that may have already been freed)
+		 * before it reads is_free. If is_free is set, it must
+		 * not use the child it acquired from ei->children, as
+		 * the list may be used for SRCU.
+		 */
+		smp_wmb();
 		put_ei(ei);
 	}
 }
@@ -627,6 +637,20 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
 	list_for_each_entry_srcu(ei_child, &ei->children, list,
 				 srcu_read_lock_held(&eventfs_srcu)) {
 
+		/*
+		 * If the ei is being freed, then the ei->children may be
+		 * being used as the rcu list, which means the next element
+		 * may be garbage. The ei->is_free is set before switching
+		 * the ei->children over to ei->rcu. The read memory barrier
+		 * here makes sure the ei_child is read before is_free is
+		 * updated.
+		 *
+		 * Matches the smp_wmb() in put_ei()
+		 */
+		smp_rmb();
+		if (ei->is_freed)
+			return -EINVAL;
+
 		if (c > 0) {
 			c--;
 			continue;
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index a4a7f8431aff..c61481d04c8e 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -46,11 +46,11 @@ struct eventfs_attr {
  * @ino:	The saved inode number
  */
 struct eventfs_inode {
+	struct list_head	list;
 	union {
-		struct list_head	list;
+		struct list_head	children;
 		struct rcu_head		rcu;
 	};
-	struct list_head		children;
 	const struct eventfs_entry	*entries;
 	const char			*name;
 	struct eventfs_attr		*entry_attrs;
-- 
2.53.0

Re: [PATCH v2] eventfs: Use children field for rcu head and add memory barriers

[Steven Rostedt]

On Fri, 7 Aug 2026 20:44:41 -0400
Steven Rostedt <steven@rostedt.org> wrote:

> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 39c7a34531e8..7d2431f2bb08 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -124,7 +124,17 @@ static inline void put_ei(struct eventfs_inode *ei)
>  static inline void free_ei(struct eventfs_inode *ei)
>  {
>  	if (ei) {
> +		/* The ei should have no children if it is being freed. */
> +		WARN_ON_ONCE(!list_empty(&ei->children));
>  		ei->is_freed = 1;
> +		/*
> +		 * The SRCU iteration has a smp_rmb() to make sure it
> +		 * sees a child (that may have already been freed)
> +		 * before it reads is_free. If is_free is set, it must
> +		 * not use the child it acquired from ei->children, as
> +		 * the list may be used for SRCU.
> +		 */
> +		smp_wmb();
>  		put_ei(ei);
>  	}
>  }
> @@ -627,6 +637,20 @@ static int eventfs_iterate(struct file *file, struct dir_context *ctx)
>  	list_for_each_entry_srcu(ei_child, &ei->children, list,
>  				 srcu_read_lock_held(&eventfs_srcu)) {
>  
> +		/*
> +		 * If the ei is being freed, then the ei->children may be
> +		 * being used as the rcu list, which means the next element
> +		 * may be garbage. The ei->is_free is set before switching
> +		 * the ei->children over to ei->rcu. The read memory barrier
> +		 * here makes sure the ei_child is read before is_free is
> +		 * updated.
> +		 *
> +		 * Matches the smp_wmb() in put_ei()

Bah! Now Sashiko is complaining that the wmb is in free_ei() and not
put_ei(). Yes it is correct, but I wish it pointed this out last time
:-p

I'll send a v3.

-- Steve


> +		 */
> +		smp_rmb();
> +		if (ei->is_freed)
> +			return -EINVAL;
> +
>  		if (c > 0) {
>  			c--;
>  			continue;
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index a4a7f8431aff..c61481d04c8e 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -46,11 +46,11 @@ struct eventfs_attr {
>   * @ino:	The saved inode number
>   */
>  struct eventfs_inode {
> +	struct list_head	list;
>  	union {
> -		struct list_head	list;
> +		struct list_head	children;
>  		struct rcu_head		rcu;
>  	};
> -	struct list_head		children;
>  	const struct eventfs_entry	*entries;
>  	const char			*name;
>  	struct eventfs_attr		*entry_attrs;

Re: [PATCH v2] eventfs: Use children field for rcu head and add memory barriers

[Steven Rostedt]

[ Replying to Sashiko ]

> --- Patch [1]: [PATCH v2] eventfs: Use children field for rcu head and add memory barriers ---
> commit 4a672c87272cb1dcd1b1464842254420c8ac2bec
> Author: Steven Rostedt <rostedt@goodmis.org>
> 
> eventfs: Use children field for rcu head and add memory barriers
> 
> When an eventfs inode is freed, it sets ei->is_freed and adds it to the
> srcu link list. This patch changes the union of the rcu list head with
> the children list and adds memory barriers to prevent SRCU iteration
> from reading a corrupt target.
> 
> Closes: https://sashiko.dev/#/patchset/20260806022719.375354-1-shuangpeng.kernel%40gmail.com
> 
> This is a pre-existing issue, but does eventfs_iterate() in
> fs/tracefs/event_inode.c truncate ctx->pos in a way that bypasses loop
> bounds checks?
> 
> If a user uses lseek(fd, offset, SEEK_SET) to set the 64-bit loff_t
> ctx->pos to 0x100000000 (4 GiB):

This shouldn't be a problem as tracefs/eventfs uses the default
maxbytes which is defined as:

  fs/super.c:     s->s_maxbytes = MAX_NON_LFS;
  include/linux/fs.h:#define      MAX_NON_LFS     ((1UL<<31) - 1)

Which ends up being 2147483647 (0x7fffffff). I tested this using code:

	unsigned long long addr;
	struct dirent *dirent;
	DIR *dir;
	char *file;
	int fd;

	if (argc != 3) {
		fprintf(stderr,"usage: %s dir addr\n",argv[0]);
		exit(-1);
	}

	file = argv[1];
	addr = strtoull(argv[2], NULL, 0);

        fd = open(file, O_RDONLY);
        if (fd < 0) {
                perror("open");
                exit(0);
        }
        printf("lseek to %llx\n", addr);
        if (lseek64(fd, addr, SEEK_SET) == (off_t)-1) {
                perror("lseek");
                exit(-1);
        }
        dir = fdopendir(fd);
        dirent = readdir(dir);
        if (!dirent) {
                printf("Nothing to read\n");
                exit(0);
        }
	printf("entry: %s\n", dirent->d_name);

The program was called "readdir" and I ran it like this:

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 2147483647
lseek to 7fffffff
Nothing to read

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 2147483648
lseek to 80000000
lseek: Invalid argument

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 5
lseek to 5
entry: filter

Which proves that pos will never be 0x100000000

~# ./readdir /sys/kernel/tracing/events/sched/sched_switch 0x100000000
lseek to 100000000
lseek: Invalid argument

-- Steve




> 
> fs/tracefs/event_inode.c:eventfs_iterate() {
>     ...
>     c = ctx->pos - 2;
> 
> Here, c = ctx->pos - 2 computes 0xFFFFFFFE, which is truncated into the
> 32-bit signed integer c as -2. The loop variable i then starts at -2:
> 
>     for (i = c; i < ei->nr_entries; i++, ctx->pos++) {
>         void *cdata = ei->data;
> 
>         entry = &ei->entries[i];
> 
> Per C integer promotion rules, the 30-bit unsigned bitfield ei->nr_entries
> is promoted to a signed int. The check evaluates as -2 < (int)ei->nr_entries,
> bypassing the bounds check. This allows an out-of-bounds array read on
> &ei->entries[-2], and subsequently calls entry->callback.
>