Re: [PATCH v5 01/15] cxl/region: Add region creation ABI

From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org, patches@lists.linux.dev,
	 Alison Schofield <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	 Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	 Bjorn Helgaas <helgaas@kernel.org>,
	Linux NVDIMM <nvdimm@lists.linux.dev>,
	 Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v5 01/15] cxl/region: Add region creation ABI
Date: Thu, 17 Feb 2022 12:26:58 -0800	[thread overview]
Message-ID: <CAPcyv4in9Pby8X8ydCLH8SOhr3pjYM7UCAbTOHzuMkKmax8M=Q@mail.gmail.com> (raw)
In-Reply-To: <20220217185811.qjct4dlnupgah7lh@intel.com>

On Thu, Feb 17, 2022 at 10:58 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 22-02-17 09:58:04, Dan Williams wrote:
> > On Thu, Feb 17, 2022 at 9:19 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > Regions are created as a child of the decoder that encompasses an
> > > address space with constraints. Regions have a number of attributes that
> > > must be configured before the region can be activated.
> > >
> > > The ABI is not meant to be secure, but is meant to avoid accidental
> > > races. As a result, a buggy process may create a region by name that was
> > > allocated by a different process. However, multiple processes which are
> > > trying not to race with each other shouldn't need special
> > > synchronization to do so.
> > >
> > > // Allocate a new region name
> > > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_region)
> > >
> > > // Create a new region by name
> > > while
> > > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_region)
> > > ! echo $region > /sys/bus/cxl/devices/decoder0.0/create_region
> > > do true; done
> > >
> > > // Region now exists in sysfs
> > > stat -t /sys/bus/cxl/devices/decoder0.0/$region
> > >
> > > // Delete the region, and name
> > > echo $region > /sys/bus/cxl/devices/decoder0.0/delete_region
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
[..]
> > > +static void unregister_region(void *_cxlr)
> > > +{
> > > +       struct cxl_region *cxlr = _cxlr;
> > > +
> > > +       if (!test_and_set_bit(REGION_DEAD, &cxlr->flags))
> > > +               device_unregister(&cxlr->dev);
> >
> > I thought REGION_DEAD was needed to prevent double
> > devm_release_action(), not double unregister?
> >
>
> I believe that's correct, repeating what you said on our internal list:
>
> On 22-02-14 14:11:41, Dan Williams wrote:
>   True, you do need to solve the race between multiple writers racing to
>   do the unregistration, but that could be done with something like:
>
>   if (!test_and_set_bit(REGION_DEAD, &cxlr->flags))
>       device_unregister(&cxlr->dev);
>
> So I was just trying to implement what you said. Remainder of the discussion
> below...

That was in the context of moving the unregistration to a workqueue
and taking the device lock to validate whether the device has already
been unbound. In this case keeping the devm_release_action() inline in
the sysfs attribute the flag needs to protect against racing
devm_release_action(). I am not saying that a workqueue is now needed,
just clarifying the context of that suggestion.

[..]
> > > +
> > > +       return cxlr;
> > > +
> > > +err_out:
> > > +       put_device(dev);
> > > +       kfree(cxlr);
> >
> > This is a double-free of cxlr;
> >
>
> Because of release()? How does release get called if the region device wasn't
> added? Or is there something else?

->release() is always called at final put_device() regardless of
whether the device was registered with device_add() or not. I.e. see
all the other dev_set_name() error handling in the core that just does
put_device().