Re: [PATCH v4 02/27] ntsync: Introduce NTSYNC_IOC_WAIT_ALL.

[Elizabeth Figura <zfigura@codeweavers.com>]

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);
 	}