January 2024 - Wine-Devel - list.winehq.org

Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.

by Elizabeth Figura

On Thursday, 25 January 2024 11:02:26 CST Arnd Bergmann wrote: > On Wed, Jan 24, 2024, at 23:28, Elizabeth Figura wrote: > > On Wednesday, 24 January 2024 13:52:52 CST Arnd Bergmann wrote: > >> On Wed, Jan 24, 2024, at 19:02, Elizabeth Figura wrote: > >> > That'd be nicer in general. I think there was some documentation that > >> > advised using timespec64 for new ioctl interfaces but it may have been > >> > outdated or misread. > >> > >> It's probably something I wrote. It depends a bit on > >> whether you have an absolute or relative timeout. If > >> the timeout is relative to the current time as I understand > >> it is here, a 64-bit number seems more logical to me. > >> > >> For absolute times, I would usually use a __kernel_timespec, > >> especially if it's CLOCK_REALTIME. In this case you would > >> also need to specify the time domain. > > > > Currently the interface does pass it as an absolute time, with the > > domain implicitly being MONOTONIC. This particular choice comes from > > process/botching-up-ioctls.rst, which is admittedly focused around GPU > > ioctls, but the rationale of having easily restartable ioctls applies > > here too. > > Ok, I was thinking of Documentation/driver-api/ioctl.rst, which > has similar recommendations. > > > (E.g. Wine does play games with signals, so we do want to be able to > > interrupt arbitrary waits with EINTR. The "usual" fast path for ntsync > > waits won't hit that, but we want to have it work.) > > > > On the other hand, if we can pass the timeout as relative, and write it > > back on exit like ppoll() does [assuming that's not proscribed], that > > would presumably be slightly better for performance. > > I've seen arguments go either way between absolute and relative > times, just pick whatever works best for you here. > > > When writing the patch I just picked the recommended option, and didn't > > bother doing any micro-optimizations afterward. > > > > What's the rationale for using timespec for absolute or written-back > > timeouts, instead of dealing in ns directly? I'm afraid it's not > > obvious to me. > > There is no hard rule either way, I mainly didn't like the > indirect pointer to the timespec that you have here. For > traditional unix-style interfaces, a timespec with CLOCK_REALTIME > times usually makes sense since that is what user space is > already using elsewhere, but you probably don't need to > worry about that. In theory, the single u64 CLOCK_REALTIME > nanoseconds have the problem of no longer working after year > 2262, but with CLOCK_MONOTONIC that is not a concern anyway. > > Between embedding a __u64 nanosecond value and embedding > a __kernel_timespec, I would pick whichever avoids converting > a __u64 back into a timespec, as that is an expensive > operation at least on 32-bit code. Makes sense. I'll probably switch to using a relative and written-back u64 then, thanks!

Jan. 25, 2024

Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.

by Elizabeth Figura

On Thursday, 25 January 2024 10:47:49 CST Arnd Bergmann wrote: > On Thu, Jan 25, 2024, at 04:42, Elizabeth Figura wrote: > > On Wednesday, 24 January 2024 16:56:23 CST Elizabeth Figura wrote: > >> On Wednesday, 24 January 2024 15:26:15 CST Andy Lutomirski wrote: > >> > On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura <zfigura(a)codeweavers.com> wrote: > >> [There is also a potential problem where some broken applications > >> create a million (literally) sync objects. Making these into files runs > >> into NOFILE. We did specifically push distributions and systemd to > >> increase those limits because an older solution *did* use eventfds and > >> *did* run into those limits. Since that push was successful I don't > >> know if this is *actually* a concern anymore, but avoiding files is > >> probably not a bad thing either.] > > > > Of course, looking at it from a kernel maintainer's perspective, it > > wouldn't be insane to do this anyway. If we at some point do start to > > care about cross- process isolation in this way, or if another NT > > emulator wants to use this interface and does care about cross-process > > isolation, it'll be necessary. At least it'd make sense to make them > > separate files even if we don't implement granular permission handling > > just yet. > > I can think of a few other possible benefits of going with > per-mutex file descriptors: > > - being able to use poll() for waiting on them individually in > combination with other file descriptor based events (socket, > signalfd, pidfd, ...) I can say for sure this isn't going to be useful for Wine, at least not with the current design. It also doesn't really mesh well with the NT design in the first place. NTSYNC_IOC_WAIT_ANY differs from poll() in two major ways: it consumes state of most object types, and (as coded here) it needs the owner thread ID to be specifically passed for mutexes. Anyway, as Alexandre has informed me I clearly have misunderstood our requirements, so I'm going to try to put together something using files instead.

Jan. 25, 2024

Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.

by Alexandre Julliard

Elizabeth Figura <zfigura(a)codeweavers.com> writes: > On Wednesday, 24 January 2024 15:26:15 CST Andy Lutomirski wrote: >> On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura >> <zfigura(a)codeweavers.com> wrote: >> > >> > ntsync uses a misc device as the simplest and least intrusive uAPI interface. >> > >> > Each file description on the device represents an isolated NT instance, intended >> > to correspond to a single NT virtual machine. >> >> If I understand this text right, and if I understood the code right, >> you're saying that each open instance of the device represents an >> entire universe of NT synchronization objects, and no security or >> isolation is possible between those objects. For single-process use, >> this seems fine. But fork() will be a bit odd (although NT doesn't >> really believe in fork, so maybe this is fine). >> >> Except that NT has *named* semaphores and such. And I'm pretty sure >> I've written GUI programs that use named synchronization objects (IIRC >> they were events, and this was a *very* common pattern, regularly >> discussed in MSDN, usenet, etc) to detect whether another instance of >> the program is running. And this all works on real Windows because >> sessions have sufficiently separated namespaces, and the security all >> works out about as any other security on Windows, etc. But >> implementing *that* on top of this >> file-description-plus-integer-equals-object will be fundamentally >> quite subject to one buggy program completely clobbering someone >> else's state. >> >> Would it make sense and scale appropriately for an NT synchronization >> *object* to be a Linux open file description? Then SCM_RIGHTS could >> pass them around, an RPC server could manage *named* objects, and >> they'd generally work just like other "Object Manager" objects like, >> say, files. > > It's a sensible concern. I think when I discussed this with Alexandre > Julliard (the Wine maintainer, CC'd) the conclusion was this wasn't > something we were concerned about. > > While the current model *does* allow for processes to arbitrarily mess > with each other, accidentally or not, I think we're not concerned with > the scope of that than we are about implementing a whole scheduler in > user space. I may have misunderstood something in that dicussion then, because it would definitely be a concern. It's OK for a process to be able to mess up the state of any object that it has an NT handle to, but it shouldn't be possible to mess up the state of unrelated objects in other processes simply by passing the wrong integer id. The concern is not so much about a malicious process going out of its way to corrupt others, because it could do that through the NT API just as well. But if a wayward pointer corrupts the client-side handle cache, that shouldn't take down the entire session. -- Alexandre Julliard julliard(a)winehq.org

