Linux userland API discussions
 help / color / mirror / Atom feed
* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-21 13:35 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240221125525.GA16773@redhat.com>

On Wed, Feb 21, 2024 at 01:55:26PM +0100, Oleg Nesterov wrote:
> On 02/21, Christian Brauner wrote:
> >
> > On Tue, Feb 20, 2024 at 05:22:02PM +0100, Oleg Nesterov wrote:
> > >
> > > > > > +       /* Currently unused. */
> > > > > > +       if (info)
> > > > > > +               return -EINVAL;
> > > > >
> > > > > Well, to me this looks like the unnecessary restriction... And why?
> > > >
> > > > Because right now we aren't sure that it's used
> > >
> > > Yes, but...
> > >
> > > > and we aren't sure what use-cases are there.
> > >
> > > the same use-cases as for rt_sigqueueinfo() ?
> >
> > Specifically for pidfd_send_signal() I mean. To me it seems very
> > unlikely that anyone would be opening a pidfd to itself
> 
> Ah, with this, I do agree. And that is why (I think) we can remove
> the "task_pid(current) != pid" check in the "info != NULL" branch.

Ok, so let's try that. :)

^ permalink raw reply

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-21 12:55 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240221-zapfhahn-pulsschlag-e8f9d919c350@brauner>

On 02/21, Christian Brauner wrote:
>
> On Tue, Feb 20, 2024 at 05:22:02PM +0100, Oleg Nesterov wrote:
> >
> > > > > +       /* Currently unused. */
> > > > > +       if (info)
> > > > > +               return -EINVAL;
> > > >
> > > > Well, to me this looks like the unnecessary restriction... And why?
> > >
> > > Because right now we aren't sure that it's used
> >
> > Yes, but...
> >
> > > and we aren't sure what use-cases are there.
> >
> > the same use-cases as for rt_sigqueueinfo() ?
>
> Specifically for pidfd_send_signal() I mean. To me it seems very
> unlikely that anyone would be opening a pidfd to itself

Ah, with this, I do agree. And that is why (I think) we can remove
the "task_pid(current) != pid" check in the "info != NULL" branch.

Oleg.


^ permalink raw reply

* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Xi Ruoyao @ 2024-02-21 12:03 UTC (permalink / raw)
  To: WANG Xuerui, linux-api
  Cc: Arnd Bergmann, Christian Brauner, Kees Cook, Huacai Chen,
	Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
	Icenowy Zheng, loongarch@lists.linux.dev, linux-arch,
	Linux Kernel Mailing List
In-Reply-To: <f59d73fb-7d3e-4c55-821a-082032267978@xen0n.name>

On Wed, 2024-02-21 at 18:49 +0800, WANG Xuerui wrote:
> 
> On 2/21/24 18:31, Xi Ruoyao wrote:
> > On Wed, 2024-02-21 at 14:31 +0800, Xi Ruoyao wrote:
> > > On Wed, 2024-02-21 at 14:09 +0800, WANG Xuerui wrote:
> > > 
> > > > - just restore fstat and be done with it;
> > > > - add a flag to statx so we can do the equivalent of just fstat(fd,
> > > > &out) with statx, and ensuring an error happens if path is not empty in
> > > > that case;
> > > It's worse than "just restore fstat" considering the performance.  Read
> > > this thread:
> > > https://sourceware.org/pipermail/libc-alpha/2023-September/151320.html
> > Hmm, but it looks like statx already suffers the same performance issue.
> > And in this libc-alpha discussion Linus said:
> > 
> >     If the user asked for 'fstat()', just give the user 'fstat()'.
> >     
> > So to me we should just add fstat (and use it in Glibc for LoongArch, only
> > falling back to statx if fstat returns -ENOSYS), or am I missing something?
> 
> Or we could add a AT_STATX_NULL_PATH flag and mandate that `path` must
> be NULL if this flag is present -- then with simple checks we could have 
> statx(fd, NULL, AT_STATX_NULL_PATH, STATX_BASIC_STATS, &out) that's both 
> fstat-like and fast.

But then to take the advantage in Glibc fstat() implementation, we'll
need to try AT_STATX_NULL_PATH first, then check for EFAULT (not
ENOSYS!) and fall back to AT_EMPTY_PATH.  The EFAULT checking seems
nasty to me...

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

^ permalink raw reply

* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: WANG Xuerui @ 2024-02-21 10:49 UTC (permalink / raw)
  To: Xi Ruoyao, linux-api
  Cc: Arnd Bergmann, Christian Brauner, Kees Cook, Huacai Chen,
	Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
	Icenowy Zheng, loongarch@lists.linux.dev, linux-arch,
	Linux Kernel Mailing List
In-Reply-To: <2d25e3bb829cbca51387eb84985db919f50ccd37.camel@xry111.site>


On 2/21/24 18:31, Xi Ruoyao wrote:
> On Wed, 2024-02-21 at 14:31 +0800, Xi Ruoyao wrote:
>> On Wed, 2024-02-21 at 14:09 +0800, WANG Xuerui wrote:
>>
>>> - just restore fstat and be done with it;
>>> - add a flag to statx so we can do the equivalent of just fstat(fd,
>>> &out) with statx, and ensuring an error happens if path is not empty in
>>> that case;
>> It's worse than "just restore fstat" considering the performance.  Read
>> this thread:
>> https://sourceware.org/pipermail/libc-alpha/2023-September/151320.html
> Hmm, but it looks like statx already suffers the same performance issue.
> And in this libc-alpha discussion Linus said:
>
>     If the user asked for 'fstat()', just give the user 'fstat()'.
>     
> So to me we should just add fstat (and use it in Glibc for LoongArch, only
> falling back to statx if fstat returns -ENOSYS), or am I missing something?

Or we could add a AT_STATX_NULL_PATH flag and mandate that `path` must 
be NULL if this flag is present -- then with simple checks we could have 
statx(fd, NULL, AT_STATX_NULL_PATH, STATX_BASIC_STATS, &out) that's both 
fstat-like and fast.

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/


^ permalink raw reply

* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Xi Ruoyao @ 2024-02-21 10:31 UTC (permalink / raw)
  To: WANG Xuerui, linux-api
  Cc: Arnd Bergmann, Christian Brauner, Kees Cook, Huacai Chen,
	Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
	Icenowy Zheng, loongarch@lists.linux.dev, linux-arch,
	Linux Kernel Mailing List
In-Reply-To: <f1a1bc708be543eb647df57b5eb0c0ef035baf8b.camel@xry111.site>

On Wed, 2024-02-21 at 14:31 +0800, Xi Ruoyao wrote:
> On Wed, 2024-02-21 at 14:09 +0800, WANG Xuerui wrote:
> 
> > - just restore fstat and be done with it;
> > - add a flag to statx so we can do the equivalent of just fstat(fd, 
> > &out) with statx, and ensuring an error happens if path is not empty in 
> > that case;
> 
> It's worse than "just restore fstat" considering the performance.  Read
> this thread:
> https://sourceware.org/pipermail/libc-alpha/2023-September/151320.html

Hmm, but it looks like statx already suffers the same performance issue.
And in this libc-alpha discussion Linus said:

   If the user asked for 'fstat()', just give the user 'fstat()'.
   
So to me we should just add fstat (and use it in Glibc for LoongArch, only
falling back to statx if fstat returns -ENOSYS), or am I missing something?

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

^ permalink raw reply

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-21  7:42 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240220162201.GD7783@redhat.com>

On Tue, Feb 20, 2024 at 05:22:02PM +0100, Oleg Nesterov wrote:
> On 02/20, Christian Brauner wrote:
> >
> > On Tue, Feb 20, 2024 at 12:00:12PM +0100, Oleg Nesterov wrote:
> > >
> > > Perhaps we can kill the "task_pid(current) != pid" check and just return
> > > EPERM if "kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL", I don't think
> > > anobody needs pidfd_send_send_signal() to signal yourself. See below.
> >
> > Yeah.
> 
> You have my ack in advance
> 
> > > > +       /* Currently unused. */
> > > > +       if (info)
> > > > +               return -EINVAL;
> > >
> > > Well, to me this looks like the unnecessary restriction... And why?
> >
> > Because right now we aren't sure that it's used
> 
> Yes, but...
> 
> > and we aren't sure what use-cases are there.
> 
> the same use-cases as for rt_sigqueueinfo() ?

Specifically for pidfd_send_signal() I mean. To me it seems very
unlikely that anyone would be opening a pidfd to itself and then use
pidfd_send_signal() when they could entirely avoid this - race free - by
simply using *sigqueueinfo(). But I may be wrong of course.

> 
> Christian, I won't really argue but I still disagree.
> 
> Let me first repeat once again, I do not know what people do with pidfd
> and pidfd_send_signal() in particular, so I won't be surprised if this
> change won't cause any regression report.

Fwiw, that's fine as long as we'd fix it up.

