Skip to contentSkip to conversation

eventfs: Use children field for rcu head and add memory barriers

PATCH In review Steven Rostedt 1 participant 2 messages Aug 8 Tracing

lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

PATCH 1/1 eventfs: Use children field for rcu head and add memory barriers

From: Steven Rostedt <[email protected]>

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]
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]>

---
Changes since v2: https://patch.msgid.link/[email protected]

- Fix comment about smp_wmb() being in put_ei() and not free_ei().

 fs/tracefs/event_inode.c | 24 ++++++++++++++++++++++++
 fs/tracefs/internal.h    |  4 ++--
 2 files changed, 26 insertions(+), 2 deletions(-)
fs/tracefs/event_inode.c +24 -0
index 39c7a34531e8..bc82d3284a2e 100644
@@ -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 free_ei()
+		 */
+		smp_rmb();
+		if (ei->is_freed)
+			return -EINVAL;
+
 		if (c > 0) {
 			c--;
 			continue;
fs/tracefs/internal.h +2 -2
index a4a7f8431aff..c61481d04c8e 100644
@@ -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;
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

I forgot to make the subject say "v3"

-- Steve