[02/31] ntsync: Introduce NTSYNC_IOC_CREATE_SEM.
Commit Message
This corresponds to the NT syscall NtCreateSemaphore().
Semaphores are one of three types of object to be implemented in this driver,
the others being mutexes and events.
An NT semaphore contains a 32-bit counter, and is signaled and can be acquired
when the counter is nonzero. The counter has a maximum value which is specified
at creation time. The initial value of the semaphore is also specified at
creation time. There are no restrictions on the maximum and initial value.
Each object is exposed as an file, to which any number of fds may be opened.
When all fds are closed, the object is deleted.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../userspace-api/ioctl/ioctl-number.rst | 2 +
drivers/misc/ntsync.c | 120 ++++++++++++++++++
include/uapi/linux/ntsync.h | 21 +++
3 files changed, 143 insertions(+)
create mode 100644 include/uapi/linux/ntsync.h
Comments
On Wed, Feb 14, 2024 at 05:36:38PM -0600, Elizabeth Figura wrote:
> This corresponds to the NT syscall NtCreateSemaphore().
>
> Semaphores are one of three types of object to be implemented in this driver,
> the others being mutexes and events.
>
> An NT semaphore contains a 32-bit counter, and is signaled and can be acquired
> when the counter is nonzero. The counter has a maximum value which is specified
> at creation time. The initial value of the semaphore is also specified at
> creation time. There are no restrictions on the maximum and initial value.
>
> Each object is exposed as an file, to which any number of fds may be opened.
> When all fds are closed, the object is deleted.
>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 2 +
> drivers/misc/ntsync.c | 120 ++++++++++++++++++
> include/uapi/linux/ntsync.h | 21 +++
> 3 files changed, 143 insertions(+)
> create mode 100644 include/uapi/linux/ntsync.h
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 457e16f06e04..2f5c6994f042 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -173,6 +173,8 @@ Code Seq# Include File Comments
> 'M' 00-0F drivers/video/fsl-diu-fb.h conflict!
> 'N' 00-1F drivers/usb/scanner.h
> 'N' 40-7F drivers/block/nvme.c
> +'N' 80-8F uapi/linux/ntsync.h NT synchronization primitives
> + <mailto:wine-devel@winehq.org>
> 'O' 00-06 mtd/ubi-user.h UBI
> 'P' all linux/soundcard.h conflict!
> 'P' 60-6F sound/sscape_ioctl.h conflict!
> diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> index e4969ef90722..3ad86d98b82d 100644
> --- a/drivers/misc/ntsync.c
> +++ b/drivers/misc/ntsync.c
> @@ -5,26 +5,146 @@
> * Copyright (C) 2024 Elizabeth Figura
> */
>
> +#include <linux/anon_inodes.h>
> +#include <linux/file.h>
> #include <linux/fs.h>
> #include <linux/miscdevice.h>
> #include <linux/module.h>
> +#include <linux/slab.h>
> +#include <uapi/linux/ntsync.h>
>
> #define NTSYNC_NAME "ntsync"
>
> +enum ntsync_type {
> + NTSYNC_TYPE_SEM,
> +};
> +
> +struct ntsync_obj {
> + enum ntsync_type type;
> +
> + union {
> + struct {
> + __u32 count;
> + __u32 max;
> + } sem;
> + } u;
> +
> + struct file *file;
> + struct ntsync_device *dev;
> +};
> +
> +struct ntsync_device {
> + struct file *file;
> +};
No reference counting is needed for your ntsync_device? Or are you
relying on the reference counting of struct file here?
You pass around pointers to this structure, and save it off into other
structures, how do you know it is "safe" to do so?
thanks,
greg k-h
On Thursday, 15 February 2024 01:28:32 CST Greg Kroah-Hartman wrote:
> On Wed, Feb 14, 2024 at 05:36:38PM -0600, Elizabeth Figura wrote:
> > This corresponds to the NT syscall NtCreateSemaphore().
> >
> > Semaphores are one of three types of object to be implemented in this driver,
> > the others being mutexes and events.
> >
> > An NT semaphore contains a 32-bit counter, and is signaled and can be acquired
> > when the counter is nonzero. The counter has a maximum value which is specified
> > at creation time. The initial value of the semaphore is also specified at
> > creation time. There are no restrictions on the maximum and initial value.
> >
> > Each object is exposed as an file, to which any number of fds may be opened.
> > When all fds are closed, the object is deleted.
> >
> > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> > ---
> > .../userspace-api/ioctl/ioctl-number.rst | 2 +
> > drivers/misc/ntsync.c | 120 ++++++++++++++++++
> > include/uapi/linux/ntsync.h | 21 +++
> > 3 files changed, 143 insertions(+)
> > create mode 100644 include/uapi/linux/ntsync.h
> >
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 457e16f06e04..2f5c6994f042 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -173,6 +173,8 @@ Code Seq# Include File Comments
> > 'M' 00-0F drivers/video/fsl-diu-fb.h conflict!
> > 'N' 00-1F drivers/usb/scanner.h
> > 'N' 40-7F drivers/block/nvme.c
> > +'N' 80-8F uapi/linux/ntsync.h NT synchronization primitives
> > + <mailto:wine-devel@winehq.org>
> > 'O' 00-06 mtd/ubi-user.h UBI
> > 'P' all linux/soundcard.h conflict!
> > 'P' 60-6F sound/sscape_ioctl.h conflict!
> > diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> > index e4969ef90722..3ad86d98b82d 100644
> > --- a/drivers/misc/ntsync.c
> > +++ b/drivers/misc/ntsync.c
> > @@ -5,26 +5,146 @@
> > * Copyright (C) 2024 Elizabeth Figura
> > */
> >
> > +#include <linux/anon_inodes.h>
> > +#include <linux/file.h>
> > #include <linux/fs.h>
> > #include <linux/miscdevice.h>
> > #include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <uapi/linux/ntsync.h>
> >
> > #define NTSYNC_NAME "ntsync"
> >
> > +enum ntsync_type {
> > + NTSYNC_TYPE_SEM,
> > +};
> > +
> > +struct ntsync_obj {
> > + enum ntsync_type type;
> > +
> > + union {
> > + struct {
> > + __u32 count;
> > + __u32 max;
> > + } sem;
> > + } u;
> > +
> > + struct file *file;
> > + struct ntsync_device *dev;
> > +};
> > +
> > +struct ntsync_device {
> > + struct file *file;
> > +};
>
> No reference counting is needed for your ntsync_device? Or are you
> relying on the reference counting of struct file here?
>
> You pass around pointers to this structure, and save it off into other
> structures, how do you know it is "safe" to do so?
Yes, this relies on the reference counting of struct file. The sync
objects (semaphore etc.) grab a reference when they're created, via
get_file(), and release it when they're destroyed. This reference is
taken from within ioctls on the ntsync_device, so the file must be
valid when we grab a reference. Maybe I'm missing something, though?
On Thu, Feb 15, 2024 at 01:22:01PM -0600, Elizabeth Figura wrote:
> On Thursday, 15 February 2024 01:28:32 CST Greg Kroah-Hartman wrote:
> > On Wed, Feb 14, 2024 at 05:36:38PM -0600, Elizabeth Figura wrote:
> > > This corresponds to the NT syscall NtCreateSemaphore().
> > >
> > > Semaphores are one of three types of object to be implemented in this driver,
> > > the others being mutexes and events.
> > >
> > > An NT semaphore contains a 32-bit counter, and is signaled and can be acquired
> > > when the counter is nonzero. The counter has a maximum value which is specified
> > > at creation time. The initial value of the semaphore is also specified at
> > > creation time. There are no restrictions on the maximum and initial value.
> > >
> > > Each object is exposed as an file, to which any number of fds may be opened.
> > > When all fds are closed, the object is deleted.
> > >
> > > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> > > ---
> > > .../userspace-api/ioctl/ioctl-number.rst | 2 +
> > > drivers/misc/ntsync.c | 120 ++++++++++++++++++
> > > include/uapi/linux/ntsync.h | 21 +++
> > > 3 files changed, 143 insertions(+)
> > > create mode 100644 include/uapi/linux/ntsync.h
> > >
> > > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > index 457e16f06e04..2f5c6994f042 100644
> > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > @@ -173,6 +173,8 @@ Code Seq# Include File Comments
> > > 'M' 00-0F drivers/video/fsl-diu-fb.h conflict!
> > > 'N' 00-1F drivers/usb/scanner.h
> > > 'N' 40-7F drivers/block/nvme.c
> > > +'N' 80-8F uapi/linux/ntsync.h NT synchronization primitives
> > > + <mailto:wine-devel@winehq.org>
> > > 'O' 00-06 mtd/ubi-user.h UBI
> > > 'P' all linux/soundcard.h conflict!
> > > 'P' 60-6F sound/sscape_ioctl.h conflict!
> > > diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> > > index e4969ef90722..3ad86d98b82d 100644
> > > --- a/drivers/misc/ntsync.c
> > > +++ b/drivers/misc/ntsync.c
> > > @@ -5,26 +5,146 @@
> > > * Copyright (C) 2024 Elizabeth Figura
> > > */
> > >
> > > +#include <linux/anon_inodes.h>
> > > +#include <linux/file.h>
> > > #include <linux/fs.h>
> > > #include <linux/miscdevice.h>
> > > #include <linux/module.h>
> > > +#include <linux/slab.h>
> > > +#include <uapi/linux/ntsync.h>
> > >
> > > #define NTSYNC_NAME "ntsync"
> > >
> > > +enum ntsync_type {
> > > + NTSYNC_TYPE_SEM,
> > > +};
> > > +
> > > +struct ntsync_obj {
> > > + enum ntsync_type type;
> > > +
> > > + union {
> > > + struct {
> > > + __u32 count;
> > > + __u32 max;
> > > + } sem;
> > > + } u;
> > > +
> > > + struct file *file;
> > > + struct ntsync_device *dev;
> > > +};
> > > +
> > > +struct ntsync_device {
> > > + struct file *file;
> > > +};
> >
> > No reference counting is needed for your ntsync_device? Or are you
> > relying on the reference counting of struct file here?
> >
> > You pass around pointers to this structure, and save it off into other
> > structures, how do you know it is "safe" to do so?
>
> Yes, this relies on the reference counting of struct file. The sync
> objects (semaphore etc.) grab a reference when they're created, via
> get_file(), and release it when they're destroyed. This reference is
> taken from within ioctls on the ntsync_device, so the file must be
> valid when we grab a reference. Maybe I'm missing something, though?
If the reference count is driven by struct file, that's fine, and great,
otherwise you end up with two different reference counts and keeping
them in sync is impossible.
But as it wasn't obvious, a comment somewhere here would be helpful for
reviewing and figuring out how this all works in 4 years when someone
has to touch it again.
thanks,
greg k-h
On Saturday, 17 February 2024 02:03:15 CST Greg Kroah-Hartman wrote:
> On Thu, Feb 15, 2024 at 01:22:01PM -0600, Elizabeth Figura wrote:
> > On Thursday, 15 February 2024 01:28:32 CST Greg Kroah-Hartman wrote:
> > > On Wed, Feb 14, 2024 at 05:36:38PM -0600, Elizabeth Figura wrote:
> > > > This corresponds to the NT syscall NtCreateSemaphore().
> > > >
> > > > Semaphores are one of three types of object to be implemented in this driver,
> > > > the others being mutexes and events.
> > > >
> > > > An NT semaphore contains a 32-bit counter, and is signaled and can be acquired
> > > > when the counter is nonzero. The counter has a maximum value which is specified
> > > > at creation time. The initial value of the semaphore is also specified at
> > > > creation time. There are no restrictions on the maximum and initial value.
> > > >
> > > > Each object is exposed as an file, to which any number of fds may be opened.
> > > > When all fds are closed, the object is deleted.
> > > >
> > > > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> > > > ---
> > > > .../userspace-api/ioctl/ioctl-number.rst | 2 +
> > > > drivers/misc/ntsync.c | 120 ++++++++++++++++++
> > > > include/uapi/linux/ntsync.h | 21 +++
> > > > 3 files changed, 143 insertions(+)
> > > > create mode 100644 include/uapi/linux/ntsync.h
> > > >
> > > > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > > index 457e16f06e04..2f5c6994f042 100644
> > > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > > > @@ -173,6 +173,8 @@ Code Seq# Include File Comments
> > > > 'M' 00-0F drivers/video/fsl-diu-fb.h conflict!
> > > > 'N' 00-1F drivers/usb/scanner.h
> > > > 'N' 40-7F drivers/block/nvme.c
> > > > +'N' 80-8F uapi/linux/ntsync.h NT synchronization primitives
> > > > + <mailto:wine-devel@winehq.org>
> > > > 'O' 00-06 mtd/ubi-user.h UBI
> > > > 'P' all linux/soundcard.h conflict!
> > > > 'P' 60-6F sound/sscape_ioctl.h conflict!
> > > > diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> > > > index e4969ef90722..3ad86d98b82d 100644
> > > > --- a/drivers/misc/ntsync.c
> > > > +++ b/drivers/misc/ntsync.c
> > > > @@ -5,26 +5,146 @@
> > > > * Copyright (C) 2024 Elizabeth Figura
> > > > */
> > > >
> > > > +#include <linux/anon_inodes.h>
> > > > +#include <linux/file.h>
> > > > #include <linux/fs.h>
> > > > #include <linux/miscdevice.h>
> > > > #include <linux/module.h>
> > > > +#include <linux/slab.h>
> > > > +#include <uapi/linux/ntsync.h>
> > > >
> > > > #define NTSYNC_NAME "ntsync"
> > > >
> > > > +enum ntsync_type {
> > > > + NTSYNC_TYPE_SEM,
> > > > +};
> > > > +
> > > > +struct ntsync_obj {
> > > > + enum ntsync_type type;
> > > > +
> > > > + union {
> > > > + struct {
> > > > + __u32 count;
> > > > + __u32 max;
> > > > + } sem;
> > > > + } u;
> > > > +
> > > > + struct file *file;
> > > > + struct ntsync_device *dev;
> > > > +};
> > > > +
> > > > +struct ntsync_device {
> > > > + struct file *file;
> > > > +};
> > >
> > > No reference counting is needed for your ntsync_device? Or are you
> > > relying on the reference counting of struct file here?
> > >
> > > You pass around pointers to this structure, and save it off into other
> > > structures, how do you know it is "safe" to do so?
> >
> > Yes, this relies on the reference counting of struct file. The sync
> > objects (semaphore etc.) grab a reference when they're created, via
> > get_file(), and release it when they're destroyed. This reference is
> > taken from within ioctls on the ntsync_device, so the file must be
> > valid when we grab a reference. Maybe I'm missing something, though?
>
> If the reference count is driven by struct file, that's fine, and great,
> otherwise you end up with two different reference counts and keeping
> them in sync is impossible.
>
> But as it wasn't obvious, a comment somewhere here would be helpful for
> reviewing and figuring out how this all works in 4 years when someone
> has to touch it again.
Ah, makes sense. I'll add comments to be clearer about the refcounting
relationships, thanks.
--Zeb
@@ -173,6 +173,8 @@ Code Seq# Include File Comments
'M' 00-0F drivers/video/fsl-diu-fb.h conflict!
'N' 00-1F drivers/usb/scanner.h
'N' 40-7F drivers/block/nvme.c
+'N' 80-8F uapi/linux/ntsync.h NT synchronization primitives
+ <mailto:wine-devel@winehq.org>
'O' 00-06 mtd/ubi-user.h UBI
'P' all linux/soundcard.h conflict!
'P' 60-6F sound/sscape_ioctl.h conflict!
@@ -5,26 +5,146 @@
* Copyright (C) 2024 Elizabeth Figura
*/
+#include <linux/anon_inodes.h>
+#include <linux/file.h>
#include <linux/fs.h>
#include <linux/miscdevice.h>
#include <linux/module.h>
+#include <linux/slab.h>
+#include <uapi/linux/ntsync.h>
#define NTSYNC_NAME "ntsync"
+enum ntsync_type {
+ NTSYNC_TYPE_SEM,
+};
+
+struct ntsync_obj {
+ enum ntsync_type type;
+
+ union {
+ struct {
+ __u32 count;
+ __u32 max;
+ } sem;
+ } u;
+
+ struct file *file;
+ struct ntsync_device *dev;
+};
+
+struct ntsync_device {
+ struct file *file;
+};
+
+static int ntsync_obj_release(struct inode *inode, struct file *file)
+{
+ struct ntsync_obj *obj = file->private_data;
+
+ fput(obj->dev->file);
+ kfree(obj);
+
+ return 0;
+}
+
+static const struct file_operations ntsync_obj_fops = {
+ .owner = THIS_MODULE,
+ .release = ntsync_obj_release,
+ .llseek = no_llseek,
+};
+
+static struct ntsync_obj *ntsync_alloc_obj(struct ntsync_device *dev,
+ enum ntsync_type type)
+{
+ struct ntsync_obj *obj;
+
+ obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+ if (!obj)
+ return NULL;
+ obj->type = type;
+ obj->dev = dev;
+ get_file(dev->file);
+
+ return obj;
+}
+
+static int ntsync_obj_get_fd(struct ntsync_obj *obj)
+{
+ struct file *file;
+ int fd;
+
+ fd = get_unused_fd_flags(O_CLOEXEC);
+ if (fd < 0)
+ return fd;
+ file = anon_inode_getfile("ntsync", &ntsync_obj_fops, obj, O_RDWR);
+ if (IS_ERR(file)) {
+ put_unused_fd(fd);
+ return PTR_ERR(file);
+ }
+ obj->file = file;
+ fd_install(fd, file);
+
+ return fd;
+}
+
+static int ntsync_create_sem(struct ntsync_device *dev, void __user *argp)
+{
+ struct ntsync_sem_args __user *user_args = argp;
+ struct ntsync_sem_args args;
+ struct ntsync_obj *sem;
+ int fd;
+
+ if (copy_from_user(&args, argp, sizeof(args)))
+ return -EFAULT;
+
+ if (args.count > args.max)
+ return -EINVAL;
+
+ sem = ntsync_alloc_obj(dev, NTSYNC_TYPE_SEM);
+ if (!sem)
+ return -ENOMEM;
+ sem->u.sem.count = args.count;
+ sem->u.sem.max = args.max;
+ fd = ntsync_obj_get_fd(sem);
+ if (fd < 0) {
+ kfree(sem);
+ return fd;
+ }
+
+ return put_user(fd, &user_args->sem);
+}
+
static int ntsync_char_open(struct inode *inode, struct file *file)
{
+ struct ntsync_device *dev;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ file->private_data = dev;
+ dev->file = file;
return nonseekable_open(inode, file);
}
static int ntsync_char_release(struct inode *inode, struct file *file)
{
+ struct ntsync_device *dev = file->private_data;
+
+ kfree(dev);
+
return 0;
}
static long ntsync_char_ioctl(struct file *file, unsigned int cmd,
unsigned long parm)
{
+ struct ntsync_device *dev = file->private_data;
+ void __user *argp = (void __user *)parm;
+
switch (cmd) {
+ case NTSYNC_IOC_CREATE_SEM:
+ return ntsync_create_sem(dev, argp);
default:
return -ENOIOCTLCMD;
}
new file mode 100644
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Kernel support for NT synchronization primitive emulation
+ *
+ * Copyright (C) 2021-2022 Elizabeth Figura
+ */
+
+#ifndef __LINUX_NTSYNC_H
+#define __LINUX_NTSYNC_H
+
+#include <linux/types.h>
+
+struct ntsync_sem_args {
+ __u32 sem;
+ __u32 count;
+ __u32 max;
+};
+
+#define NTSYNC_IOC_CREATE_SEM _IOWR('N', 0x80, struct ntsync_sem_args)
+
+#endif