tracefs: Fixes for 7.1

tracefs fixes for v7.1:

- Use list_add_tail_rcu() for walking eventfs children

The children link list is protected by SRCU and list walkers can walk the list without the lock and only using SRCU. Using just list_add_tail() on weakly ordered architectures can cause issues. Instead use list_add_tail_rcu().

- Hold eventfs_mutex and SRCU for remount walk events

The trace_apply_options() walks the tracefs_inodes where some are eventfs inodes and eventfs_remount() is called which in turn calls eventfs_set_attr(). This walk only holds normal RCU read locks, but the eventfs_mutex and SRCU should be held. Add a eventfs_remount_(un)lock() helpers to take the necessary locks before iterating the list.

git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
tracefs/fixes

Head SHA1: 07004a8c4b572171934390148ee48c4175c77eed

David Carlier (2):
eventfs: Use list_add_tail_rcu() for SRCU-protected children list
eventfs: Hold eventfs_mutex and SRCU when remount walks events

----

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

From: David Carlier <[email protected]>

Commit d2603279c7d6 ("eventfs: Use list_del_rcu() for SRCU protected list variable") converted the removal side to pair with the list_for_each_entry_srcu() walker in eventfs_iterate(). The insertion in eventfs_create_dir() was left as a plain list_add_tail(), which on weakly-ordered architectures can expose a new entry to the SRCU reader before its list pointers and fields are observable.

Use list_add_tail_rcu() so the publication pairs with the existing list_del_rcu() and list_for_each_entry_srcu().

---
 fs/tracefs/event_inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 81df94038f2e..8dd554508828 100644
@@ -706,7 +706,7 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
 
 	scoped_guard(mutex, &eventfs_mutex) {
 		if (!parent->is_freed)
-			list_add_tail(&ei->list, &parent->children);
+			list_add_tail_rcu(&ei->list, &parent->children);
 	}
 	/* Was the parent freed? */
 	if (list_empty(&ei->list)) {

2.51.0

From: David Carlier <[email protected]>

Commit 340f0c7067a9 ("eventfs: Update all the eventfs_inodes from the events descriptor") had eventfs_set_attrs() recurse through ei->children on remount. The walk only holds the rcu_read_lock() taken by tracefs_apply_options() over tracefs_inodes, which is wrong:

- list_for_each_entry over ei->children races with the list_del_rcu() in eventfs_remove_rec() -- LIST_POISON1 deref, same shape as d2603279c7d6.
- eventfs_inodes are freed via call_srcu(&eventfs_srcu, ...). rcu_read_lock() does not extend an SRCU grace period, so ti->private can be reclaimed under the walk.
- The writes to ei->attr race with eventfs_set_attr(), which holds eventfs_mutex.

Reproducer:

  while :; do mount -o remount,uid=$((RANDOM%1000)) /sys/kernel/tracing; done &
  while :; do
      echo "p:kp submit_bio" > /sys/kernel/tracing/kprobe_events
      echo > /sys/kernel/tracing/kprobe_events
  done

Wrap the events portion of tracefs_apply_options() in eventfs_remount_lock()/_unlock() that take eventfs_mutex and srcu_read_lock(&eventfs_srcu). eventfs_set_attrs() doesn't sleep so the nested rcu_read_lock() is fine; lockdep_assert_held() pins the contract.

Comment in tracefs_drop_inode() said "RCU cycle" -- it is SRCU.

---
 fs/tracefs/event_inode.c | 14 ++++++++++++++
 fs/tracefs/inode.c       |  5 ++++-
 fs/tracefs/internal.h    |  3 +++
 3 files changed, 21 insertions(+), 1 deletion(-)

index 8dd554508828..26b6453de30e 100644
@@ -244,6 +244,8 @@ static void eventfs_set_attrs(struct eventfs_inode *ei, bool update_uid, kuid_t
 {
 	struct eventfs_inode *ei_child;
 
+	lockdep_assert_held(&eventfs_mutex);
+
 	/* Update events/<system>/<event> */
 	if (WARN_ON_ONCE(level > 3))
 		return;

@@ -886,3 +888,15 @@ void eventfs_remove_events_dir(struct eventfs_inode *ei)
 	d_invalidate(dentry);
 	d_make_discardable(dentry);
 }
+
+int eventfs_remount_lock(void)
+{
+	mutex_lock(&eventfs_mutex);
+	return srcu_read_lock(&eventfs_srcu);
+}
+
+void eventfs_remount_unlock(int srcu_idx)
+{
+	srcu_read_unlock(&eventfs_srcu, srcu_idx);
+	mutex_unlock(&eventfs_mutex);
+}

index 5602baf980f6..1e8a78c5e996 100644
@@ -313,6 +313,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
 	struct inode *inode = d_inode(sb->s_root);
 	struct tracefs_inode *ti;
 	bool update_uid, update_gid;
+	int srcu_idx;
 	umode_t tmp_mode;
 
 	/*

@@ -337,6 +338,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
 		update_uid = fsi->opts & BIT(Opt_uid);
 		update_gid = fsi->opts & BIT(Opt_gid);
 
+		srcu_idx = eventfs_remount_lock();
 		rcu_read_lock();
 		list_for_each_entry_rcu(ti, &tracefs_inodes, list) {
 			if (update_uid) {

@@ -358,6 +360,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
 				eventfs_remount(ti, update_uid, update_gid);
 		}
 		rcu_read_unlock();
+		eventfs_remount_unlock(srcu_idx);
 	}
 
 	return 0;

@@ -403,7 +406,7 @@ static int tracefs_drop_inode(struct inode *inode)
 	 * This inode is being freed and cannot be used for
 	 * eventfs. Clear the flag so that it doesn't call into
 	 * eventfs during the remount flag updates. The eventfs_inode
-	 * gets freed after an RCU cycle, so the content will still
+	 * gets freed after an SRCU cycle, so the content will still
 	 * be safe if the iteration is going on now.
 	 */
 	ti->flags &= ~TRACEFS_EVENT_INODE;

index d83c2a25f288..a4a7f8431aff 100644
@@ -76,4 +76,7 @@ struct inode *tracefs_get_inode(struct super_block *sb);
 void eventfs_remount(struct tracefs_inode *ti, bool update_uid, bool update_gid);
 void eventfs_d_release(struct dentry *dentry);
 
+int eventfs_remount_lock(void);
+void eventfs_remount_unlock(int srcu_idx);
+
 #endif /* _TRACEFS_INTERNAL_H */

2.51.0