| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4629754.LvFx2qVVIh@watership>
Date: Mon, 13 May 2024 23:15:42 -0500
From: Elizabeth Figura <zfigura@...eweavers.com>
To: Peter Zijlstra <peterz@...radead.org>
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 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);
}
Powered by blists - more mailing lists