linux-kernel - Re: [PATCH v3 7/8] cxl/region: Consolidate cxl_decoder_kill_region() and cxl_region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6876856d329fb_2ead10062@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 15 Jul 2025 09:44:29 -0700
From: <dan.j.williams@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Dan Williams
	<dan.j.williams@...el.com>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Davidlohr
 Bueso" <dave@...olabs.net>, Dave Jiang <dave.jiang@...el.com>, "Alison
 Schofield" <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, "Peter Zijlstra
 (Intel)" <peterz@...radead.org>
Subject: Re: [PATCH v3 7/8] cxl/region: Consolidate cxl_decoder_kill_region()
 and cxl_region_detach()

Jonathan Cameron wrote:
> On Fri, 11 Jul 2025 16:49:31 -0700
> Dan Williams <dan.j.williams@...el.com> wrote:
> 
> > Both detach_target() and cxld_unregister() want to tear down a cxl_region
> > when an endpoint decoder is either detached or destroyed.
> > 
> > When a region is to be destroyed cxl_region_detach() releases
> > cxl_region_rwsem unbinds the cxl_region driver and re-acquires the rwsem.
> > 
> > This "reverse" locking pattern is difficult to reason about, not amenable
> > to scope-based cleanup, and the minor differences in the calling context of
> > detach_target() and cxld_unregister() currently results in the
> > cxl_decoder_kill_region() wrapper.
> > 
> > Introduce cxl_decoder_detach() to wrap a core __cxl_decoder_detach() that
> > serves both cases. I.e. either detaching a known position in a region
> > (interruptible), or detaching an endpoint decoder if it is found to be a
> > member of a region (uninterruptible).
> > 
> > Cc: Davidlohr Bueso <dave@...olabs.net>
> > Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
> > Cc: Dave Jiang <dave.jiang@...el.com>
> > Cc: Alison Schofield <alison.schofield@...el.com>
> > Cc: Vishal Verma <vishal.l.verma@...el.com>
> > Cc: Ira Weiny <ira.weiny@...el.com>
> > Acked-by: "Peter Zijlstra (Intel)" <peterz@...radead.org>
> > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> One query inline about what I think is a change in when a reference count is
> held on the region device.  I'm struggling to reason about whether that change
> would have always been safe or if there is another change here that makes
> it fine now?
> 
> (or whether I'm just misreading the change).
> 
> Jonathan
[..]
> 
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index eb46c6764d20..087a20a9ee1c 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -2001,12 +2001,9 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, "CXL");
> >  
> >  static void cxld_unregister(void *dev)
> >  {
> > -	struct cxl_endpoint_decoder *cxled;
> > -
> > -	if (is_endpoint_decoder(dev)) {
> > -		cxled = to_cxl_endpoint_decoder(dev);
> > -		cxl_decoder_kill_region(cxled);
> > -	}
> > +	if (is_endpoint_decoder(dev))
> > +		cxl_decoder_detach(NULL, to_cxl_endpoint_decoder(dev), -1,
> > +				   DETACH_INVALIDATE);
> >  
> >  	device_unregister(dev);
> >  }
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 2a97fa9a394f..4314aaed8ad8 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2135,27 +2135,43 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  	return 0;
> >  }
> >  
> > -static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
> > +static struct cxl_region *
> > +__cxl_decoder_detach(struct cxl_region *cxlr,
> > +		     struct cxl_endpoint_decoder *cxled, int pos,
> > +		     enum cxl_detach_mode mode)
> >  {
> > -	struct cxl_port *iter, *ep_port = cxled_to_port(cxled);
> > -	struct cxl_region *cxlr = cxled->cxld.region;
> >  	struct cxl_region_params *p;
> > -	int rc = 0;
> >  
> >  	lockdep_assert_held_write(&cxl_region_rwsem);
> >  
> > -	if (!cxlr)
> > -		return 0;
> > +	if (!cxled) {
> > +		p = &cxlr->params;
> >  
> > -	p = &cxlr->params;
> > -	get_device(&cxlr->dev);
> 
> This is a fairly nasty patch to unwind and fully understand but
> I'm nervous that in the existing we have a get_device(&cxlr->dev)
> before potential cxl_region_decode_reset(cxlr, ...)
> and now we don't.  I'm not sure if that is a real problem though,
> it just makes me nervous.

The reference count is not for cxl_region_decode_reset(). The reference
count is to keep the region from being freed when the lock is dropped so
that device_release_driver() can see if it has work to do.

The lookup result from endpoint decoder to region is only stable while
the lock is held and the region could be freed at any point after that.
The pin holds that off until we are done with the potential follow-on
work from detaching a decoder.

The reference is not taken in other paths like region sysfs because
userspace activity in sysfs attributes holds off attribute
unregistration.