[PATCH 0/2] tracing/user_events: Share state between existing threads

[PATCH 0/2] tracing/user_events: Share state between existing threads

[Jeff Barnes]

Threads sharing an mm_struct can create separate user_event_mm instances
when they were created before either thread first uses user_events.  Each
instance then has its own enabler list, allowing two threads in the same
address space to register different events using the same enable address
and bit.

The first patch adds a deterministic regression test that creates both
threads before registration and verifies that the second registration is
rejected with EADDRINUSE.

The second patch makes current_user_event_mm() find and attach to an
existing user_event_mm for the current mm_struct, and synchronizes
attachment with removal of the last task.

Without the fix, the new selftest fails because the duplicate registration
succeeds.  With the fix, the registration is rejected with EADDRINUSE and
all seven user_events ABI tests pass.

---
2.43.0

[PATCH 1/2] selftests/user_events: Test registration from existing threads

[Jeff Barnes]

Create two threads before either registers a user event. Have the first
thread register an event using a shared enable address and bit while it
remains alive, then have the second thread register a different event
using the same address and bit.

The second registration should fail with EADDRINUSE because both threads
share the same address space.

This test fails without the accompanying kernel fix because each thread
can create a separate user_event_mm with an independent enabler list.

Signed-off-by: Jeff Barnes <jeffbarnes@linux.microsoft.com>
---
 .../testing/selftests/user_events/abi_test.c  | 132 ++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/tools/testing/selftests/user_events/abi_test.c b/tools/testing/selftests/user_events/abi_test.c
index b71813eaf5c0..bb1a17ce00da 100644
--- a/tools/testing/selftests/user_events/abi_test.c
+++ b/tools/testing/selftests/user_events/abi_test.c
@@ -11,6 +11,7 @@
 #include <errno.h>
 #include <linux/user_events.h>
 #include <stdio.h>
+#include <pthread.h>
 #include <stdlib.h>
 #include <fcntl.h>
 #include <sys/ioctl.h>
@@ -258,6 +259,137 @@ FIXTURE_TEARDOWN(user) {
 	USER_EVENT_FIXTURE_TEARDOWN(self->umount);
 }
 
