[PATCH 1/2] cxl/region: Reject a repeat write of an endpoint decoder to targetN

[PATCH 1/2] cxl/region: Reject a repeat write of an endpoint decoder to targetN

[Alison Schofield]

Writing an endpoint decoder to a region targetN attribute that already
holds it returns -EBUSY, but leaves the partially populated region in an
inconsistent state. The targetN attribute still names the decoder, so the
failed write gives no indication that the region was damaged.

Filling the remaining target positions can then trigger a NULL pointer
dereference in cxl_region_setup_targets(). Root can reproduce this
through sysfs:

  echo $decoder > /sys/bus/cxl/devices/region0/target0
  echo $decoder > /sys/bus/cxl/devices/region0/target0
  echo $other_decoder > /sys/bus/cxl/devices/region0/target1

Reject a write to an already assigned position before modifying the region.

Fixes: b9686e8c8e39 ("cxl/region: Enable the assignment of endpoint decoders to regions")
Assisted-by: LLM
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---


An ndctl patch backstopping this fix in the cxl unit tests will follow.



 drivers/cxl/core/region.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 27e63e6dab7c..f9aafe89e5bb 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1762,9 +1762,6 @@ static int cxl_region_validate_position(struct cxl_region *cxlr,
 		return -ENXIO;
 	}
 
-	if (p->targets[pos] == cxled)
-		return 0;
-
 	if (p->targets[pos]) {
 		struct cxl_endpoint_decoder *cxled_target = p->targets[pos];
 		struct cxl_memdev *cxlmd_target = cxled_to_memdev(cxled_target);

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.37.3

[PATCH 2/2] cxl/Documentation: Describe clearing a region targetN position

[Alison Schofield]

The regionZ/target[0..N] ABI says a write to an occupied position fails
with EBUSY, but does not describe how to free the position. Clearing a
position is supported but undocumented, leaving no documented recovery
path after writing the wrong endpoint decoder.

Document how to clear a position.

Assisted-by: LLM
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 7352dbd70bc7..3b96b855961a 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -559,15 +559,20 @@ Description:
 		(RW) Write an endpoint decoder object name to 'targetX' where X
 		is the intended position of the endpoint device in the region
 		interleave and N is the 'interleave_ways' setting for the
-		region. ENXIO is returned if the write results in an impossible
-		to map decode scenario, like the endpoint is unreachable at that
-		position relative to the root decoder interleave. EBUSY is
-		returned if the position in the region is already occupied, or
-		if the region is not in a state to accept interleave
-		configuration changes. EINVAL is returned if the object name is
-		not an endpoint decoder. Once all positions have been
-		successfully written a final validation for decode conflicts is
-		performed before activating the region.
+		region. Write a newline to 'targetX', as 'echo "" > targetX'
+		does, to remove the endpoint decoder from that position, for
+		example to replace it with another decoder. Removing a target
+		from a committed region also decommits the region. It can be
+		committed again after all positions are filled. ENXIO is
+		returned if the write results in an impossible to map decode
+		scenario, like the endpoint is unreachable at that position
+		relative to the root decoder interleave. EINVAL is returned if
+		the object name is not an endpoint decoder. EBUSY is returned if
+		the position in the region is already occupied, or if the region
+		is not in a state to accept interleave configuration changes.
+		Once all positions have been successfully written a final
+		validation for decode conflicts is performed before activating
+		the region.
 
 
 What:		/sys/bus/cxl/devices/regionZ/commit
-- 
2.37.3

Re: [PATCH 1/2] cxl/region: Reject a repeat write of an endpoint decoder to targetN

[Jonathan Cameron]

On Wed, 30 Sep 2026 21:56:38 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> Writing an endpoint decoder to a region targetN attribute that already
> holds it returns -EBUSY, but leaves the partially populated region in an
> inconsistent state. The targetN attribute still names the decoder, so the
> failed write gives no indication that the region was damaged.
> 
> Filling the remaining target positions can then trigger a NULL pointer
> dereference in cxl_region_setup_targets(). Root can reproduce this
> through sysfs:
> 
>   echo $decoder > /sys/bus/cxl/devices/region0/target0
>   echo $decoder > /sys/bus/cxl/devices/region0/target0
>   echo $other_decoder > /sys/bus/cxl/devices/region0/target1
> 
> Reject a write to an already assigned position before modifying the region.

Rejecting a write that does nothing with an error isn't the nicest of
ABI choices - not to mention it's an ABI change (even if doesn't matter
much here given previously it probably crashed :).

