Re: [PATCH 6.12 163/181] eventfs: Use children field for rcu head and add memory barriers

HI Greg,

On 17/08/26 7:04 pm, Greg Kroah-Hartman wrote:
> 6.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Steven Rostedt <[email protected]>
> 
> commit f0ece16ffca7384787b692431961ce202907acf5 upstream.
> 
> 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: [email protected]
> Link: https://patch.msgid.link/20260808094215.4252430d@robin
> Fixes: 704f960dbee2f ("eventfs: Read ei->entries before ei->children in eventfs_iterate()")
> Reported-by: Sashiko <[email protected]>
> Closes: https://sashiko.dev/#/patchset/20260806022719.375354-1-shuangpeng.kernel%40gmail.com
> Reviewed-by: Masami Hiramatsu (Google) <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
>   fs/tracefs/event_inode.c |   24 ++++++++++++++++++++++++
>   fs/tracefs/internal.h    |    4 ++--
>   2 files changed, 26 insertions(+), 2 deletions(-)
> 
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -124,7 +124,17 @@ static inline void put_ei(struct eventfs
>   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);
>   	}
>   }
> @@ -647,6 +657,20 @@ static int eventfs_iterate(struct file *
>   	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 free_ei()
> +		 */
> +		smp_rmb();
> +		if (ei->is_freed)
> +			return -EINVAL;
> +

I have run an AI-assisted backport review and it spotted an issue. I
checked the affected control flow.

Upstream has automatic SRCU cleanup:

	guard(srcu)(&eventfs_srcu);
	...
	if (ei->is_freed)
		return -EINVAL;

6.12.y still uses manual cleanup:

	idx = srcu_read_lock(&eventfs_srcu);
	...
	if (ei->is_freed)
		return -EINVAL;
	...
out:
	srcu_read_unlock(&eventfs_srcu, idx);

The new direct return bypasses srcu_read_unlock(). This leaves the SRCU
reader counts unbalanced and can prevent eventfs callbacks from
completing, allowing removed inode objects to accumulate.

I would suggest adapting the new check to set ret = -EINVAL and goto
out.


thanks,
Harshit
>   		if (c > 0) {
>   			c--;
>   			continue;
> --- 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;
> 
> 
>