> 
> But at the same time, I can easily imagine the following scenario: a
> userspace programmer tries to use pidfd_send_signal(info != NULL), gets
> -EINVAL, decides it can't/shouldn't work, and switches to sigqueueinfo()
> without any report to lkml.
> 
> > Yes, absolutely. That was always the plan. See appended patch I put on top.
> > I put you as author since you did spot this. Let me know if you don't
> > want that.
> 
> Ah. Thanks Christian. I am fine either way, whatever is more convenient
> for you.
> 
> But just in case, I won't mind at all if you simply fold this minor fix
> into your PIDFD_SEND_PROCESS_GROUP patch, I certainly don't care about
> the "From" tag ;)
> 
> A really, really minor/cosmetic nit below, feel free to ignore:
> 
> > -		if ((task_pid(current) != pid) &&
> > +		if (((task_pid(current) != pid) || type > PIDTYPE_TGID) &&
> 
> we can remove the unnecessary parens around "task_pid(current) != pid"
> or add the extra parens aroung "type > PIDTYPE_TGID".
> 
> I mean, the 1st operand of "&&" is
> 
> 	(task_pid(current) != pid) || type > PIDTYPE_TGID
> 
> and this looks a bit inconsistent to me.

Ok, will do.

^ permalink raw reply

* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Xi Ruoyao @ 2024-02-21  6:31 UTC (permalink / raw)
  To: WANG Xuerui, linux-api
  Cc: Arnd Bergmann, Christian Brauner, Kees Cook, Huacai Chen,
	Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
	Icenowy Zheng, loongarch@lists.linux.dev, linux-arch,
	Linux Kernel Mailing List
In-Reply-To: <599df4a3-47a4-49be-9c81-8e21ea1f988a@xen0n.name>

On Wed, 2024-02-21 at 14:09 +0800, WANG Xuerui wrote:

> - just restore fstat and be done with it;
> - add a flag to statx so we can do the equivalent of just fstat(fd, 
> &out) with statx, and ensuring an error happens if path is not empty in 
> that case;

It's worse than "just restore fstat" considering the performance.  Read
this thread:
https://sourceware.org/pipermail/libc-alpha/2023-September/151320.html

> - tackle the long-standing problem of seccomp deep argument inspection (!).

Frankly I'm never a fan of syscall blocklisting.  When I develop the
Online Judge system for the programming contest training in Xidian
University I deliberately avoid using seccomp.  This thing is very
likely to break innocent programs with some system change innocent as
well (for example Glibc or libstdc++ update).

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

^ permalink raw reply

* Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: WANG Xuerui @ 2024-02-21  6:09 UTC (permalink / raw)
  To: linux-api
  Cc: Arnd Bergmann, Christian Brauner, Kees Cook, Huacai Chen,
	Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
	Icenowy Zheng, loongarch@lists.linux.dev, linux-arch,
	Linux Kernel Mailing List

Hi,

Recently, we -- community LoongArch porters -- have noticed a problem 
where the Chromium sandbox apparently wants to deny statx [^1] so it 
could properly inspect arguments after the sandboxed process later falls 
back to fstat. The reasoning behind the change was not clear in the 
patch; but we found out it's basically because there's currently not a 
"fd-only" version of statx, so that the sandbox has no way to ensure the 
path argument is empty without being able to peek into the sandboxed 
process's memory. For architectures able to do newfstatat though, the 
glibc falls back to newfstatat after getting -ENOSYS for statx, then the 
respective SIGSYS handler [^2] takes care of inspecting the path 
argument, transforming allowed newfstatat's into fstat instead which is 
allowed and has the same type of return value.

But, as loongarch is the first architecture to not have fstat nor 
newfstatat, the LoongArch glibc does not attempt falling back at all 
when it gets -ENOSYS for statx -- and you see the problem there!

Actually, back when the loongarch port was under review, people were 
aware of the same problem with sandboxing clone3 [^3], so clone was 
eventually kept. Unfortunately it seemed at that time no one had noticed 
statx, so besides restoring fstat/newfstatat to loongarch uapi (and 
postponing the problem further), it seems inevitable that we would need 
to tackle seccomp deep argument inspection; this is obviously a decision 
that shouldn't be taken lightly, so I'm posting this to restart the 
conversation to figure out a way forward together. We basically could do 
one of below:

- just restore fstat and be done with it;
- add a flag to statx so we can do the equivalent of just fstat(fd, 
&out) with statx, and ensuring an error happens if path is not empty in 
that case;
- tackle the long-standing problem of seccomp deep argument inspection (!).

Obviously, the simplest solution would be to "just restore fstat". But 
again, in my opinion this is not quite a solution but a workaround -- we 
have good reasons to keep just statx (mainly because its feature set is 
a strict superset of those of fstat/newfstatat), and we're not quite in 
a hurry to resolve this. Because one of the prerequisites for a new 
Chromium port is "inclusion in Debian stable" -- as the loong64 port 
[^4] is progressing and the next Debian release would not happen until 
2025, we still have time for a more "elegant" solution.

Alternatively, we could also introduce a new flag for statx, maybe named 
AT_STATX_NO_PATH or something like that, so that statx would ignore the 
path altogether or error on non-empty paths if called with flags 
containing AT_EMPTY_PATH | AT_STATX_NO_PATH. This way a seccomp policy 
could allow statx calls only with such flags (that are passed via 
register and accessible) and maintain the same level of safety as with 
fstat. But there is also disadvantage to this approach: the flag would 
be useful only for sandboxes, because in other cases there's no need to 
avoid reading from &path. This is also more of a workaround to avoid the 
deep argument inspection problem.

Lastly, should we decide to go the hardest way, according to a previous 
mail [^5] (about clone3) and the LPC 2019 discussion [^6] [^7], we 
probably would try the metadata-annotation-based and piece-by-piece 
approach, as it's expected to provide the most benefit and involve less 
code changes. The implementation, as I surmise, will involve modifying 
the generic syscall entrypoint for early copying of user data, and 
corresponding changes to seccomp plumbing so this information is 
properly exposed. I don't have a roadmap for non-generic-entry arches 
right now, and I also haven't started designing the new seccomp ABI for 
that. As a matter of fact, members of the LoongArch community (myself 
included) are still fresh to this area of expertise, so a bit more of 
your feedback will be appreciated.

Thanks to Miao Wang from AOSC for doing much of the investigation.

[^1]: https://chromium-review.googlesource.com/c/chromium/src/+/2823150
[^2]: 
https://chromium.googlesource.com/chromium/src/sandbox/+/c085b51940bd/linux/seccomp-bpf-helpers/sigsys_handlers.cc#355
[^3]: 
https://lore.kernel.org/linux-arch/20220511211231.GG7074@brightrain.aerifal.cx/
[^4]: https://wiki.debian.org/Ports/loong64
[^5]: https://lwn.net/ml/linux-kernel/201905301122.88FD40B3@keescook/
[^6]: https://lwn.net/Articles/799557/
[^7]: 
https://lpc.events/event/4/contributions/560/attachments/397/640/deep-arg-inspection.pdf

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list:https://lore.kernel.org/loongarch/


^ permalink raw reply

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-20 16:22 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240220-anlegen-feinmechaniker-3c2cfcc3ec01@brauner>

On 02/20, Christian Brauner wrote:
>
> On Tue, Feb 20, 2024 at 12:00:12PM +0100, Oleg Nesterov wrote:
> >
> > Perhaps we can kill the "task_pid(current) != pid" check and just return
> > EPERM if "kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL", I don't think
> > anobody needs pidfd_send_send_signal() to signal yourself. See below.
>
> Yeah.

You have my ack in advance

> > > +       /* Currently unused. */
> > > +       if (info)
> > > +               return -EINVAL;
> >
> > Well, to me this looks like the unnecessary restriction... And why?
>
> Because right now we aren't sure that it's used

Yes, but...

> and we aren't sure what use-cases are there.

the same use-cases as for rt_sigqueueinfo() ?

Christian, I won't really argue but I still disagree.

Let me first repeat once again, I do not know what people do with pidfd
and pidfd_send_signal() in particular, so I won't be surprised if this
change won't cause any regression report.

But at the same time, I can easily imagine the following scenario: a
userspace programmer tries to use pidfd_send_signal(info != NULL), gets
-EINVAL, decides it can't/shouldn't work, and switches to sigqueueinfo()
without any report to lkml.

> Yes, absolutely. That was always the plan. See appended patch I put on top.
> I put you as author since you did spot this. Let me know if you don't
> want that.

Ah. Thanks Christian. I am fine either way, whatever is more convenient
for you.

But just in case, I won't mind at all if you simply fold this minor fix
into your PIDFD_SEND_PROCESS_GROUP patch, I certainly don't care about
the "From" tag ;)

A really, really minor/cosmetic nit below, feel free to ignore:

> -		if ((task_pid(current) != pid) &&
> +		if (((task_pid(current) != pid) || type > PIDTYPE_TGID) &&

we can remove the unnecessary parens around "task_pid(current) != pid"
or add the extra parens aroung "type > PIDTYPE_TGID".

I mean, the 1st operand of "&&" is

	(task_pid(current) != pid) || type > PIDTYPE_TGID

and this looks a bit inconsistent to me.

Oleg.


^ permalink raw reply

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-20 12:59 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240220110012.GB7783@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

On Tue, Feb 20, 2024 at 12:00:12PM +0100, Oleg Nesterov wrote:
> On 02/20, Christian Brauner wrote:
> >
> > On Tue, Feb 20, 2024 at 10:02:56AM +0100, Oleg Nesterov wrote:
> > >
> > > Ah. IIRC criu uses this hack to restore the pending (arbitrary) signals
> > > collected at dump time.
> > >
> > > I was a bit surprise sys_pidfd_send_signal() allows this hack too, I don't
> >
> > I think that we simply mirrored the restrictions in the other system
> > calls.
> >
> > > think that criu uses pidfd at restore time, but I do not know.
> >
> > Hm, I just checked and it doesn't use pidfd_send_signal(). It uses
> > pidfds but only for pid reuse detection for RPC clients.
> 
> But perhaps something else already uses pidfd_send_signal() with info != NULL
> or with info->si_code == SI_USER, we can't know. Please see below.
> 
> > So right now si_code is blocked for >= 0 and for SI_TKILL. If we were to
> > simply ensure that si_code can't be < 0 then this amounts to effectively
> > blocking @info from being filled in by userspace at all. Because 0 is a
> > valid value.
> 
> I'am afraid I misunderstand you again... 0 == SI_USER is not a valid value
> when siginfo != NULL.

Yes, I know. We're on the same page. I would just have preferred to
restrict remulating si_code completely because we don't know whether
that's actually used for pidfd_send_signal(). The point I was trying to
make is that si_code has no value that means "unset" so restricting
si_code further means always rejecting @info when it's passed.

> 
> Perhaps we can kill the "task_pid(current) != pid" check and just return
> EPERM if "kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL", I don't think
> anobody needs pidfd_send_send_signal() to signal yourself. See below.

Yeah.

> 
> > +       /* Currently unused. */
> > +       if (info)
> > +               return -EINVAL;
> 
> Well, to me this looks like the unnecessary restriction... And why?

Because right now we aren't sure that it's used and we aren't sure what
use-cases are there.

> 
> But whatever we do,
> 
> > -               /* Only allow sending arbitrary signals to yourself. */
> > -               ret = -EPERM;
> > -               if ((task_pid(current) != pid) &&
> > -                   (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
> > -                       goto err;
> 
> Can I suggest to fix this check in your tree (add type > PIDTYPE_TGID as
> we discussed) first, then do other changes on top?

Yes, absolutely. That was always the plan. See appended patch I put on top.
I put you as author since you did spot this. Let me know if you don't
want that.

[-- Attachment #2: 0001-signal-adjust-si_code-restriction-in-pidfd_send_sign.patch --]
[-- Type: text/x-diff, Size: 1207 bytes --]

From 67a1a77630c00f457a46e1164caf0d32c0edc127 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Tue, 20 Feb 2024 13:53:00 +0100
Subject: [PATCH] signal: adjust si_code restriction in pidfd_send_signal()

Since we now allow specifying PIDFD_SEND_PROCESS_GROUP for
pidfd_send_signal() to send signals to process groups we need to adjust
the check restricting si_code emulation by userspace to account for
PIDTYPE_PGID.

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Link: https://lore.kernel.org/r/20240214123655.GB16265@redhat.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index cf6539a6b1cb..5f5620c81d3a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3956,7 +3956,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
 
 		/* Only allow sending arbitrary signals to yourself. */
 		ret = -EPERM;
-		if ((task_pid(current) != pid) &&
+		if (((task_pid(current) != pid) || type > PIDTYPE_TGID) &&
 		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
 			goto err;
 	} else {
-- 
2.43.0


^ permalink raw reply related

* Re: [PATCH v2] uapi/auxvec: Define AT_HWCAP3 and AT_HWCAP4 aux vector, entries
From: Michael Ellerman @ 2024-02-20 12:53 UTC (permalink / raw)
  To: linux-api, linux-arch, linux-kernel, linuxppc-dev, Peter Bergner
  Cc: Adhemerval Zanella, Szabolcs Nagy, Nick Piggin, Arnd Bergmann
In-Reply-To: <a406b535-dc55-4856-8ae9-5a063644a1af@linux.ibm.com>

On Wed, 14 Feb 2024 16:34:06 -0600, Peter Bergner wrote:
> Changes from v1:
> - Add Acked-by lines.
> 
> The powerpc toolchain keeps a copy of the HWCAP bit masks in our TCB for fast
> access by the __builtin_cpu_supports built-in function.  The TCB space for
> the HWCAP entries - which are created in pairs - is an ABI extension, so
> waiting to create the space for HWCAP3 and HWCAP4 until we need them is
> problematical.  Define AT_HWCAP3 and AT_HWCAP4 in the generic uapi header
> so they can be used in glibc to reserve space in the powerpc TCB for their
> future use.
> 
> [...]

Applied to powerpc/next.

[1/1] uapi/auxvec: Define AT_HWCAP3 and AT_HWCAP4 aux vector, entries
      https://git.kernel.org/powerpc/c/3281366a8e79a512956382885091565db1036b64

cheers

^ permalink raw reply

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-20 11:00 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240220-pragmatisch-parzelle-8a1d10a94fae@brauner>

On 02/20, Christian Brauner wrote:
>
> On Tue, Feb 20, 2024 at 10:02:56AM +0100, Oleg Nesterov wrote:
> >
> > Ah. IIRC criu uses this hack to restore the pending (arbitrary) signals
> > collected at dump time.
> >
> > I was a bit surprise sys_pidfd_send_signal() allows this hack too, I don't
>
> I think that we simply mirrored the restrictions in the other system
> calls.
>
> > think that criu uses pidfd at restore time, but I do not know.
>
> Hm, I just checked and it doesn't use pidfd_send_signal(). It uses
> pidfds but only for pid reuse detection for RPC clients.

But perhaps something else already uses pidfd_send_signal() with info != NULL
or with info->si_code == SI_USER, we can't know. Please see below.

> So right now si_code is blocked for >= 0 and for SI_TKILL. If we were to
> simply ensure that si_code can't be < 0 then this amounts to effectively
> blocking @info from being filled in by userspace at all. Because 0 is a
> valid value.

I'am afraid I misunderstand you again... 0 == SI_USER is not a valid value
when siginfo != NULL.

Perhaps we can kill the "task_pid(current) != pid" check and just return
EPERM if "kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL", I don't think
anobody needs pidfd_send_send_signal() to signal yourself. See below.

> +       /* Currently unused. */
> +       if (info)
> +               return -EINVAL;

Well, to me this looks like the unnecessary restriction... And why?

But whatever we do,

> -               /* Only allow sending arbitrary signals to yourself. */
> -               ret = -EPERM;
> -               if ((task_pid(current) != pid) &&
> -                   (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
> -                       goto err;

Can I suggest to fix this check in your tree (add type > PIDTYPE_TGID as
we discussed) first, then do other changes on top?

This way we can revert the next change(s) if we get regressions reports
without re-introducing the security problem.

Oleg.


^ permalink raw reply

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-20  9:22 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240220090255.GA7783@redhat.com>

On Tue, Feb 20, 2024 at 10:02:56AM +0100, Oleg Nesterov wrote:
> On 02/20, Christian Brauner wrote:
> >
> > On Fri, Feb 16, 2024 at 07:12:14PM +0100, Oleg Nesterov wrote:
> > > On 02/16, Christian Brauner wrote:
> > > >
> > > > > SI_USER means that the target can trust the values of si_pid/si_uid
> > > > > in siginfo.
> > > >
> > > > Bah, what an annoying nonsense. I see that this can be used to emulate
> > > > stuff like SI_TIMER and SI_ASYNCIO. But I very much doubt the value of
> > > > e.g., emulating SI_DETHREAD. Maybe I'm missing something very obvious.
> > >
> > > I don't understand...
> >
> > My question was what the purpose of being able to to set si_code to
> > e.g., SI_DETHREAD is and then to send a signal to yourself? Because it
> > looks like that's what rt_{tg}sigqueueinfo() and pidfd_send_signal()
> > allows the caller to do. I'm just trying to understand use-cases for
> > this.
> 
> Ah. IIRC criu uses this hack to restore the pending (arbitrary) signals
> collected at dump time.
> 
> I was a bit surprise sys_pidfd_send_signal() allows this hack too, I don't

I think that we simply mirrored the restrictions in the other system
calls.

> think that criu uses pidfd at restore time, but I do not know.

Hm, I just checked and it doesn't use pidfd_send_signal(). It uses
pidfds but only for pid reuse detection for RPC clients.

So right now si_code is blocked for >= 0 and for SI_TKILL. If we were to
simply ensure that si_code can't be < 0 then this amounts to effectively
blocking @info from being filled in by userspace at all. Because 0 is a
valid value.

So could we just _try_ and either ignore the @info argument completely
or consistenly report EINVAL when @info is non-NULL and see if anyone
reports a regression? If there ever is a valid use-case then we can just
add a flag argument PIDFD_SIGNAL_INFO to indicate that @info should be
taken into account.

So something like the completely untested?

diff --git a/kernel/signal.c b/kernel/signal.c
index cf6539a6b1cb..2cca42175480 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3849,22 +3849,6 @@ static bool access_pidfd_pidns(struct pid *pid)
        return true;
 }

-static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo,
-               siginfo_t __user *info)
-{
-#ifdef CONFIG_COMPAT
-       /*
-        * Avoid hooking up compat syscalls and instead handle necessary
-        * conversions here. Note, this is a stop-gap measure and should not be
-        * considered a generic solution.
-        */
-       if (in_compat_syscall())
-               return copy_siginfo_from_user32(
-                       kinfo, (struct compat_siginfo __user *)info);
-#endif
-       return copy_siginfo_from_user(kinfo, info);
-}
-
 static struct pid *pidfd_to_pid(const struct file *file)
 {
        struct pid *pid;
@@ -3911,6 +3895,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
        if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
                return -EINVAL;

+       /* Currently unused. */
+       if (info)
+               return -EINVAL;
+
        f = fdget(pidfd);
        if (!f.file)
                return -EBADF;
@@ -3945,23 +3933,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
                break;
        }

-       if (info) {
-               ret = copy_siginfo_from_user_any(&kinfo, info);
-               if (unlikely(ret))
-                       goto err;
-
-               ret = -EINVAL;
-               if (unlikely(sig != kinfo.si_signo))
-                       goto err;
-
-               /* Only allow sending arbitrary signals to yourself. */
-               ret = -EPERM;
-               if ((task_pid(current) != pid) &&
-                   (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
-                       goto err;
-       } else {
-               prepare_kill_siginfo(sig, &kinfo, type);
-       }
+       prepare_kill_siginfo(sig, &kinfo, type);

        if (type == PIDTYPE_PGID)
                ret = kill_pgrp_info(sig, &kinfo, pid);


^ permalink raw reply related

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Oleg Nesterov @ 2024-02-20  9:02 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240220-einwurf-depesche-d8682be0370c@brauner>

On 02/20, Christian Brauner wrote:
>
> On Fri, Feb 16, 2024 at 07:12:14PM +0100, Oleg Nesterov wrote:
> > On 02/16, Christian Brauner wrote:
> > >
> > > > SI_USER means that the target can trust the values of si_pid/si_uid
> > > > in siginfo.
> > >
> > > Bah, what an annoying nonsense. I see that this can be used to emulate
> > > stuff like SI_TIMER and SI_ASYNCIO. But I very much doubt the value of
> > > e.g., emulating SI_DETHREAD. Maybe I'm missing something very obvious.
> >
> > I don't understand...
>
> My question was what the purpose of being able to to set si_code to
> e.g., SI_DETHREAD is and then to send a signal to yourself? Because it
> looks like that's what rt_{tg}sigqueueinfo() and pidfd_send_signal()
> allows the caller to do. I'm just trying to understand use-cases for
> this.

Ah. IIRC criu uses this hack to restore the pending (arbitrary) signals
collected at dump time.

I was a bit surprise sys_pidfd_send_signal() allows this hack too, I don't
think that criu uses pidfd at restore time, but I do not know.

Oleg.


^ permalink raw reply

* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-20  8:34 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
	linux-kernel
In-Reply-To: <20240216181214.GA10393@redhat.com>

On Fri, Feb 16, 2024 at 07:12:14PM +0100, Oleg Nesterov wrote:
> On 02/16, Christian Brauner wrote:
> >
> > > SI_USER means that the target can trust the values of si_pid/si_uid
> > > in siginfo.
> >
> > Bah, what an annoying nonsense. I see that this can be used to emulate
> > stuff like SI_TIMER and SI_ASYNCIO. But I very much doubt the value of
> > e.g., emulating SI_DETHREAD. Maybe I'm missing something very obvious.
> 
> I don't understand...

My question was what the purpose of being able to to set si_code to
e.g., SI_DETHREAD is and then to send a signal to yourself? Because it
looks like that's what rt_{tg}sigqueueinfo() and pidfd_send_signal()
allows the caller to do. I'm just trying to understand use-cases for
this.

^ permalink raw reply

* Re: [PATCH v2 17/31] ntsync: Allow waits to use the REALTIME clock.
From: Arnd Bergmann @ 2024-02-20  7:01 UTC (permalink / raw)
  To: Elizabeth Figura, Greg Kroah-Hartman, Jonathan Corbet, shuah
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap
In-Reply-To: <20240219223833.95710-18-zfigura@codeweavers.com>

On Mon, Feb 19, 2024, at 23:38, Elizabeth Figura wrote:
> NtWaitForMultipleObjects() can receive a timeout in two forms, relative or
> absolute. Relative timeouts are unaffected by changes to the system time and do
> not count down while the system suspends; for absolute timeouts the opposite is
> true.
>
> In order to make the interface and implementation simpler, the ntsync driver
> only deals in absolute timeouts. However, we need to be able to emulate both
> behaviours apropos suspension and time adjustment, which is achieved by allowing
> either the MONOTONIC or REALTIME clock to be used.
>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>

I understand that there is no practical problem in building
up the API one patch at a time in the initial merge, but
it still feels wrong to have an incompatible ABI change in
the middle of the series:

> @@ -35,6 +37,8 @@ struct ntsync_wait_args {
>  	__u32 owner;
>  	__u32 index;
>  	__u32 alert;
> +	__u32 flags;
> +	__u32 pad;
>  };

If this was patch to get merged at any later point, you'd have
to support both the shorter and the longer structure layout
with their distinct ioctl command codes.

If you do a v3 series, maybe just merge this patch into the
one that introduces the struct ntsync_wait_args. Overall,
you could probably have fewer but larger patches anyway
without harming the review process, but other than this
one that is not a problem.

       Arnd

^ permalink raw reply

* [PATCH v2 31/31] docs: ntsync: Add documentation for the ntsync uAPI.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Add an overall explanation of the driver architecture, and complete and precise
specification for its intended behaviour.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 Documentation/userspace-api/index.rst  |   1 +
 Documentation/userspace-api/ntsync.rst | 399 +++++++++++++++++++++++++
 2 files changed, 400 insertions(+)
 create mode 100644 Documentation/userspace-api/ntsync.rst

diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index 09f61bd2ac2e..f5a72ed27def 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -34,6 +34,7 @@ place where this information is gathered.
    tee
    isapnp
    dcdbas
+   ntsync
 
 .. only::  subproject and html
 
diff --git a/Documentation/userspace-api/ntsync.rst b/Documentation/userspace-api/ntsync.rst
new file mode 100644
index 000000000000..202c2350d3af
--- /dev/null
+++ b/Documentation/userspace-api/ntsync.rst
@@ -0,0 +1,399 @@
+===================================
+NT synchronization primitive driver
+===================================
+
+This page documents the user-space API for the ntsync driver.
+
+ntsync is a support driver for emulation of NT synchronization
+primitives by user-space NT emulators. It exists because implementation
+in user-space, using existing tools, cannot match Windows performance
+while offering accurate semantics. It is implemented entirely in
+software, and does not drive any hardware device.
+
+This interface is meant as a compatibility tool only, and should not
+be used for general synchronization. Instead use generic, versatile
+interfaces such as futex(2) and poll(2).
+
+Synchronization primitives
+==========================
+
+The ntsync driver exposes three types of synchronization primitives:
+semaphores, mutexes, and events.
+
+A semaphore holds a single volatile 32-bit counter, and a static 32-bit
+integer denoting the maximum value. It is considered signaled when the
+counter is nonzero. The counter is decremented by one when a wait is
+satisfied. Both the initial and maximum count are established when the
+semaphore is created.
+
+A mutex holds a volatile 32-bit recursion count, and a volatile 32-bit
+identifier denoting its owner. A mutex is considered signaled when its
+owner is zero (indicating that it is not owned). The recursion count is
+incremented when a wait is satisfied, and ownership is set to the given
+identifier.
+
+A mutex also holds an internal flag denoting whether its previous owner
+has died; such a mutex is said to be abandoned. Owner death is not
+tracked automatically based on thread death, but rather must be
+communicated using ``NTSYNC_IOC_MUTEX_KILL``. An abandoned mutex is
+inherently considered unowned.
+
+Except for the "unowned" semantics of zero, the actual value of the
+owner identifier is not interpreted by the ntsync driver at all. The
+intended use is to store a thread identifier; however, the ntsync
+driver does not actually validate that a calling thread provides
+consistent or unique identifiers.
+
+An event holds a volatile boolean state denoting whether it is signaled
+or not. There are two types of events, auto-reset and manual-reset. An
+auto-reset event is designaled when a wait is satisfied; a manual-reset
+event is not. The event type is specified when the event is created.
+
+Unless specified otherwise, all operations on an object are atomic and
+totally ordered with respect to other operations on the same object.
+
+Objects are represented by files. When all file descriptors to an
+object are closed, that object is deleted.
+
+Char device
+===========
+
+The ntsync driver creates a single char device /dev/ntsync. Each file
+description opened on the device represents a unique instance intended
+to back an individual NT virtual machine. Objects created by one ntsync
+instance may only be used with other objects created by the same
+instance.
+
+ioctl reference
+===============
+
+All operations on the device are done through ioctls. There are four
+structures used in ioctl calls::
+
+   struct ntsync_sem_args {
+   	__u32 sem;
+   	__u32 count;
+   	__u32 max;
+   };
+
+   struct ntsync_mutex_args {
+   	__u32 mutex;
+   	__u32 owner;
+   	__u32 count;
+   };
+
+   struct ntsync_event_args {
+   	__u32 event;
+   	__u32 signaled;
+   	__u32 manual;
+   };
+
+   struct ntsync_wait_args {
+   	__u64 timeout;
+   	__u64 objs;
+   	__u32 count;
+   	__u32 owner;
+   	__u32 index;
+   	__u32 alert;
+   	__u32 flags;
+   	__u32 pad;
+   };
+
+Depending on the ioctl, members of the structure may be used as input,
+output, or not at all. All ioctls return 0 on success.
+
+The ioctls on the device file are as follows:
+
+.. c:macro:: NTSYNC_IOC_CREATE_SEM
+
+  Create a semaphore object. Takes a pointer to struct
+  :c:type:`ntsync_sem_args`, which is used as follows:
+
+  .. list-table::
+
+     * - ``sem``
+       - On output, contains a file descriptor to the created semaphore.
+     * - ``count``
+       - Initial count of the semaphore.
+     * - ``max``
+       - Maximum count of the semaphore.
+
+  Fails with ``EINVAL`` if ``count`` is greater than ``max``.
+
+.. c:macro:: NTSYNC_IOC_CREATE_MUTEX
+
+  Create a mutex object. Takes a pointer to struct
+  :c:type:`ntsync_mutex_args`, which is used as follows:
+
+  .. list-table::
+
+     * - ``mutex``
+       - On output, contains a file descriptor to the created mutex.
+     * - ``count``
+       - Initial recursion count of the mutex.
+     * - ``owner``
+       - Initial owner of the mutex.
+
+  If ``owner`` is nonzero and ``count`` is zero, or if ``owner`` is
+  zero and ``count`` is nonzero, the function fails with ``EINVAL``.
+
+.. c:macro:: NTSYNC_IOC_CREATE_EVENT
+
+  Create an event object. Takes a pointer to struct
+  :c:type:`ntsync_event_args`, which is used as follows:
+
+  .. list-table::
+
+     * - ``event``
+       - On output, contains a file descriptor to the created event.
+     * - ``signaled``
+       - If nonzero, the event is initially signaled, otherwise
+         nonsignaled.
+     * - ``manual``
+       - If nonzero, the event is a manual-reset event, otherwise
+         auto-reset.
+
+The ioctls on the individual objects are as follows:
+
+.. c:macro:: NTSYNC_IOC_SEM_POST
+
+  Post to a semaphore object. Takes a pointer to a 32-bit integer,
+  which on input holds the count to be added to the semaphore, and on
+  output contains its previous count.
+
+  If adding to the semaphore's current count would raise the latter
+  past the semaphore's maximum count, the ioctl fails with
+  ``EOVERFLOW`` and the semaphore is not affected. If raising the
+  semaphore's count causes it to become signaled, eligible threads
+  waiting on this semaphore will be woken and the semaphore's count
+  decremented appropriately.
+
+.. c:macro:: NTSYNC_IOC_MUTEX_UNLOCK
+
+  Release a mutex object. Takes a pointer to struct
+  :c:type:`ntsync_mutex_args`, which is used as follows:
+
+  .. list-table::
+
+     * - ``mutex``
+       - Ignored.
+     * - ``owner``
+       - Specifies the owner trying to release this mutex.
+     * - ``count``
+       - On output, contains the previous recursion count.
+
+  If ``owner`` is zero, the ioctl fails with ``EINVAL``. If ``owner``
+  is not the current owner of the mutex, the ioctl fails with
+  ``EPERM``.
+
+  The mutex's count will be decremented by one. If decrementing the
+  mutex's count causes it to become zero, the mutex is marked as
+  unowned and signaled, and eligible threads waiting on it will be
+  woken as appropriate.
+
+.. c:macro:: NTSYNC_IOC_SET_EVENT
+
+  Signal an event object. Takes a pointer to a 32-bit integer, which on
+  output contains the previous state of the event.
+
+  Eligible threads will be woken, and auto-reset events will be
+  designaled appropriately.
+
+.. c:macro:: NTSYNC_IOC_RESET_EVENT
+
+  Designal an event object. Takes a pointer to a 32-bit integer, which
+  on output contains the previous state of the event.
+
+.. c:macro:: NTSYNC_IOC_PULSE_EVENT
+
+  Wake threads waiting on an event object while leaving it in an
+  unsignaled state. Takes a pointer to a 32-bit integer, which on
+  output contains the previous state of the event.
+
+  A pulse operation can be thought of as a set followed by a reset,
+  performed as a single atomic operation. If two threads are waiting on
+  an auto-reset event which is pulsed, only one will be woken. If two
+  threads are waiting a manual-reset event which is pulsed, both will
+  be woken. However, in both cases, the event will be unsignaled
+  afterwards, and a simultaneous read operation will always report the
+  event as unsignaled.
+
+.. c:macro:: NTSYNC_IOC_READ_SEM
+
+  Read the current state of a semaphore object. Takes a pointer to
+  struct :c:type:`ntsync_sem_args`, which is used as follows:
+
+  .. list-table::
+
+     * - ``sem``
+       - Ignored.
+     * - ``count``
+       - On output, contains the current count of the semaphore.
+     * - ``max``
+       - On output, contains the maximum count of the semaphore.
+
+.. c:macro:: NTSYNC_IOC_READ_MUTEX
+
+  Read the current state of a mutex object. Takes a pointer to struct
+  :c:type:`ntsync_mutex_args`, which is used as follows:
+
+  .. list-table::
+
+     * - ``mutex``
+       - Ignored.
+     * - ``owner``
+       - On output, contains the current owner of the mutex, or zero
+         if the mutex is not currently owned.
+     * - ``count``
+       - On output, contains the current recursion count of the mutex.
+
+  If the mutex is marked as abandoned, the function fails with
+  ``EOWNERDEAD``. In this case, ``count`` and ``owner`` are set to
+  zero.
+
+.. c:macro:: NTSYNC_IOC_READ_EVENT
+
+  Read the current state of an event object. Takes a pointer to struct
+  :c:type:`ntsync_event_args`, which is used as follows:
+
+  .. list-table::
+
+     * - ``event``
+       - Ignored.
+     * - ``signaled``
+       - On output, contains the current state of the event.
+     * - ``manual``
+       - On output, contains 1 if the event is a manual-reset event,
+         and 0 otherwise.
+
+.. c:macro:: NTSYNC_IOC_KILL_OWNER
+
+  Mark a mutex as unowned and abandoned if it is owned by the given
+  owner. Takes an input-only pointer to a 32-bit integer denoting the
+  owner. If the owner is zero, the ioctl fails with ``EINVAL``. If the
+  owner does not own the mutex, the function fails with ``EPERM``.
+
+  Eligible threads waiting on the mutex will be woken as appropriate
+  (and such waits will fail with ``EOWNERDEAD``, as described below).
+
+.. c:macro:: NTSYNC_IOC_WAIT_ANY
+
+  Poll on any of a list of objects, atomically acquiring at most one.
+  Takes a pointer to struct :c:type:`ntsync_wait_args`, which is
+  used as follows:
+
+  .. list-table::
+
+     * - ``timeout``
+       - Absolute timeout in nanoseconds. If ``NTSYNC_WAIT_REALTIME``
+         is set, the timeout is measured against the REALTIME clock;
+         otherwise it is measured against the MONOTONIC clock. If the
+         timeout is equal to or earlier than the current time, the
+         function returns immediately without sleeping. If ``timeout``
+         is U64_MAX, the function will sleep until an object is
+         signaled, and will not fail with ``ETIMEDOUT``.
+     * - ``objs``
+       - Pointer to an array of ``count`` file descriptors
+         (specified as an integer so that the structure has the same
+         size regardless of architecture). If any object is
+         invalid, the function fails with ``EINVAL``.
+     * - ``count``
+       - Number of objects specified in the ``objs`` array.
+         If greater than ``NTSYNC_MAX_WAIT_COUNT``, the function fails
+         with ``EINVAL``.
+     * - ``owner``
+       - Mutex owner identifier. If any object in ``objs`` is a mutex,
+         the ioctl will attempt to acquire that mutex on behalf of
+         ``owner``. If ``owner`` is zero, the ioctl fails with
+         ``EINVAL``.
+     * - ``index``
+       - On success, contains the index (into ``objs``) of the object
+         which was signaled. If ``alert`` was signaled instead,
+         this contains ``count``.
+     * - ``alert``
+       - Optional event object file descriptor. If nonzero, this
+         specifies an "alert" event object which, if signaled, will
+         terminate the wait. If nonzero, the identifier must point to a
+         valid event.
+     * - ``flags``
+       - Zero or more flags. Currently the only flag is
+         ``NTSYNC_WAIT_REALTIME``, which causes the timeout to be
+         measured against the REALTIME clock instead of MONOTONIC.
+     * - ``pad``
+       - Unused, must be set to zero.
+
+  This function attempts to acquire one of the given objects. If unable
+  to do so, it sleeps until an object becomes signaled, subsequently
+  acquiring it, or the timeout expires. In the latter case the ioctl
+  fails with ``ETIMEDOUT``. The function only acquires one object, even
+  if multiple objects are signaled.
+
+  A semaphore is considered to be signaled if its count is nonzero, and
+  is acquired by decrementing its count by one. A mutex is considered
+  to be signaled if it is unowned or if its owner matches the ``owner``
+  argument, and is acquired by incrementing its recursion count by one
+  and setting its owner to the ``owner`` argument. An auto-reset event
+  is acquired by designaling it; a manual-reset event is not affected
+  by acquisition.
+
+  Acquisition is atomic and totally ordered with respect to other
+  operations on the same object. If two wait operations (with different
+  ``owner`` identifiers) are queued on the same mutex, only one is
+  signaled. If two wait operations are queued on the same semaphore,
+  and a value of one is posted to it, only one is signaled. The order
+  in which threads are signaled is not specified.
+
+  If an abandoned mutex is acquired, the ioctl fails with
+  ``EOWNERDEAD``. Although this is a failure return, the function may
+  otherwise be considered successful. The mutex is marked as owned by
+  the given owner (with a recursion count of 1) and as no longer
+  abandoned, and ``index`` is still set to the index of the mutex.
+
+  The ``alert`` argument is an "extra" event which can terminate the
+  wait, independently of all other objects. If members of ``objs`` and
+  ``alert`` are both simultaneously signaled, a member of ``objs`` will
+  always be given priority and acquired first.
+
+  It is valid to pass the same object more than once, including by
+  passing the same event in the ``objs`` array and in ``alert``. If a
+  wakeup occurs due to that object being signaled, ``index`` is set to
+  the lowest index corresponding to that object.
+
+  The function may fail with ``EINTR`` if a signal is received.
+
+.. c:macro:: NTSYNC_IOC_WAIT_ALL
+
+  Poll on a list of objects, atomically acquiring all of them. Takes a
+  pointer to struct :c:type:`ntsync_wait_args`, which is used
+  identically to ``NTSYNC_IOC_WAIT_ANY``, except that ``index`` is
+  always filled with zero on success if not woken via alert.
+
+  This function attempts to simultaneously acquire all of the given
+  objects. If unable to do so, it sleeps until all objects become
+  simultaneously signaled, subsequently acquiring them, or the timeout
+  expires. In the latter case the ioctl fails with ``ETIMEDOUT`` and no
+  objects are modified.
+
+  Objects may become signaled and subsequently designaled (through
+  acquisition by other threads) while this thread is sleeping. Only
+  once all objects are simultaneously signaled does the ioctl acquire
+  them and return. The entire acquisition is atomic and totally ordered
+  with respect to other operations on any of the given objects.
+
+  If an abandoned mutex is acquired, the ioctl fails with
+  ``EOWNERDEAD``. Similarly to ``NTSYNC_IOC_WAIT_ANY``, all objects are
+  nevertheless marked as acquired. Note that if multiple mutex objects
+  are specified, there is no way to know which were marked as
+  abandoned.
+
+  As with "any" waits, the ``alert`` argument is an "extra" event which
+  can terminate the wait. Critically, however, an "all" wait will
+  succeed if all members in ``objs`` are signaled, *or* if ``alert`` is
+  signaled. In the latter case ``index`` will be set to ``count``. As
+  with "any" waits, if both conditions are filled, the former takes
+  priority, and objects in ``objs`` will be acquired.
+
+  Unlike ``NTSYNC_IOC_WAIT_ANY``, it is not valid to pass the same
+  object more than once, nor is it valid to pass the same object in
+  ``objs`` and in ``alert``. If this is attempted, the function fails
+  with ``EINVAL``.
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 29/31] selftests: ntsync: Add a stress test for contended waits.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Test a more realistic usage pattern, and one with heavy contention, in order to
actually exercise ntsync's internal synchronization.

This test has several threads in a tight loop acquiring a mutex, modifying some
shared data, and then releasing the mutex. At the end we check if the data is
consistent.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 .../testing/selftests/drivers/ntsync/ntsync.c | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 968874d7e325..5fa2c9a0768c 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -1330,4 +1330,78 @@ TEST(alert_all)
 	close(fd);
 }
 
+#define STRESS_LOOPS 10000
+#define STRESS_THREADS 4
+
+static unsigned int stress_counter;
+static int stress_device, stress_start_event, stress_mutex;
+
+static void *stress_thread(void *arg)
+{
+	struct ntsync_wait_args wait_args = {0};
+	__u32 index, count, i;
+	int ret;
+
+	wait_args.timeout = UINT64_MAX;
+	wait_args.count = 1;
+	wait_args.objs = (uintptr_t)&stress_start_event;
+	wait_args.owner = gettid();
+	wait_args.index = 0xdeadbeef;
+
+	ioctl(stress_device, NTSYNC_IOC_WAIT_ANY, &wait_args);
+
+	wait_args.objs = (uintptr_t)&stress_mutex;
+
+	for (i = 0; i < STRESS_LOOPS; ++i) {
+		ioctl(stress_device, NTSYNC_IOC_WAIT_ANY, &wait_args);
+
+		++stress_counter;
+
+		unlock_mutex(stress_mutex, wait_args.owner, &count);
+	}
+
+	return NULL;
+}
+
+TEST(stress_wait)
+{
+	struct ntsync_event_args event_args;
+	struct ntsync_mutex_args mutex_args;
+	pthread_t threads[STRESS_THREADS];
+	__u32 signaled, i;
+	int ret;
+
+	stress_device = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+	ASSERT_LE(0, stress_device);
+
+	mutex_args.owner = 0;
+	mutex_args.count = 0;
+	ret = ioctl(stress_device, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+	EXPECT_EQ(0, ret);
+	stress_mutex = mutex_args.mutex;
+
+	event_args.manual = 1;
+	event_args.signaled = 0;
+	ret = ioctl(stress_device, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+	stress_start_event = event_args.event;
+
+	for (i = 0; i < STRESS_THREADS; ++i)
+		pthread_create(&threads[i], NULL, stress_thread, NULL);
+
+	ret = ioctl(stress_start_event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+
+	for (i = 0; i < STRESS_THREADS; ++i) {
+		ret = pthread_join(threads[i], NULL);
+		EXPECT_EQ(0, ret);
+	}
+
+	EXPECT_EQ(STRESS_LOOPS * STRESS_THREADS, stress_counter);
+
+	close(stress_start_event);
+	close(stress_mutex);
+	close(stress_device);
+}
+
 TEST_HARNESS_MAIN
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 30/31] maintainers: Add an entry for ntsync.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Add myself as maintainer, supported by CodeWeavers.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ed4d3868539..d83dd35d9f73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15595,6 +15595,15 @@ T:	git https://github.com/Paragon-Software-Group/linux-ntfs3.git
 F:	Documentation/filesystems/ntfs3.rst
 F:	fs/ntfs3/
 
+NTSYNC SYNCHRONIZATION PRIMITIVE DRIVER
+M:	Elizabeth Figura <zfigura@codeweavers.com>
+L:	wine-devel@winehq.org
+S:	Supported
+F:	Documentation/userspace-api/ntsync.rst
+F:	drivers/misc/ntsync.c
+F:	include/uapi/linux/ntsync.h
+F:	tools/testing/selftests/drivers/ntsync/
+
 NUBUS SUBSYSTEM
 M:	Finn Thain <fthain@linux-m68k.org>
 L:	linux-m68k@lists.linux-m68k.org
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 28/31] selftests: ntsync: Add some tests for wakeup signaling via alerts.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Expand the alert tests to cover alerting a thread mid-wait, to test that the
relevant scheduling logic works correctly.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 .../testing/selftests/drivers/ntsync/ntsync.c | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 5465a16d38b3..968874d7e325 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -1113,9 +1113,12 @@ TEST(wake_all)
 TEST(alert_any)
 {
 	struct ntsync_event_args event_args = {0};
+	struct ntsync_wait_args wait_args = {0};
 	struct ntsync_sem_args sem_args = {0};
 	__u32 index, count, signaled;
+	struct wait_args thread_args;
 	int objs[2], fd, ret;
+	pthread_t thread;
 
 	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
 	ASSERT_LE(0, fd);
@@ -1163,6 +1166,34 @@ TEST(alert_any)
 	EXPECT_EQ(0, ret);
 	EXPECT_EQ(2, index);
 
+	/* test wakeup via alert */
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+
+	wait_args.timeout = get_abs_timeout(1000);
+	wait_args.objs = (uintptr_t)objs;
+	wait_args.count = 2;
+	wait_args.owner = 123;
+	wait_args.index = 0xdeadbeef;
+	wait_args.alert = event_args.event;
+	thread_args.fd = fd;
+	thread_args.args = &wait_args;
+	thread_args.request = NTSYNC_IOC_WAIT_ANY;
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, thread_args.ret);
+	EXPECT_EQ(2, wait_args.index);
+
 	close(event_args.event);
 
 	/* test with an auto-reset event */
@@ -1199,9 +1230,12 @@ TEST(alert_any)
 TEST(alert_all)
 {
 	struct ntsync_event_args event_args = {0};
+	struct ntsync_wait_args wait_args = {0};
 	struct ntsync_sem_args sem_args = {0};
+	struct wait_args thread_args;
 	__u32 index, count, signaled;
 	int objs[2], fd, ret;
+	pthread_t thread;
 
 	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
 	ASSERT_LE(0, fd);
@@ -1235,6 +1269,34 @@ TEST(alert_all)
 	EXPECT_EQ(0, ret);
 	EXPECT_EQ(2, index);
 
+	/* test wakeup via alert */
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+
+	wait_args.timeout = get_abs_timeout(1000);
+	wait_args.objs = (uintptr_t)objs;
+	wait_args.count = 2;
+	wait_args.owner = 123;
+	wait_args.index = 0xdeadbeef;
+	wait_args.alert = event_args.event;
+	thread_args.fd = fd;
+	thread_args.args = &wait_args;
+	thread_args.request = NTSYNC_IOC_WAIT_ALL;
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, thread_args.ret);
+	EXPECT_EQ(2, wait_args.index);
+
 	close(event_args.event);
 
 	/* test with an auto-reset event */
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 26/31] selftests: ntsync: Add some tests for wakeup signaling with events.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Expand the contended wait tests, which previously only covered events and
semaphores, to cover events as well.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 .../testing/selftests/drivers/ntsync/ntsync.c | 151 +++++++++++++++++-
 1 file changed, 147 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 12ccb4ec28e4..5d17eff6a370 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -622,6 +622,7 @@ TEST(test_wait_any)
 
 TEST(test_wait_all)
 {
+	struct ntsync_event_args event_args = {0};
 	struct ntsync_mutex_args mutex_args = {0};
 	struct ntsync_sem_args sem_args = {0};
 	__u32 owner, index, count;
@@ -644,6 +645,11 @@ TEST(test_wait_all)
 	EXPECT_EQ(0, ret);
 	EXPECT_NE(0xdeadbeef, mutex_args.mutex);
 
+	event_args.manual = true;
+	event_args.signaled = true;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+
 	objs[0] = sem_args.sem;
 	objs[1] = mutex_args.mutex;
 
@@ -692,6 +698,14 @@ TEST(test_wait_all)
 	check_sem_state(sem_args.sem, 1, 3);
 	check_mutex_state(mutex_args.mutex, 1, 123);
 
+	objs[0] = sem_args.sem;
+	objs[1] = event_args.event;
+	ret = wait_all(fd, 2, objs, 123, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+	check_sem_state(sem_args.sem, 0, 3);
+	check_event_state(event_args.event, 1, 1);
+
 	/* test waiting on the same object twice */
 	objs[0] = objs[1] = sem_args.sem;
 	ret = wait_all(fd, 2, objs, 123, &index);
@@ -700,6 +714,7 @@ TEST(test_wait_all)
 
 	close(sem_args.sem);
 	close(mutex_args.mutex);
+	close(event_args.event);
 
 	close(fd);
 }
@@ -746,12 +761,13 @@ static int wait_for_thread(pthread_t thread, unsigned int ms)
 
 TEST(wake_any)
 {
+	struct ntsync_event_args event_args = {0};
 	struct ntsync_mutex_args mutex_args = {0};
 	struct ntsync_wait_args wait_args = {0};
 	struct ntsync_sem_args sem_args = {0};
 	struct wait_args thread_args;
+	__u32 count, index, signaled;
 	int objs[2], fd, ret;
-	__u32 count, index;
 	pthread_t thread;
 
 	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
@@ -833,10 +849,101 @@ TEST(wake_any)
 	EXPECT_EQ(0, thread_args.ret);
 	EXPECT_EQ(1, wait_args.index);
 
+	/* test waking events */
+
+	event_args.manual = false;
+	event_args.signaled = false;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+
+	objs[1] = event_args.event;
+	wait_args.timeout = get_abs_timeout(1000);
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event_args.event, 0, 0);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, thread_args.ret);
+	EXPECT_EQ(1, wait_args.index);
+
+	wait_args.timeout = get_abs_timeout(1000);
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event_args.event, 0, 0);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, thread_args.ret);
+	EXPECT_EQ(1, wait_args.index);
+
+	close(event_args.event);
+
+	event_args.manual = true;
+	event_args.signaled = false;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+
+	objs[1] = event_args.event;
+	wait_args.timeout = get_abs_timeout(1000);
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event_args.event, 1, 1);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, thread_args.ret);
+	EXPECT_EQ(1, wait_args.index);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+
+	wait_args.timeout = get_abs_timeout(1000);
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event_args.event, 0, 1);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, thread_args.ret);
+	EXPECT_EQ(1, wait_args.index);
+
+	close(event_args.event);
+
 	/* delete an object while it's being waited on */
 
 	wait_args.timeout = get_abs_timeout(200);
 	wait_args.owner = 123;
+	objs[1] = mutex_args.mutex;
 	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
 	EXPECT_EQ(0, ret);
 
@@ -856,12 +963,14 @@ TEST(wake_any)
 
 TEST(wake_all)
 {
+	struct ntsync_event_args manual_event_args = {0};
+	struct ntsync_event_args auto_event_args = {0};
 	struct ntsync_mutex_args mutex_args = {0};
 	struct ntsync_wait_args wait_args = {0};
 	struct ntsync_sem_args sem_args = {0};
 	struct wait_args thread_args;
-	int objs[2], fd, ret;
-	__u32 count, index;
+	__u32 count, index, signaled;
+	int objs[4], fd, ret;
 	pthread_t thread;
 
 	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
@@ -881,12 +990,24 @@ TEST(wake_all)
 	EXPECT_EQ(0, ret);
 	EXPECT_NE(0xdeadbeef, mutex_args.mutex);
 
+	manual_event_args.manual = true;
+	manual_event_args.signaled = true;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &manual_event_args);
+	EXPECT_EQ(0, ret);
+
+	auto_event_args.manual = false;
+	auto_event_args.signaled = true;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
+	EXPECT_EQ(0, ret);
+
 	objs[0] = sem_args.sem;
 	objs[1] = mutex_args.mutex;
+	objs[2] = manual_event_args.event;
+	objs[3] = auto_event_args.event;
 
 	wait_args.timeout = get_abs_timeout(1000);
 	wait_args.objs = (uintptr_t)objs;
-	wait_args.count = 2;
+	wait_args.count = 4;
 	wait_args.owner = 456;
 	thread_args.fd = fd;
 	thread_args.args = &wait_args;
@@ -920,12 +1041,32 @@ TEST(wake_all)
 
 	check_mutex_state(mutex_args.mutex, 0, 0);
 
+	ret = ioctl(manual_event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+
 	count = 2;
 	ret = post_sem(sem_args.sem, &count);
 	EXPECT_EQ(0, ret);
 	EXPECT_EQ(0, count);
+	check_sem_state(sem_args.sem, 2, 3);
+
+	ret = ioctl(auto_event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+
+	ret = ioctl(manual_event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+
+	ret = ioctl(auto_event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+
 	check_sem_state(sem_args.sem, 1, 3);
 	check_mutex_state(mutex_args.mutex, 1, 456);
+	check_event_state(manual_event_args.event, 1, 1);
+	check_event_state(auto_event_args.event, 0, 0);
 
 	ret = wait_for_thread(thread, 100);
 	EXPECT_EQ(0, ret);
@@ -943,6 +1084,8 @@ TEST(wake_all)
 
 	close(sem_args.sem);
 	close(mutex_args.mutex);
+	close(manual_event_args.event);
+	close(auto_event_args.event);
 
 	ret = wait_for_thread(thread, 200);
 	EXPECT_EQ(0, ret);
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 27/31] selftests: ntsync: Add tests for alertable waits.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Test the "alert" functionality of NTSYNC_IOC_WAIT_ALL and NTSYNC_IOC_WAIT_ANY,
when a wait is woken with an alert and when it is woken by an object.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 .../testing/selftests/drivers/ntsync/ntsync.c | 179 +++++++++++++++++-
 1 file changed, 176 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 5d17eff6a370..5465a16d38b3 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -95,7 +95,7 @@ static int read_event_state(int event, __u32 *signaled, __u32 *manual)
 	})
 
 static int wait_objs(int fd, unsigned long request, __u32 count,
-		     const int *objs, __u32 owner, __u32 *index)
+		     const int *objs, __u32 owner, int alert, __u32 *index)
 {
 	struct ntsync_wait_args args = {0};
 	struct timespec timeout;
@@ -108,6 +108,7 @@ static int wait_objs(int fd, unsigned long request, __u32 count,
 	args.objs = (uintptr_t)objs;
 	args.owner = owner;
 	args.index = 0xdeadbeef;
+	args.alert = alert;
 	ret = ioctl(fd, request, &args);
 	*index = args.index;
 	return ret;
@@ -115,12 +116,26 @@ static int wait_objs(int fd, unsigned long request, __u32 count,
 
 static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
 {
-	return wait_objs(fd, NTSYNC_IOC_WAIT_ANY, count, objs, owner, index);
+	return wait_objs(fd, NTSYNC_IOC_WAIT_ANY, count, objs, owner, 0, index);
 }
 
 static int wait_all(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
 {
-	return wait_objs(fd, NTSYNC_IOC_WAIT_ALL, count, objs, owner, index);
+	return wait_objs(fd, NTSYNC_IOC_WAIT_ALL, count, objs, owner, 0, index);
+}
+
+static int wait_any_alert(int fd, __u32 count, const int *objs,
+			  __u32 owner, int alert, __u32 *index)
+{
+	return wait_objs(fd, NTSYNC_IOC_WAIT_ANY,
+			 count, objs, owner, alert, index);
+}
+
+static int wait_all_alert(int fd, __u32 count, const int *objs,
+			  __u32 owner, int alert, __u32 *index)
+{
+	return wait_objs(fd, NTSYNC_IOC_WAIT_ALL,
+			 count, objs, owner, alert, index);
 }
 
 TEST(semaphore_state)
@@ -1095,4 +1110,162 @@ TEST(wake_all)
 	close(fd);
 }
 
+TEST(alert_any)
+{
+	struct ntsync_event_args event_args = {0};
+	struct ntsync_sem_args sem_args = {0};
+	__u32 index, count, signaled;
+	int objs[2], fd, ret;
+
+	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+	ASSERT_LE(0, fd);
+
+	sem_args.count = 0;
+	sem_args.max = 2;
+	sem_args.sem = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, sem_args.sem);
+	objs[0] = sem_args.sem;
+
+	sem_args.count = 1;
+	sem_args.max = 2;
+	sem_args.sem = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, sem_args.sem);
+	objs[1] = sem_args.sem;
+
+	event_args.manual = true;
+	event_args.signaled = true;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_any_alert(fd, 0, NULL, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_any_alert(fd, 0, NULL, 123, event_args.event, &index);
+	EXPECT_EQ(-1, ret);
+	EXPECT_EQ(ETIMEDOUT, errno);
+
+	ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_any_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, index);
+
+	ret = wait_any_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(2, index);
+
+	close(event_args.event);
+
+	/* test with an auto-reset event */
+
+	event_args.manual = false;
+	event_args.signaled = true;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+
+	count = 1;
+	ret = post_sem(objs[0], &count);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_any_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+
+	ret = wait_any_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(2, index);
+
+	ret = wait_any_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(-1, ret);
+	EXPECT_EQ(ETIMEDOUT, errno);
+
+	close(event_args.event);
+
+	close(objs[0]);
+	close(objs[1]);
+
+	close(fd);
+}
+
+TEST(alert_all)
+{
+	struct ntsync_event_args event_args = {0};
+	struct ntsync_sem_args sem_args = {0};
+	__u32 index, count, signaled;
+	int objs[2], fd, ret;
+
+	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+	ASSERT_LE(0, fd);
+
+	sem_args.count = 2;
+	sem_args.max = 2;
+	sem_args.sem = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, sem_args.sem);
+	objs[0] = sem_args.sem;
+
+	sem_args.count = 1;
+	sem_args.max = 2;
+	sem_args.sem = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, sem_args.sem);
+	objs[1] = sem_args.sem;
+
+	event_args.manual = true;
+	event_args.signaled = true;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_all_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+
+	ret = wait_all_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(2, index);
+
+	close(event_args.event);
+
+	/* test with an auto-reset event */
+
+	event_args.manual = false;
+	event_args.signaled = true;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+
+	count = 2;
+	ret = post_sem(objs[1], &count);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_all_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+
+	ret = wait_all_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(2, index);
+
+	ret = wait_all_alert(fd, 2, objs, 123, event_args.event, &index);
+	EXPECT_EQ(-1, ret);
+	EXPECT_EQ(ETIMEDOUT, errno);
+
+	close(event_args.event);
+
+	close(objs[0]);
+	close(objs[1]);
+
+	close(fd);
+}
+
 TEST_HARNESS_MAIN
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 23/31] selftests: ntsync: Add some tests for wakeup signaling with WINESYNC_IOC_WAIT_ALL.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Test contended "wait-for-all" waits, to make sure that scheduling and wakeup
logic works correctly, and that the wait only exits once objects are all
simultaneously signaled.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 .../testing/selftests/drivers/ntsync/ntsync.c | 98 +++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 993f5db23768..b77fb0b2c4b1 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -706,4 +706,102 @@ TEST(wake_any)
 	close(fd);
 }
 
