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: <20250716113208.00001c7d@huawei.com>
Date: Wed, 16 Jul 2025 11:32:08 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <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()

On Tue, 15 Jul 2025 09:44:29 -0700
dan.j.williams@...el.com wrote:

> 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.
Fair enough and thanks for the explanation.
Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>

>