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;