+TEST(wake_all)
+{
+	struct ntsync_mutex_args mutex_args = {0};
+	struct ntsync_wait_args wait_args = {0};
+	struct ntsync_sem_args sem_args = {0};
+	struct wait_args thread_args;
+	int objs[2], fd, ret;
+	__u32 count, index;
+	pthread_t thread;
+
+	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+	ASSERT_LE(0, fd);
+
+	sem_args.count = 0;
+	sem_args.max = 3;
+	sem_args.sem = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, sem_args.sem);
+
+	mutex_args.owner = 123;
+	mutex_args.count = 1;
+	mutex_args.mutex = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+
+	objs[0] = sem_args.sem;
+	objs[1] = mutex_args.mutex;
+
+	wait_args.timeout = get_abs_timeout(1000);
+	wait_args.objs = (uintptr_t)objs;
+	wait_args.count = 2;
+	wait_args.owner = 456;
+	thread_args.fd = fd;
+	thread_args.args = &wait_args;
+	thread_args.request = NTSYNC_IOC_WAIT_ALL;
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	count = 1;
+	ret = post_sem(sem_args.sem, &count);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, count);
+
+	ret = pthread_tryjoin_np(thread, NULL);
+	EXPECT_EQ(EBUSY, ret);
+
+	check_sem_state(sem_args.sem, 1, 3);
+
+	ret = wait_any(fd, 1, &sem_args.sem, 123, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+
+	ret = unlock_mutex(mutex_args.mutex, 123, &count);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, count);
+
+	ret = pthread_tryjoin_np(thread, NULL);
+	EXPECT_EQ(EBUSY, ret);
+
+	check_mutex_state(mutex_args.mutex, 0, 0);
+
+	count = 2;
+	ret = post_sem(sem_args.sem, &count);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, count);
+	check_sem_state(sem_args.sem, 1, 3);
+	check_mutex_state(mutex_args.mutex, 1, 456);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, thread_args.ret);
+
+	/* delete an object while it's being waited on */
+
+	wait_args.timeout = get_abs_timeout(200);
+	wait_args.owner = 123;
+	ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+	EXPECT_EQ(0, ret);
+
+	ret = wait_for_thread(thread, 100);
+	EXPECT_EQ(ETIMEDOUT, ret);
+
+	close(sem_args.sem);
+	close(mutex_args.mutex);
+
+	ret = wait_for_thread(thread, 200);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(-1, thread_args.ret);
+	EXPECT_EQ(ETIMEDOUT, thread_args.err);
+
+	close(fd);
+}
+
 TEST_HARNESS_MAIN
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 25/31] selftests: ntsync: Add some tests for auto-reset event state.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Test event-specific ioctls NTSYNC_IOC_EVENT_SET, NTSYNC_IOC_EVENT_RESET,
NTSYNC_IOC_EVENT_PULSE, NTSYNC_IOC_EVENT_READ for auto-reset events, and
waiting on auto-reset events.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 .../testing/selftests/drivers/ntsync/ntsync.c | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index b6481c2b85cc..12ccb4ec28e4 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -442,6 +442,65 @@ TEST(manual_event_state)
 	close(fd);
 }
 