+struct thread_registration {
+	pthread_mutex_t lock;
+	pthread_cond_t cond;
+	__u32 enable __attribute__((aligned(sizeof(__u32))));
+	bool start;
+	bool first_done;
+	bool second_done;
+	int first_ret;
+	int first_errno;
+	int first_unreg_ret;
+	int second_ret;
+	int second_errno;
+	int second_unreg_ret;
+};
+
+static int reg_enable_name(void *enable, const char *name, int *saved_errno)
+{
+	struct user_reg reg = {0};
+	int fd = open(data_file, O_RDWR);
+	int ret;
+
+	if (fd < 0) {
+		*saved_errno = errno;
+		return -1;
+	}
+
+	reg.size = sizeof(reg);
+	reg.name_args = (__u64)name;
+	reg.enable_bit = 0;
+	reg.enable_addr = (__u64)enable;
+	reg.enable_size = sizeof(__u32);
+
+	errno = 0;
+	ret = ioctl(fd, DIAG_IOCSREG, &reg);
+	*saved_errno = errno;
+
+	close(fd);
+
+	return ret;
+}
+
+static void *register_first(void *arg)
+{
+	struct thread_registration *registration = arg;
+
+	pthread_mutex_lock(&registration->lock);
+	while (!registration->start)
+		pthread_cond_wait(&registration->cond, &registration->lock);
+	pthread_mutex_unlock(&registration->lock);
+
+	registration->first_ret = reg_enable_name(&registration->enable,
+						 "__abi_event_thread_a",
+						 &registration->first_errno);
+
+	pthread_mutex_lock(&registration->lock);
+	registration->first_done = true;
+	pthread_cond_broadcast(&registration->cond);
+
+	while (!registration->second_done)
+		pthread_cond_wait(&registration->cond, &registration->lock);
+	pthread_mutex_unlock(&registration->lock);
+
+	if (!registration->first_ret)
+		registration->first_unreg_ret =
+			reg_disable(&registration->enable, 0);
+
+	return NULL;
+}
+
+static void *register_second(void *arg)
+{
+	struct thread_registration *registration = arg;
+
+	pthread_mutex_lock(&registration->lock);
+	while (!registration->start)
+		pthread_cond_wait(&registration->cond, &registration->lock);
+	while (!registration->first_done)
+		pthread_cond_wait(&registration->cond, &registration->lock);
+	pthread_mutex_unlock(&registration->lock);
+
+	registration->second_ret = reg_enable_name(&registration->enable,
+						  "__abi_event_thread_b",
+						  &registration->second_errno);
+
+	pthread_mutex_lock(&registration->lock);
+	registration->second_done = true;
+	pthread_cond_broadcast(&registration->cond);
+	pthread_mutex_unlock(&registration->lock);
+
+	if (!registration->second_ret)
+		registration->second_unreg_ret =
+			reg_disable(&registration->enable, 0);
+
+	return NULL;
+}
+
+TEST_F(user, preexisting_threads_same_address) {
+	struct thread_registration registration = {
+		.lock = PTHREAD_MUTEX_INITIALIZER,
+		.cond = PTHREAD_COND_INITIALIZER,
+	};
+	pthread_t first;
+	pthread_t second;
+
+	ASSERT_EQ(0, pthread_create(&first, NULL, register_first, &registration));
+	ASSERT_EQ(0, pthread_create(&second, NULL, register_second, &registration));
+
+	pthread_mutex_lock(&registration.lock);
+	registration.start = true;
+	pthread_cond_broadcast(&registration.cond);
+	pthread_mutex_unlock(&registration.lock);
+
+	ASSERT_EQ(0, pthread_join(first, NULL));
+	ASSERT_EQ(0, pthread_join(second, NULL));
+
+	pthread_cond_destroy(&registration.cond);
+	pthread_mutex_destroy(&registration.lock);
+
+	ASSERT_EQ(0, registration.first_ret);
+	ASSERT_EQ(0, registration.first_errno);
+
+	if (!registration.first_ret)
+		ASSERT_EQ(0, registration.first_unreg_ret);
+
+	ASSERT_EQ(-1, registration.second_ret);
+	ASSERT_EQ(EADDRINUSE, registration.second_errno);
+
+	if (!registration.second_ret)
+		ASSERT_EQ(0, registration.second_unreg_ret);
+}
+
 TEST_F(user, enablement) {
 	/* Changes should be reflected immediately */
 	ASSERT_EQ(0, self->check);
-- 
2.43.0

[PATCH 2/2] tracing/user_events: Share tracing state between existing threads

[Jeff Barnes]

Threads created before the first user-events registration can share an
mm_struct while each has a NULL user_event_mm pointer.

When one thread registers an event, current_user_event_mm() creates a
user_event_mm and attaches it only to that task. A pre-existing sibling
thread sharing the same mm_struct can later create another user_event_mm.
The two wrappers have independent enabler lists, allowing both threads to
register different events using the same enable address and bit despite
the EADDRINUSE check.

Look for an active user_event_mm associated with the current mm_struct
before allocating a new one. Attach the current task to that wrapper and
increment its task count.

Serialize lookup and attachment with the final task-count decrement and
list removal so that a task cannot attach to a wrapper after its
last-task transition has begun.

Without this change, the new selftest reports that the second
registration succeeds. With the change, it is rejected with
EADDRINUSE, and all user_events ABI tests pass.

Signed-off-by: Jeff Barnes <jeffbarnes@linux.microsoft.com>
---
 kernel/trace/trace_events_user.c | 78 ++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index f658c3a77aa7..950d8a3cd0e3 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -210,6 +210,7 @@ static int user_event_parse(struct user_event_group *group, char *name,
 
 static struct user_event_mm *user_event_mm_get(struct user_event_mm *mm);
 static struct user_event_mm *user_event_mm_get_all(struct user_event *user);
+static void user_event_mm_destroy(struct user_event_mm *mm);
 static void user_event_mm_put(struct user_event_mm *mm);
 static int destroy_user_event(struct user_event *user);
 static bool user_fields_match(struct user_event *user, int argc,
@@ -754,33 +755,78 @@ static struct user_event_mm *user_event_mm_alloc(struct task_struct *t)
 	return user_mm;
 }
 
+static struct user_event_mm *
+user_event_mm_find_locked(struct mm_struct *mm)
+{
+	struct user_event_mm *user_mm;
+
+	lockdep_assert_held(&user_event_mms_lock);
+
+	list_for_each_entry(user_mm, &user_event_mms, mms_link)
+		if (user_mm->mm == mm)
+			return user_mm;
+
+	return NULL;
+}
+
 static void user_event_mm_attach(struct user_event_mm *user_mm, struct task_struct *t)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&user_event_mms_lock, flags);
 	list_add_rcu(&user_mm->mms_link, &user_event_mms);
-	spin_unlock_irqrestore(&user_event_mms_lock, flags);
-
 	t->user_event_mm = user_mm;
+	spin_unlock_irqrestore(&user_event_mms_lock, flags);
 }
 
 static struct user_event_mm *current_user_event_mm(void)
 {
+	struct user_event_mm *new_mm;
 	struct user_event_mm *user_mm = current->user_event_mm;
+	unsigned long flags;
 
 	if (user_mm)
-		goto inc;
+		return user_event_mm_get(user_mm);
 
-	user_mm = user_event_mm_alloc(current);
+	spin_lock_irqsave(&user_event_mms_lock, flags);
 
-	if (!user_mm)
-		goto error;
+	user_mm = user_event_mm_find_locked(current->mm);
+
+	if (user_mm) {
+		refcount_inc(&user_mm->tasks);
+		current->user_event_mm = user_mm;
+		user_event_mm_get(user_mm);
+	}
+
+	spin_unlock_irqrestore(&user_event_mms_lock, flags);
+
+	if (user_mm)
+		return user_mm;
+
+	new_mm = user_event_mm_alloc(current);
+
+	spin_lock_irqsave(&user_event_mms_lock, flags);
+
+	user_mm = user_event_mm_find_locked(current->mm);
+
+	if (user_mm) {
+		refcount_inc(&user_mm->tasks);
+	} else if (new_mm) {
+		user_mm = new_mm;
+		list_add_rcu(&user_mm->mms_link, &user_event_mms);
+	} else {
+		spin_unlock_irqrestore(&user_event_mms_lock, flags);
+		return NULL;
+	}
+
+	current->user_event_mm = user_mm;
+	user_event_mm_get(user_mm);
+
+	spin_unlock_irqrestore(&user_event_mms_lock, flags);
+
+	if (new_mm && new_mm != user_mm)
+		user_event_mm_destroy(new_mm);
 
-	user_event_mm_attach(user_mm, current);
-inc:
-	refcount_inc(&user_mm->refcnt);
-error:
 	return user_mm;
 }
 
@@ -817,14 +863,18 @@ void user_event_mm_remove(struct task_struct *t)
 	might_sleep();
 
 	mm = t->user_event_mm;
+
+	spin_lock_irqsave(&user_event_mms_lock, flags);
+
 	t->user_event_mm = NULL;
 
-	/* Clone will increment the tasks, only remove if last clone */
-	if (!refcount_dec_and_test(&mm->tasks))
+	/* Clones and attached tasks increment this count. */
+	if (!refcount_dec_and_test(&mm->tasks)) {
+		spin_unlock_irqrestore(&user_event_mms_lock, flags);
 		return;
+	}
 
-	/* Remove the mm from the list, so it can no longer be enabled */
-	spin_lock_irqsave(&user_event_mms_lock, flags);
+	/* Prevent new tasks from attaching after the last-task transition. */
 	list_del_rcu(&mm->mms_link);
 	spin_unlock_irqrestore(&user_event_mms_lock, flags);
 
-- 
2.43.0