Can we instead just reject much earlier before doing anything that
needs tearing down?  Would moving the check to the top of
cxl_region_attach() work?  There are some queries before that but
I don't think any of them update state and I think we
already have the stuff needed for a sanity check and early bail
out at that point.

I'm too lazy to actually test that theory so may well have missed something!

Thanks,

Jonathan



> 
> Fixes: b9686e8c8e39 ("cxl/region: Enable the assignment of endpoint decoders to regions")
> Assisted-by: LLM
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> 
> 
> An ndctl patch backstopping this fix in the cxl unit tests will follow.
> 
> 
> 
>  drivers/cxl/core/region.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 27e63e6dab7c..f9aafe89e5bb 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1762,9 +1762,6 @@ static int cxl_region_validate_position(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> -	if (p->targets[pos] == cxled)
> -		return 0;
> -
>  	if (p->targets[pos]) {
>  		struct cxl_endpoint_decoder *cxled_target = p->targets[pos];
>  		struct cxl_memdev *cxlmd_target = cxled_to_memdev(cxled_target);
> 
> base-commit: cee9395acd8043be0644b25c34bfa86623f2b935

Re: [PATCH 1/2] cxl/region: Reject a repeat write of an endpoint decoder to targetN

[Alison Schofield]

On Tue, Oct 20, 2026 at 10:39:20PM +0100, Jonathan Cameron wrote:
> On Wed, 30 Sep 2026 21:56:38 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
> 
> > Writing an endpoint decoder to a region targetN attribute that already
> > holds it returns -EBUSY, but leaves the partially populated region in an
> > inconsistent state. The targetN attribute still names the decoder, so the
> > failed write gives no indication that the region was damaged.
> > 
> > Filling the remaining target positions can then trigger a NULL pointer
> > dereference in cxl_region_setup_targets(). Root can reproduce this
> > through sysfs:
> > 
> >   echo $decoder > /sys/bus/cxl/devices/region0/target0
> >   echo $decoder > /sys/bus/cxl/devices/region0/target0
> >   echo $other_decoder > /sys/bus/cxl/devices/region0/target1
> > 
> > Reject a write to an already assigned position before modifying the region.

Thanks for the review Jonathan,

> 
> Rejecting a write that does nothing with an error isn't the nicest of
> ABI choices - not to mention it's an ABI change (even if doesn't matter
> much here given previously it probably crashed :).

Agree. Rather than change the repeat write to return -EBUSY, in v2
I'll preserve ABI and return success...without modifying the region.
That is the key - don't modify the region!

> 
> Can we instead just reject much earlier before doing anything that
> needs tearing down?  Would moving the check to the top of
> cxl_region_attach() work?  There are some queries before that but
> I don't think any of them update state and I think we
> already have the stuff needed for a sanity check and early bail
> out at that point.

Almost. cxl_region_perf_data_calculate() updates the bandwidth before
the position checks, so the no-op check is going just before it.
Keeping it below the region state checks preserves existing behavior
for full or committed regions.

This revealed another issue that'll I'll come back around to in another
patch. That perf data is updated before attachment has succeeded, so
that is a problem on other failure paths. I don't think that fix
collides w this one, but I'll work both out before posting this v2.

BTW although the ABI won't change in v2, I do think it's useful to document
how to clear a targetN position. I'm leaving that in, you can comment
when you see it next.

> I'm too lazy to actually test that theory so may well have missed something!

My test is w cxl-test. Repeat writes return 0 without changing the
region and conflicting writes still return -EBUSY, and the region 
does complete and commit.

-- Alison

> 
> Thanks,
> 
> Jonathan
> 
> 
> 
> > 
> > Fixes: b9686e8c8e39 ("cxl/region: Enable the assignment of endpoint decoders to regions")
> > Assisted-by: LLM
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> > 
> > 
> > An ndctl patch backstopping this fix in the cxl unit tests will follow.
> > 
> > 
> > 
> >  drivers/cxl/core/region.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 27e63e6dab7c..f9aafe89e5bb 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1762,9 +1762,6 @@ static int cxl_region_validate_position(struct cxl_region *cxlr,
> >  		return -ENXIO;
> >  	}
> >  
> > -	if (p->targets[pos] == cxled)
> > -		return 0;
> > -
> >  	if (p->targets[pos]) {
> >  		struct cxl_endpoint_decoder *cxled_target = p->targets[pos];
> >  		struct cxl_memdev *cxlmd_target = cxled_to_memdev(cxled_target);
> > 
> > base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
>