Message-ID: <20240418093511.GQ40213@noisy.programming.kicks-ass.net>
Date: Thu, 18 Apr 2024 11:35:11 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Elizabeth Figura <zfigura@...eweavers.com>
Cc: Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	wine-devel@...ehq.org,
	André Almeida <andrealmeid@...lia.com>,
	Wolfram Sang <wsa@...nel.org>,
	Arkadiusz Hiler <ahiler@...eweavers.com>,
	Andy Lutomirski <luto@...nel.org>, linux-doc@...r.kernel.org,
	linux-kselftest@...r.kernel.org,
	Randy Dunlap <rdunlap@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>
Subject: Re: [PATCH v4 02/27] ntsync: Introduce NTSYNC_IOC_WAIT_ALL.

On Wed, Apr 17, 2024 at 03:03:05PM -0500, Elizabeth Figura wrote:

> Ach. I wrote this with the idea that the race isn't meaningful, but
> looking at it again you're right—there is a harmful race here.
> 
> I think it should be fixable by moving the atomic_read inside the lock,
> though.

Right, I've ended up with the (as yet untested) below. I'll see if I can
find time later to actually test things.

---

--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -18,6 +18,7 @@
 #include <linux/sched/signal.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <uapi/linux/ntsync.h>
 
 #define NTSYNC_NAME	"ntsync"