Jan. 25, 2024

Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.

by Elizabeth Figura

On Wednesday, 24 January 2024 16:56:23 CST Elizabeth Figura wrote: > On Wednesday, 24 January 2024 15:26:15 CST Andy Lutomirski wrote: > > > On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura > > <zfigura(a)codeweavers.com> wrote: > > > > > > > > > > > ntsync uses a misc device as the simplest and least intrusive uAPI > > > interface. > > > > > > > > > > > Each file description on the device represents an isolated NT instance, > > > intended to correspond to a single NT virtual machine. > > > > > > If I understand this text right, and if I understood the code right, > > you're saying that each open instance of the device represents an > > entire universe of NT synchronization objects, and no security or > > isolation is possible between those objects. For single-process use, > > this seems fine. But fork() will be a bit odd (although NT doesn't > > really believe in fork, so maybe this is fine). > > > > Except that NT has *named* semaphores and such. And I'm pretty sure > > I've written GUI programs that use named synchronization objects (IIRC > > they were events, and this was a *very* common pattern, regularly > > discussed in MSDN, usenet, etc) to detect whether another instance of > > the program is running. And this all works on real Windows because > > sessions have sufficiently separated namespaces, and the security all > > works out about as any other security on Windows, etc. But > > implementing *that* on top of this > > file-description-plus-integer-equals-object will be fundamentally > > quite subject to one buggy program completely clobbering someone > > else's state. > > > > Would it make sense and scale appropriately for an NT synchronization > > *object* to be a Linux open file description? Then SCM_RIGHTS could > > pass them around, an RPC server could manage *named* objects, and > > they'd generally work just like other "Object Manager" objects like, > > say, files. > > > It's a sensible concern. I think when I discussed this with Alexandre > Julliard (the Wine maintainer, CC'd) the conclusion was this wasn't > something we were concerned about. > > While the current model *does* allow for processes to arbitrarily mess > with each other, accidentally or not, I think we're not concerned with > the scope of that than we are about implementing a whole scheduler in > user space. > > For one, you can't corrupt the wineserver state this way—wineserver > being sort of like a dedicated process that handles many of the things > that a kernel would, and so sometimes needs to set or reset events, or > perform NTSYNC_IOC_KILL_MUTEX, but never relies on ntsync object state. > Whereas trying to implement a scheduler in user space would involve the > wineserver taking locks, and hence other processes could deadlock. > > For two, it's probably a lot harder to mess with that internal state > accidentally. > > [There is also a potential problem where some broken applications > create a million (literally) sync objects. Making these into files runs > into NOFILE. We did specifically push distributions and systemd to > increase those limits because an older solution *did* use eventfds and > *did* run into those limits. Since that push was successful I don't > know if this is *actually* a concern anymore, but avoiding files is > probably not a bad thing either.] Of course, looking at it from a kernel maintainer's perspective, it wouldn't be insane to do this anyway. If we at some point do start to care about cross- process isolation in this way, or if another NT emulator wants to use this interface and does care about cross-process isolation, it'll be necessary. At least it'd make sense to make them separate files even if we don't implement granular permission handling just yet. The main question is, is NOFILE a realistic concern, and what other problems might there be, in terms of making these heavier objects? Besides memory usage I can't think of any, but of course I don't have much knowledge of this area. Alternatively, maybe there's another more lightweight way to store per-process data?

Jan. 25, 2024

Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.

by Elizabeth Figura

On Wednesday, 24 January 2024 15:26:15 CST Andy Lutomirski wrote: > On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura > <zfigura(a)codeweavers.com> wrote: > > > > ntsync uses a misc device as the simplest and least intrusive uAPI interface. > > > > Each file description on the device represents an isolated NT instance, intended > > to correspond to a single NT virtual machine. > > If I understand this text right, and if I understood the code right, > you're saying that each open instance of the device represents an > entire universe of NT synchronization objects, and no security or > isolation is possible between those objects. For single-process use, > this seems fine. But fork() will be a bit odd (although NT doesn't > really believe in fork, so maybe this is fine). > > Except that NT has *named* semaphores and such. And I'm pretty sure > I've written GUI programs that use named synchronization objects (IIRC > they were events, and this was a *very* common pattern, regularly > discussed in MSDN, usenet, etc) to detect whether another instance of > the program is running. And this all works on real Windows because > sessions have sufficiently separated namespaces, and the security all > works out about as any other security on Windows, etc. But > implementing *that* on top of this > file-description-plus-integer-equals-object will be fundamentally > quite subject to one buggy program completely clobbering someone > else's state. > > Would it make sense and scale appropriately for an NT synchronization > *object* to be a Linux open file description? Then SCM_RIGHTS could > pass them around, an RPC server could manage *named* objects, and > they'd generally work just like other "Object Manager" objects like, > say, files. It's a sensible concern. I think when I discussed this with Alexandre Julliard (the Wine maintainer, CC'd) the conclusion was this wasn't something we were concerned about. While the current model *does* allow for processes to arbitrarily mess with each other, accidentally or not, I think we're not concerned with the scope of that than we are about implementing a whole scheduler in user space. For one, you can't corrupt the wineserver state this way—wineserver being sort of like a dedicated process that handles many of the things that a kernel would, and so sometimes needs to set or reset events, or perform NTSYNC_IOC_KILL_MUTEX, but never relies on ntsync object state. Whereas trying to implement a scheduler in user space would involve the wineserver taking locks, and hence other processes could deadlock. For two, it's probably a lot harder to mess with that internal state accidentally. [There is also a potential problem where some broken applications create a million (literally) sync objects. Making these into files runs into NOFILE. We did specifically push distributions and systemd to increase those limits because an older solution *did* use eventfds and *did* run into those limits. Since that push was successful I don't know if this is *actually* a concern anymore, but avoiding files is probably not a bad thing either.] --Zeb

Jan. 24, 2024

Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.

by Elizabeth Figura

On Wednesday, 24 January 2024 13:52:52 CST Arnd Bergmann wrote: > On Wed, Jan 24, 2024, at 19:02, Elizabeth Figura wrote: > > On Wednesday, 24 January 2024 01:56:52 CST Arnd Bergmann wrote: > >> On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote: > >> > >> > + if (args->timeout) { > >> > + struct timespec64 to; > >> > + > >> > + if (get_timespec64(&to, u64_to_user_ptr(args->timeout))) > >> > + return -EFAULT; > >> > + if (!timespec64_valid(&to)) > >> > + return -EINVAL; > >> > + > >> > + timeout = timespec64_to_ns(&to); > >> > + } > >> > >> Have you considered just passing the nanosecond value here? > >> Since you do not appear to write it back, that would avoid > >> the complexities of dealing with timespec layout differences > >> and indirection. > > > > That'd be nicer in general. I think there was some documentation that advised > > using timespec64 for new ioctl interfaces but it may have been outdated or > > misread. > > It's probably something I wrote. It depends a bit on > whether you have an absolute or relative timeout. If > the timeout is relative to the current time as I understand > it is here, a 64-bit number seems more logical to me. > > For absolute times, I would usually use a __kernel_timespec, > especially if it's CLOCK_REALTIME. In this case you would > also need to specify the time domain. Currently the interface does pass it as an absolute time, with the domain implicitly being MONOTONIC. This particular choice comes from process/botching-up-ioctls.rst, which is admittedly focused around GPU ioctls, but the rationale of having easily restartable ioctls applies here too. (E.g. Wine does play games with signals, so we do want to be able to interrupt arbitrary waits with EINTR. The "usual" fast path for ntsync waits won't hit that, but we want to have it work.) On the other hand, if we can pass the timeout as relative, and write it back on exit like ppoll() does [assuming that's not proscribed], that would presumably be slightly better for performance. When writing the patch I just picked the recommended option, and didn't bother doing any micro-optimizations afterward. What's the rationale for using timespec for absolute or written-back timeouts, instead of dealing in ns directly? I'm afraid it's not obvious to me. --Zeb

Jan. 24, 2024

Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.

by Andy Lutomirski

On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura <zfigura(a)codeweavers.com> wrote: > > ntsync uses a misc device as the simplest and least intrusive uAPI interface. > > Each file description on the device represents an isolated NT instance, intended > to correspond to a single NT virtual machine. If I understand this text right, and if I understood the code right, you're saying that each open instance of the device represents an entire universe of NT synchronization objects, and no security or isolation is possible between those objects. For single-process use, this seems fine. But fork() will be a bit odd (although NT doesn't really believe in fork, so maybe this is fine). Except that NT has *named* semaphores and such. And I'm pretty sure I've written GUI programs that use named synchronization objects (IIRC they were events, and this was a *very* common pattern, regularly discussed in MSDN, usenet, etc) to detect whether another instance of the program is running. And this all works on real Windows because sessions have sufficiently separated namespaces, and the security all works out about as any other security on Windows, etc. But implementing *that* on top of this file-description-plus-integer-equals-object will be fundamentally quite subject to one buggy program completely clobbering someone else's state. Would it make sense and scale appropriately for an NT synchronization *object* to be a Linux open file description? Then SCM_RIGHTS could pass them around, an RPC server could manage *named* objects, and they'd generally work just like other "Object Manager" objects like, say, files. --Andy

