Received-SPF: pass (google.com: domain of linux-kernel+bounces-178303-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Elizabeth Figura <zfigura@codeweavers.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>,
 linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
 wine-devel@winehq.org,
 =?ISO-8859-1?Q?Andr=E9?= Almeida <andrealmeid@igalia.com>,
 Wolfram Sang <wsa@kernel.org>, Arkadiusz Hiler <ahiler@codeweavers.com>,
 Andy Lutomirski <luto@kernel.org>, linux-doc@vger.kernel.org,
 linux-kselftest@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
 Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
 Waiman Long <longman@redhat.com>, Boqun Feng <boqun.feng@gmail.com>
Subject: Re: [PATCH v4 02/27] ntsync: Introduce NTSYNC_IOC_WAIT_ALL.
Date: Mon, 13 May 2024 23:15:42 -0500
Message-ID: <4629754.LvFx2qVVIh@watership>
In-Reply-To: <20240419162814.GA39162@noisy.programming.kicks-ass.net>
References:
 <20240416010837.333694-1-zfigura@codeweavers.com>
 <20240418093511.GQ40213@noisy.programming.kicks-ass.net>
 <20240419162814.GA39162@noisy.programming.kicks-ass.net>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"

On Friday, April 19, 2024 11:28:14=E2=80=AFAM 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=E2=80=94there is a harmful race here.
> > >=20
> > > I think it should be fixable by moving the atomic_read inside the loc=
k,
> > > though.
> >=20
> > Right, I've ended up with the (as yet untested) below. I'll see if I can
> > find time later to actually test things.
>=20
> Latest hackery... I tried testing this but I'm not having luck using the
> patched wine as per the other email.
>=20

I converted the rest of the direct uses of spin_lock() using the below patc=
h=20
and tested it myself, and it passes Wine tests. As far as I can tell the lo=
gic=20
is correct, too; I couldn't find any races.

I'll incorporate these changes into the next revision, unless there's a goo=
d=20
reason not to.

=2D--
=2D-- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -569,17 +569,19 @@ static int ntsync_event_set(struct ntsync_obj *event,=
=20
void __user *argp, bool pu
=20
 static int ntsync_event_reset(struct ntsync_obj *event, void __user *argp)
 {
+	struct ntsync_device *dev =3D event->dev;
 	__u32 prev_state;
+	bool all;
=20
 	if (event->type !=3D NTSYNC_TYPE_EVENT)
 		return -EINVAL;
=20
=2D	spin_lock(&event->lock);
+	all =3D ntsync_lock_obj(dev, event);
=20
 	prev_state =3D event->u.event.signaled;
 	event->u.event.signaled =3D false;
=20
=2D	spin_unlock(&event->lock);
+	ntsync_unlock_obj(dev, event, all);
=20
 	if (put_user(prev_state, (__u32 __user *)argp))
 		return -EFAULT;
@@ -590,16 +592,21 @@ static int ntsync_event_reset(struct ntsync_obj *even=
t,=20
void __user *argp)
 static int ntsync_sem_read(struct ntsync_obj *sem, void __user *argp)
 {
 	struct ntsync_sem_args __user *user_args =3D argp;
+	struct ntsync_device *dev =3D sem->dev;
 	struct ntsync_sem_args args;
+	bool all;
=20
 	if (sem->type !=3D NTSYNC_TYPE_SEM)
 		return -EINVAL;
=20
 	args.sem =3D 0;
=2D	spin_lock(&sem->lock);
+
+	all =3D ntsync_lock_obj(dev, sem);
+
 	args.count =3D sem->u.sem.count;
 	args.max =3D sem->u.sem.max;
=2D	spin_unlock(&sem->lock);
+
+	ntsync_unlock_obj(dev, sem, all);
=20
 	if (copy_to_user(user_args, &args, sizeof(args)))
 		return -EFAULT;
@@ -609,18 +616,23 @@ static int ntsync_sem_read(struct ntsync_obj *sem, vo=
id=20
__user *argp)
 static int ntsync_mutex_read(struct ntsync_obj *mutex, void __user *argp)
 {
 	struct ntsync_mutex_args __user *user_args =3D argp;
+	struct ntsync_device *dev =3D mutex->dev;
 	struct ntsync_mutex_args args;
+	bool all;
 	int ret;
=20
 	if (mutex->type !=3D NTSYNC_TYPE_MUTEX)
 		return -EINVAL;
=20
 	args.mutex =3D 0;
=2D	spin_lock(&mutex->lock);
+
+	all =3D ntsync_lock_obj(dev, mutex);
+
 	args.count =3D mutex->u.mutex.count;
 	args.owner =3D mutex->u.mutex.owner;
 	ret =3D mutex->u.mutex.ownerdead ? -EOWNERDEAD : 0;
=2D	spin_unlock(&mutex->lock);
+
+	ntsync_unlock_obj(dev, mutex, all);
=20
 	if (copy_to_user(user_args, &args, sizeof(args)))
 		return -EFAULT;
@@ -630,16 +642,21 @@ static int ntsync_mutex_read(struct ntsync_obj *mutex=
,=20
void __user *argp)
 static int ntsync_event_read(struct ntsync_obj *event, void __user *argp)
 {
 	struct ntsync_event_args __user *user_args =3D argp;
+	struct ntsync_device *dev =3D event->dev;
 	struct ntsync_event_args args;
+	bool all;
=20
 	if (event->type !=3D NTSYNC_TYPE_EVENT)
 		return -EINVAL;
=20
 	args.event =3D 0;
=2D	spin_lock(&event->lock);
+
+	all =3D ntsync_lock_obj(dev, event);
+
 	args.manual =3D event->u.event.manual;
 	args.signaled =3D event->u.event.signaled;
=2D	spin_unlock(&event->lock);
+
+	ntsync_unlock_obj(dev, event, all);
=20
 	if (copy_to_user(user_args, &args, sizeof(args)))
 		return -EFAULT;
@@ -962,6 +979,7 @@ static int ntsync_wait_any(struct ntsync_device *dev, v=
oid=20
__user *argp)
 	__u32 i, total_count;
 	struct ntsync_q *q;
 	int signaled;
+	bool all;
 	int ret;
=20
 	if (copy_from_user(&args, argp, sizeof(args)))
@@ -981,9 +999,9 @@ static int ntsync_wait_any(struct ntsync_device *dev, v=
oid=20
__user *argp)
 		struct ntsync_q_entry *entry =3D &q->entries[i];
 		struct ntsync_obj *obj =3D entry->obj;
=20
=2D		spin_lock(&obj->lock);
+		all =3D ntsync_lock_obj(dev, obj);
 		list_add_tail(&entry->node, &obj->any_waiters);
=2D		spin_unlock(&obj->lock);
+		ntsync_unlock_obj(dev, obj, all);
 	}
=20
 	/*
@@ -1000,9 +1018,9 @@ static int ntsync_wait_any(struct ntsync_device *dev,=
=20
void __user *argp)
 		if (atomic_read(&q->signaled) !=3D -1)
 			break;
=20
=2D		spin_lock(&obj->lock);
+		all =3D ntsync_lock_obj(dev, obj);
 		try_wake_any_obj(obj);
=2D		spin_unlock(&obj->lock);
+		ntsync_unlock_obj(dev, obj, all);
 	}
=20
 	/* sleep */
@@ -1015,9 +1033,9 @@ static int ntsync_wait_any(struct ntsync_device *dev,=
=20
void __user *argp)
 		struct ntsync_q_entry *entry =3D &q->entries[i];
 		struct ntsync_obj *obj =3D entry->obj;
=20
=2D		spin_lock(&obj->lock);
+		all =3D ntsync_lock_obj(dev, obj);
 		list_del(&entry->node);
=2D		spin_unlock(&obj->lock);
+		ntsync_unlock_obj(dev, obj, all);
=20
 		put_obj(obj);
 	}
@@ -1075,9 +1093,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,=
=20
void __user *argp)
 		struct ntsync_q_entry *entry =3D &q->entries[args.count];
 		struct ntsync_obj *obj =3D entry->obj;
=20
=2D		spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock);
+		dev_lock_obj(dev, obj);
 		list_add_tail(&entry->node, &obj->any_waiters);
=2D		spin_unlock(&obj->lock);
+		dev_unlock_obj(dev, obj);
 	}
=20
 	/* check if we are already signaled */
@@ -1095,9 +1113,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,=
=20
void __user *argp)
 		struct ntsync_obj *obj =3D q->entries[args.count].obj;
=20
 		if (atomic_read(&q->signaled) =3D=3D -1) {
=2D			spin_lock(&obj->lock);
+			dev_lock_obj(dev, obj);
 			try_wake_any_obj(obj);
=2D			spin_unlock(&obj->lock);
+			dev_unlock_obj(dev, obj);
 		}
 	}
=20
@@ -1127,9 +1145,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,=
=20
void __user *argp)
 		struct ntsync_q_entry *entry =3D &q->entries[args.count];
 		struct ntsync_obj *obj =3D entry->obj;
=20
=2D		spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock);
+		dev_lock_obj(dev, obj);
 		list_del(&entry->node);
=2D		spin_unlock(&obj->lock);
+		dev_unlock_obj(dev, obj);
=20
 		put_obj(obj);
 	}