Received-SPF: pass (google.com: domain of linux-kernel+bounces-71851-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Elizabeth Figura <zfigura@codeweavers.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>, Jonathan Corbet <corbet@lwn.net>,
 Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org,
 linux-api@vger.kernel.org, wine-devel@winehq.org,
 =?ISO-8859-1?Q?Andr=E9?= Almeida <andrealmeid@igalia.com>,
 Wolfram Sang <wsa@kernel.org>, Arkadiusz Hiler <ahiler@codeweavers.com>,
 Peter Zijlstra <peterz@infradead.org>, Andy Lutomirski <luto@kernel.org>,
 linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 02/31] ntsync: Introduce NTSYNC_IOC_CREATE_SEM.
Date: Mon, 19 Feb 2024 13:02:07 -0600
Message-ID: <3547727.iIbC2pHGDl@camazotz>
In-Reply-To: <2024021707-sloppy-hurt-df0c@gregkh>
References:
 <20240214233645.9273-1-zfigura@codeweavers.com>
 <13452408.uLZWGnKmhe@camazotz> <2024021707-sloppy-hurt-df0c@gregkh>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

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