Jan. 24, 2024

Re: [RFC PATCH 8/9] ntsync: Introduce NTSYNC_IOC_PUT_MUTEX.

by Elizabeth Figura

On Wednesday, 24 January 2024 01:42:19 CST Arnd Bergmann wrote: > On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote: > > @@ -738,6 +803,8 @@ static long ntsync_char_ioctl(struct file *file, > > diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h > > index 26d1b3d4847f..2e44e7e77776 100644 > > --- a/include/uapi/linux/ntsync.h > > +++ b/include/uapi/linux/ntsync.h > > @@ -46,5 +46,7 @@ struct ntsync_wait_args { > > struct ntsync_wait_args) > > #define NTSYNC_IOC_CREATE_MUTEX _IOWR(NTSYNC_IOC_BASE, 5, \ > > struct ntsync_mutex_args) > > +#define NTSYNC_IOC_PUT_MUTEX _IOWR(NTSYNC_IOC_BASE, 6, \ > > + struct ntsync_mutex_args) > > > > In your implementation, this argument is not written back to > user space, so I think this should be _IOW rather than than _IORW. > > Again, no practical difference here. Hm, but there is a put_user() at the end of the function, or am I missing something?

Jan. 24, 2024

Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.

by Elizabeth Figura

On Wednesday, 24 January 2024 01:56:52 CST Arnd Bergmann wrote: > On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote: > > > + if (args->timeout) { > > + struct timespec64 to; > > + > > + if (get_timespec64(&to, u64_to_user_ptr(args->timeout))) > > + return -EFAULT; > > + if (!timespec64_valid(&to)) > > + return -EINVAL; > > + > > + timeout = timespec64_to_ns(&to); > > + } > > Have you considered just passing the nanosecond value here? > Since you do not appear to write it back, that would avoid > the complexities of dealing with timespec layout differences > and indirection. That'd be nicer in general. I think there was some documentation that advised using timespec64 for new ioctl interfaces but it may have been outdated or misread. > > > + ids = kmalloc_array(count, sizeof(*ids), GFP_KERNEL); > > + if (!ids) > > + return -ENOMEM; > > + if (copy_from_user(ids, u64_to_user_ptr(args->objs), > > + array_size(count, sizeof(*ids)))) { > > + kfree(ids); > > + return -EFAULT; > > + } > > This looks like memdup_user() would be slightly simpler. That's useful, thanks.

Jan. 24, 2024

Re: [RFC PATCH 2/9] ntsync: Reserve a minor device number and ioctl range.

by Elizabeth Figura

On Wednesday, 24 January 2024 06:32:13 CST Greg Kroah-Hartman wrote: > On Tue, Jan 23, 2024 at 09:43:09PM -0600, Elizabeth Figura wrote: > > > Why do you need a fixed minor number? Can't your userspace handle > > > dynamic numbers? What systems require a static value? > > > > I believe I added this because it's necessary for MODULE_ALIAS (and, more > > broadly, because I was following the example of vaguely comparable devices > > like /dev/loop-control). I suppose I could instead just remove MODULE_ALIAS > > (or even remove the ability to compile ntsync as a module entirely). > > Do you really need MODULE_ALIAS()? Having it for char devices to be > auto-loaded is not generally considered a good idea anymore as systems > should have the module loaded before userspace goes and asks for it. > > It also reduces suprises when any random userspace program can cause > kernel modules to be loaded for no real reason. I think there's no reason we can't make loading the system's problem. I'll remove it in v2.

Jan. 24, 2024