@@ -43,6 +44,7 @@ enum ntsync_type {
 
 struct ntsync_obj {
 	spinlock_t lock;
+	int dev_locked;
 
 	enum ntsync_type type;
 
@@ -132,7 +134,7 @@ struct ntsync_device {
 	 * wait_all_lock is taken first whenever multiple objects must be locked
 	 * at the same time.
 	 */
-	spinlock_t wait_all_lock;
+	struct mutex wait_all_lock;
 
 	struct file *file;
 };
@@ -157,6 +159,56 @@ static bool is_signaled(struct ntsync_ob
 }
 
 /*
+ * XXX write coherent comment on the locking
+ */
+
+static void dev_lock_obj(struct ntsync_device *dev, struct ntsync_obj *obj)
+{
+	lockdep_assert_held(&dev->wait_all_lock);
+	WARN_ON_ONCE(obj->dev != dev);
+	spin_lock(&obj->lock);
+	obj->dev_locked = 1;
+	spin_unlock(&obj->lock);
+}
+
+static void dev_unlock_obj(struct ntsync_device *dev, struct ntsync_obj *obj)
+{
+	lockdep_assert_held(&dev->wait_all_lock);
+	WARN_ON_ONCE(obj->dev != dev);
+	spin_lock(&obj->lock);
+	obj->dev_locked = 0;
+	spin_unlock(&obj->lock);
+}
+
+static void obj_lock(struct ntsync_obj *obj)
+{
+	struct ntsync_device *dev = obj->dev;
+
+	for (;;) {
+		spin_lock(&obj->lock);
+		if (likely(!obj->dev_locked))
+			break;
+
+		spin_unlock(&obj->lock);
+		mutex_lock(&dev->wait_all_lock);
+		spin_lock(&obj->lock);
+		/*
+		 * obj->dev_locked should be set and released under the same
+		 * wait_all_lock section, since we now own this lock, it should
+		 * be clear.
+		 */
+		WARN_ON_ONCE(obj->dev_locked);
+		spin_unlock(&obj->lock);
+		mutex_unlock(&dev->wait_all_lock);
+	}
+}
+
+static void obj_unlock(struct ntsync_obj *obj)
+{
+	spin_unlock(&obj->lock);
+}
+
+/*
  * "locked_obj" is an optional pointer to an object which is already locked and
  * should not be locked again. This is necessary so that changing an object's
  * state and waking it can be a single atomic operation.
@@ -175,7 +227,7 @@ static void try_wake_all(struct ntsync_d
 
 	for (i = 0; i < count; i++) {
 		if (q->entries[i].obj != locked_obj)
-			spin_lock_nest_lock(&q->entries[i].obj->lock, &dev->wait_all_lock);
+			dev_lock_obj(dev, q->entries[i].obj);
 	}
 
 	for (i = 0; i < count; i++) {
@@ -211,7 +263,7 @@ static void try_wake_all(struct ntsync_d
 
 	for (i = 0; i < count; i++) {
 		if (q->entries[i].obj != locked_obj)
-			spin_unlock(&q->entries[i].obj->lock);
+			dev_unlock_obj(dev, q->entries[i].obj);
 	}
 }
 
@@ -231,6 +283,7 @@ static void try_wake_any_sem(struct ntsy
 	struct ntsync_q_entry *entry;
 
 	lockdep_assert_held(&sem->lock);
+	WARN_ON_ONCE(sem->type != NTSYNC_TYPE_SEM);
 
 	list_for_each_entry(entry, &sem->any_waiters, node) {
 		struct ntsync_q *q = entry->q;
@@ -251,6 +304,7 @@ static void try_wake_any_mutex(struct nt
 	struct ntsync_q_entry *entry;
 
 	lockdep_assert_held(&mutex->lock);
+	WARN_ON_ONCE(mutex->type != NTSYNC_TYPE_MUTEX);
 
 	list_for_each_entry(entry, &mutex->any_waiters, node) {
 		struct ntsync_q *q = entry->q;
@@ -302,6 +356,7 @@ static int post_sem_state(struct ntsync_
 	__u32 sum;
 
 	lockdep_assert_held(&sem->lock);
+	WARN_ON_ONCE(sem->type != NTSYNC_TYPE_SEM);
 
 	if (check_add_overflow(sem->u.sem.count, count, &sum) ||
 	    sum > sem->u.sem.max)
@@ -316,6 +371,7 @@ static int ntsync_sem_post(struct ntsync
 	struct ntsync_device *dev = sem->dev;
 	__u32 __user *user_args = argp;
 	__u32 prev_count;
+	bool all = false;
 	__u32 args;
 	int ret;
 
@@ -325,28 +381,27 @@ static int ntsync_sem_post(struct ntsync
 	if (sem->type != NTSYNC_TYPE_SEM)
 		return -EINVAL;
 
-	if (atomic_read(&sem->all_hint) > 0) {
-		spin_lock(&dev->wait_all_lock);
-		spin_lock_nest_lock(&sem->lock, &dev->wait_all_lock);
-
-		prev_count = sem->u.sem.count;
-		ret = post_sem_state(sem, args);
-		if (!ret) {
+	obj_lock(sem);
+	all = atomic_read(&sem->all_hint);
+	if (unlikely(all)) {
+		obj_unlock(sem);
+		mutex_lock(&dev->wait_all_lock);
+		dev_lock_obj(dev, sem);
+	}
+
+	prev_count = sem->u.sem.count;
+	ret = post_sem_state(sem, args);
+	if (!ret) {
+		if (all)
 			try_wake_all_obj(dev, sem);
-			try_wake_any_sem(sem);
-		}
+		try_wake_any_sem(sem);
+	}
 
-		spin_unlock(&sem->lock);
-		spin_unlock(&dev->wait_all_lock);
+	if (all) {
+		dev_unlock_obj(dev, sem);
+		mutex_unlock(&dev->wait_all_lock);
 	} else {
-		spin_lock(&sem->lock);
-
-		prev_count = sem->u.sem.count;
-		ret = post_sem_state(sem, args);
-		if (!ret)
-			try_wake_any_sem(sem);
-
-		spin_unlock(&sem->lock);
+		obj_unlock(sem);
 	}
 
 	if (!ret && put_user(prev_count, user_args))
@@ -376,6 +431,7 @@ static int ntsync_mutex_unlock(struct nt
 	struct ntsync_mutex_args __user *user_args = argp;
 	struct ntsync_device *dev = mutex->dev;
 	struct ntsync_mutex_args args;
+	bool all = false;
 	__u32 prev_count;
 	int ret;
 
@@ -387,28 +443,27 @@ static int ntsync_mutex_unlock(struct nt
 	if (mutex->type != NTSYNC_TYPE_MUTEX)
 		return -EINVAL;
 
-	if (atomic_read(&mutex->all_hint) > 0) {
-		spin_lock(&dev->wait_all_lock);
-		spin_lock_nest_lock(&mutex->lock, &dev->wait_all_lock);
-
-		prev_count = mutex->u.mutex.count;
-		ret = unlock_mutex_state(mutex, &args);
-		if (!ret) {
+	obj_lock(mutex);
+	all = atomic_read(&mutex->all_hint);
+	if (unlikely(all)) {
+		obj_unlock(mutex);
+		mutex_lock(&dev->wait_all_lock);
+		dev_lock_obj(dev, mutex);
+	}
+
+	prev_count = mutex->u.mutex.count;
+	ret = unlock_mutex_state(mutex, &args);
+	if (!ret) {
+		if (all)
 			try_wake_all_obj(dev, mutex);
-			try_wake_any_mutex(mutex);
-		}
+		try_wake_any_mutex(mutex);
+	}
 
-		spin_unlock(&mutex->lock);
-		spin_unlock(&dev->wait_all_lock);
+	if (all) {
+		dev_unlock_obj(dev, mutex);
+		mutex_unlock(&dev->wait_all_lock);
 	} else {
-		spin_lock(&mutex->lock);
-
-		prev_count = mutex->u.mutex.count;
-		ret = unlock_mutex_state(mutex, &args);
-		if (!ret)
-			try_wake_any_mutex(mutex);
-
-		spin_unlock(&mutex->lock);
+		obj_unlock(mutex);
 	}
 
 	if (!ret && put_user(prev_count, &user_args->count))
@@ -437,6 +492,7 @@ static int kill_mutex_state(struct ntsyn
 static int ntsync_mutex_kill(struct ntsync_obj *mutex, void __user *argp)
 {
 	struct ntsync_device *dev = mutex->dev;
+	bool all = false;
 	__u32 owner;
 	int ret;
 
@@ -448,26 +504,26 @@ static int ntsync_mutex_kill(struct ntsy
 	if (mutex->type != NTSYNC_TYPE_MUTEX)
 		return -EINVAL;
 
-	if (atomic_read(&mutex->all_hint) > 0) {
-		spin_lock(&dev->wait_all_lock);
-		spin_lock_nest_lock(&mutex->lock, &dev->wait_all_lock);
+	obj_lock(mutex);
+	all = atomic_read(&mutex->all_hint);
+	if (unlikely(all)) {
+		obj_unlock(mutex);
+		mutex_lock(&dev->wait_all_lock);
+		dev_lock_obj(dev, mutex);
+	}
 
-		ret = kill_mutex_state(mutex, owner);
-		if (!ret) {
+	ret = kill_mutex_state(mutex, owner);
+	if (!ret) {
+		if (all)
 			try_wake_all_obj(dev, mutex);
-			try_wake_any_mutex(mutex);
-		}
+		try_wake_any_mutex(mutex);
+	}
 
-		spin_unlock(&mutex->lock);
-		spin_unlock(&dev->wait_all_lock);
+	if (all) {
+		dev_unlock_obj(dev, mutex);
+		mutex_unlock(&dev->wait_all_lock);
 	} else {
-		spin_lock(&mutex->lock);
-
-		ret = kill_mutex_state(mutex, owner);
-		if (!ret)
-			try_wake_any_mutex(mutex);
-
-		spin_unlock(&mutex->lock);
+		obj_unlock(mutex);
 	}
 
 	return ret;
@@ -477,35 +533,35 @@ static int ntsync_event_set(struct ntsyn
 {
 	struct ntsync_device *dev = event->dev;
 	__u32 prev_state;
+	bool all = false;
 
 	if (event->type != NTSYNC_TYPE_EVENT)
 		return -EINVAL;
 
-	if (atomic_read(&event->all_hint) > 0) {
-		spin_lock(&dev->wait_all_lock);
-		spin_lock_nest_lock(&event->lock, &dev->wait_all_lock);
+	obj_lock(event);
+	all = atomic_read(&event->all_hint);
+	if (unlikely(all)) {
+		obj_unlock(event);
+		mutex_lock(&dev->wait_all_lock);
+		dev_lock_obj(dev, event);
+	}
 
-		prev_state = event->u.event.signaled;
-		event->u.event.signaled = true;
+	prev_state = event->u.event.signaled;
+	event->u.event.signaled = true;
+	if (all)
 		try_wake_all_obj(dev, event);
-		try_wake_any_event(event);
-		if (pulse)
-			event->u.event.signaled = false;
-
-		spin_unlock(&event->lock);
-		spin_unlock(&dev->wait_all_lock);
+	try_wake_any_event(event);
+	if (pulse)
+		event->u.event.signaled = false;
+
+	if (all) {
+		dev_unlock_obj(dev, event);
+		mutex_unlock(&dev->wait_all_lock);
 	} else {
-		spin_lock(&event->lock);
-
-		prev_state = event->u.event.signaled;
-		event->u.event.signaled = true;
-		try_wake_any_event(event);
-		if (pulse)
-			event->u.event.signaled = false;
-
-		spin_unlock(&event->lock);
+		obj_unlock(event);
 	}
 
+
 	if (put_user(prev_state, (__u32 __user *)argp))
 		return -EFAULT;
 
@@ -984,7 +1040,7 @@ static int ntsync_wait_all(struct ntsync
 
 	/* queue ourselves */
 
-	spin_lock(&dev->wait_all_lock);
+	mutex_lock(&dev->wait_all_lock);
 
 	for (i = 0; i < args.count; i++) {
 		struct ntsync_q_entry *entry = &q->entries[i];
@@ -1004,7 +1060,7 @@ static int ntsync_wait_all(struct ntsync
 
 	try_wake_all(dev, q, NULL);
 
-	spin_unlock(&dev->wait_all_lock);
+	mutex_unlock(&dev->wait_all_lock);
 
 	/* sleep */
 
@@ -1012,7 +1068,7 @@ static int ntsync_wait_all(struct ntsync
 
 	/* and finally, unqueue */
 
-	spin_lock(&dev->wait_all_lock);
+	mutex_lock(&dev->wait_all_lock);
 
 	for (i = 0; i < args.count; i++) {
 		struct ntsync_q_entry *entry = &q->entries[i];
@@ -1029,7 +1085,7 @@ static int ntsync_wait_all(struct ntsync
 		put_obj(obj);
 	}
 
-	spin_unlock(&dev->wait_all_lock);
+	mutex_unlock(&dev->wait_all_lock);
 
 	signaled = atomic_read(&q->signaled);
 	if (signaled != -1) {
@@ -1056,7 +1112,7 @@ static int ntsync_char_open(struct inode
 	if (!dev)
 		return -ENOMEM;
 
-	spin_lock_init(&dev->wait_all_lock);
+	mutex_init(&dev->wait_all_lock);
 
 	file->private_data = dev;
 	dev->file = file;

Powered by blists - more mailing lists