+TEST(auto_event_state)
+{
+	struct ntsync_event_args event_args;
+	__u32 index, signaled;
+	int fd, event, ret;
+
+	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+	ASSERT_LE(0, fd);
+
+	event_args.manual = 0;
+	event_args.signaled = 1;
+	event_args.event = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, event_args.event);
+	event = event_args.event;
+
+	check_event_state(event, 1, 0);
+
+	signaled = 0xdeadbeef;
+	ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+	check_event_state(event, 1, 0);
+
+	ret = wait_any(fd, 1, &event, 123, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+	check_event_state(event, 0, 0);
+
+	signaled = 0xdeadbeef;
+	ret = ioctl(event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event, 0, 0);
+
+	ret = wait_any(fd, 1, &event, 123, &index);
+	EXPECT_EQ(-1, ret);
+	EXPECT_EQ(ETIMEDOUT, errno);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+	check_event_state(event, 0, 0);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event, 0, 0);
+
+	close(event);
+
+	close(fd);
+}
+
 TEST(test_wait_any)
 {
 	int objs[NTSYNC_MAX_WAIT_COUNT + 1], fd, ret;
-- 
2.43.0


^ permalink raw reply related

* [PATCH v2 24/31] selftests: ntsync: Add some tests for manual-reset event state.
From: Elizabeth Figura @ 2024-02-19 22:38 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
  Cc: linux-kernel, linux-api, wine-devel, André Almeida,
	Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
	linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240219223833.95710-1-zfigura@codeweavers.com>

Test event-specific ioctls NTSYNC_IOC_EVENT_SET, NTSYNC_IOC_EVENT_RESET,
NTSYNC_IOC_EVENT_PULSE, NTSYNC_IOC_EVENT_READ for manual-reset events, and
waiting on manual-reset events.

Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
 .../testing/selftests/drivers/ntsync/ntsync.c | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index b77fb0b2c4b1..b6481c2b85cc 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -73,6 +73,27 @@ static int unlock_mutex(int mutex, __u32 owner, __u32 *count)
 	return ret;
 }
 
