On Friday, April 19, 2024 11:28:14 AM CDT Peter Zijlstra wrote:
On Thu, Apr 18, 2024 at 11:35:11AM +0200, Peter Zijlstra wrote:
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.
Latest hackery... I tried testing this but I'm not having luck using the patched wine as per the other email.
I converted the rest of the direct uses of spin_lock() using the below patch and tested it myself, and it passes Wine tests. As far as I can tell the logic is correct, too; I couldn't find any races.
I'll incorporate these changes into the next revision, unless there's a good reason not to.
--- --- a/drivers/misc/ntsync.c +++ b/drivers/misc/ntsync.c @@ -569,17 +569,19 @@ static int ntsync_event_set(struct ntsync_obj *event, void __user *argp, bool pu
static int ntsync_event_reset(struct ntsync_obj *event, void __user *argp) { + struct ntsync_device *dev = event->dev; __u32 prev_state; + bool all;
if (event->type != NTSYNC_TYPE_EVENT) return -EINVAL;
- spin_lock(&event->lock); + all = ntsync_lock_obj(dev, event);
prev_state = event->u.event.signaled; event->u.event.signaled = false;
- spin_unlock(&event->lock); + ntsync_unlock_obj(dev, event, all);
if (put_user(prev_state, (__u32 __user *)argp)) return -EFAULT; @@ -590,16 +592,21 @@ static int ntsync_event_reset(struct ntsync_obj *event, void __user *argp) static int ntsync_sem_read(struct ntsync_obj *sem, void __user *argp) { struct ntsync_sem_args __user *user_args = argp; + struct ntsync_device *dev = sem->dev; struct ntsync_sem_args args; + bool all;
if (sem->type != NTSYNC_TYPE_SEM) return -EINVAL;
args.sem = 0; - spin_lock(&sem->lock); + + all = ntsync_lock_obj(dev, sem); + args.count = sem->u.sem.count; args.max = sem->u.sem.max; - spin_unlock(&sem->lock); + + ntsync_unlock_obj(dev, sem, all);
if (copy_to_user(user_args, &args, sizeof(args))) return -EFAULT; @@ -609,18 +616,23 @@ static int ntsync_sem_read(struct ntsync_obj *sem, void __user *argp) static int ntsync_mutex_read(struct ntsync_obj *mutex, void __user *argp) { struct ntsync_mutex_args __user *user_args = argp; + struct ntsync_device *dev = mutex->dev; struct ntsync_mutex_args args; + bool all; int ret;
if (mutex->type != NTSYNC_TYPE_MUTEX) return -EINVAL;
args.mutex = 0; - spin_lock(&mutex->lock); + + all = ntsync_lock_obj(dev, mutex); + args.count = mutex->u.mutex.count; args.owner = mutex->u.mutex.owner; ret = mutex->u.mutex.ownerdead ? -EOWNERDEAD : 0; - spin_unlock(&mutex->lock); + + ntsync_unlock_obj(dev, mutex, all);
if (copy_to_user(user_args, &args, sizeof(args))) return -EFAULT; @@ -630,16 +642,21 @@ static int ntsync_mutex_read(struct ntsync_obj *mutex, void __user *argp) static int ntsync_event_read(struct ntsync_obj *event, void __user *argp) { struct ntsync_event_args __user *user_args = argp; + struct ntsync_device *dev = event->dev; struct ntsync_event_args args; + bool all;
if (event->type != NTSYNC_TYPE_EVENT) return -EINVAL;
args.event = 0; - spin_lock(&event->lock); + + all = ntsync_lock_obj(dev, event); + args.manual = event->u.event.manual; args.signaled = event->u.event.signaled; - spin_unlock(&event->lock); + + ntsync_unlock_obj(dev, event, all);
if (copy_to_user(user_args, &args, sizeof(args))) return -EFAULT; @@ -962,6 +979,7 @@ static int ntsync_wait_any(struct ntsync_device *dev, void __user *argp) __u32 i, total_count; struct ntsync_q *q; int signaled; + bool all; int ret;
if (copy_from_user(&args, argp, sizeof(args))) @@ -981,9 +999,9 @@ static int ntsync_wait_any(struct ntsync_device *dev, void __user *argp) struct ntsync_q_entry *entry = &q->entries[i]; struct ntsync_obj *obj = entry->obj;
- spin_lock(&obj->lock); + all = ntsync_lock_obj(dev, obj); list_add_tail(&entry->node, &obj->any_waiters); - spin_unlock(&obj->lock); + ntsync_unlock_obj(dev, obj, all); }
/* @@ -1000,9 +1018,9 @@ static int ntsync_wait_any(struct ntsync_device *dev, void __user *argp) if (atomic_read(&q->signaled) != -1) break;
- spin_lock(&obj->lock); + all = ntsync_lock_obj(dev, obj); try_wake_any_obj(obj); - spin_unlock(&obj->lock); + ntsync_unlock_obj(dev, obj, all); }
/* sleep */ @@ -1015,9 +1033,9 @@ static int ntsync_wait_any(struct ntsync_device *dev, void __user *argp) struct ntsync_q_entry *entry = &q->entries[i]; struct ntsync_obj *obj = entry->obj;
- spin_lock(&obj->lock); + all = ntsync_lock_obj(dev, obj); list_del(&entry->node); - spin_unlock(&obj->lock); + ntsync_unlock_obj(dev, obj, all);
put_obj(obj); } @@ -1075,9 +1093,9 @@ static int ntsync_wait_all(struct ntsync_device *dev, void __user *argp) struct ntsync_q_entry *entry = &q->entries[args.count]; struct ntsync_obj *obj = entry->obj;
- spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock); + dev_lock_obj(dev, obj); list_add_tail(&entry->node, &obj->any_waiters); - spin_unlock(&obj->lock); + dev_unlock_obj(dev, obj); }
/* check if we are already signaled */ @@ -1095,9 +1113,9 @@ static int ntsync_wait_all(struct ntsync_device *dev, void __user *argp) struct ntsync_obj *obj = q->entries[args.count].obj;
if (atomic_read(&q->signaled) == -1) { - spin_lock(&obj->lock); + dev_lock_obj(dev, obj); try_wake_any_obj(obj); - spin_unlock(&obj->lock); + dev_unlock_obj(dev, obj); } }
@@ -1127,9 +1145,9 @@ static int ntsync_wait_all(struct ntsync_device *dev, void __user *argp) struct ntsync_q_entry *entry = &q->entries[args.count]; struct ntsync_obj *obj = entry->obj;
- spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock); + dev_lock_obj(dev, obj); list_del(&entry->node); - spin_unlock(&obj->lock); + dev_unlock_obj(dev, obj);
put_obj(obj); }
-
Elizabeth Figura