+static int read_event_state(int event, __u32 *signaled, __u32 *manual)
+{
+	struct ntsync_event_args args;
+	int ret;
+
+	memset(&args, 0xcc, sizeof(args));
+	ret = ioctl(event, NTSYNC_IOC_EVENT_READ, &args);
+	*signaled = args.signaled;
+	*manual = args.manual;
+	return ret;
+}
+
+#define check_event_state(event, signaled, manual) \
+	({ \
+		__u32 __signaled, __manual; \
+		int ret = read_event_state((event), &__signaled, &__manual); \
+		EXPECT_EQ(0, ret); \
+		EXPECT_EQ((signaled), __signaled); \
+		EXPECT_EQ((manual), __manual); \
+	})
+
 static int wait_objs(int fd, unsigned long request, __u32 count,
 		     const int *objs, __u32 owner, __u32 *index)
 {
@@ -353,6 +374,74 @@ TEST(mutex_state)
 	close(fd);
 }
 
+TEST(manual_event_state)
+{
+	struct ntsync_event_args event_args;
+	__u32 index, signaled;
+	int fd, event, ret;
+
+	fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+	ASSERT_LE(0, fd);
+
+	event_args.manual = 1;
+	event_args.signaled = 0;
+	event_args.event = 0xdeadbeef;
+	ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+	EXPECT_EQ(0, ret);
+	EXPECT_NE(0xdeadbeef, event_args.event);
+	event = event_args.event;
+	check_event_state(event, 0, 1);
+
+	signaled = 0xdeadbeef;
+	ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event, 1, 1);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+	check_event_state(event, 1, 1);
+
+	ret = wait_any(fd, 1, &event, 123, &index);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, index);
+	check_event_state(event, 1, 1);
+
+	signaled = 0xdeadbeef;
+	ret = ioctl(event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+	check_event_state(event, 0, 1);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_RESET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event, 0, 1);
+
+	ret = wait_any(fd, 1, &event, 123, &index);
+	EXPECT_EQ(-1, ret);
+	EXPECT_EQ(ETIMEDOUT, errno);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(1, signaled);
+	check_event_state(event, 0, 1);
+
+	ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+	EXPECT_EQ(0, ret);
+	EXPECT_EQ(0, signaled);
+	check_event_state(event, 0, 1);
+
+	close(event);
+
+	close(fd);
+}
+
 TEST(test_wait_any)
 {
 	int objs[NTSYNC_MAX_WAIT_COUNT + 1], fd, ret;
-- 
2.43.0


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox