[PATCH] cxl/region: Add decoder check to check_commit_order()

[PATCH] cxl/region: Add decoder check to check_commit_order()

[Dave Jiang]

check_commit_order() attempts to convert a device to a decoder without
making sure the device is a decoder. So far this has been working due
to pure luck. Issue discovered while doing deferred dport probing when
child ports are now in the midst of decoders due to ordering change
of child port additions. Add a check before attempting to do decoder
conversion.

Fixes: 105b6235ad0f ("cxl/port: Prevent out-of-order decoder allocation")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/region.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c3f4dc244df7..a91d4eb061e4 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -788,7 +788,12 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 
 static int check_commit_order(struct device *dev, void *data)
 {
-	struct cxl_decoder *cxld = to_cxl_decoder(dev);
+	struct cxl_decoder *cxld;
+
+	if (!is_switch_decoder(dev))
+		return 0;
+
+	cxld = to_cxl_decoder(dev);
 
 	/*
 	 * if port->commit_end is not the only free decoder, then out of
-- 
2.49.0

Re: [PATCH] cxl/region: Add decoder check to check_commit_order()

[Alison Schofield]

On Wed, Apr 30, 2025 at 02:36:53PM -0700, Dave Jiang wrote:
> check_commit_order() attempts to convert a device to a decoder without
> making sure the device is a decoder. So far this has been working due
> to pure luck. Issue discovered while doing deferred dport probing when
> child ports are now in the midst of decoders due to ordering change
> of child port additions. Add a check before attempting to do decoder
> conversion.
> 
> Fixes: 105b6235ad0f ("cxl/port: Prevent out-of-order decoder allocation")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Update the commit message to mention and paste in the ensuing NULL ptr
dereference so other unlucky folks can find this.

with that -

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH] cxl/region: Add decoder check to check_commit_order()

[Li Ming]

On 5/1/2025 5:36 AM, Dave Jiang wrote:
> check_commit_order() attempts to convert a device to a decoder without
> making sure the device is a decoder. So far this has been working due
> to pure luck. Issue discovered while doing deferred dport probing when
> child ports are now in the midst of decoders due to ordering change
> of child port additions. Add a check before attempting to do decoder
> conversion.
>
> Fixes: 105b6235ad0f ("cxl/port: Prevent out-of-order decoder allocation")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

I also have a patch for this issue during I worked on deferred dport probing.

https://lore.kernel.org/linux-cxl/20250107035708.1134954-4-ming.li@zohomail.com/

I am not sure if we should define it as a bug, my understanding is that it will never happen with current implementation, it only happens when we are trying to delay the switch decoders initialization.

Because current CXL subsystem always initialize switch decoders during switch port probing and child ports are always added after the switch decoders initialization done, so the sequence of children device list of switch port device always like that:

child dev list head - decoder 0 - decoder 1 - decoder 2 ..... - child port dev 0 - child port dev 1.....

Using device_for_each_child_reverse_from(switch_port_dev, current_decoder, NULL, check_commit_order) is not problem for this sequence.


But when have deferred dport probing implementation, the first child port attaching is responsible for triggering switch decoders initialization, the sequence will be changed like that:

child dev list head - child port dev 0 - decoder 0 - decoder 1 - decoder 2 ..... - child port dev 1.....

Always have a child port device ahead of all decoders, it causes the problem.


Anyhow, it is also OK to me if it should be considered as a bug.

Reviewed-by: Li Ming <ming.li@zohomail.com>

Re: [PATCH] cxl/region: Add decoder check to check_commit_order()

[dan.j.williams]

Li Ming wrote:
> On 5/1/2025 5:36 AM, Dave Jiang wrote:
> > check_commit_order() attempts to convert a device to a decoder without
> > making sure the device is a decoder. So far this has been working due
> > to pure luck. Issue discovered while doing deferred dport probing when
> > child ports are now in the midst of decoders due to ordering change
> > of child port additions. Add a check before attempting to do decoder
> > conversion.
> >
> > Fixes: 105b6235ad0f ("cxl/port: Prevent out-of-order decoder allocation")
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> I also have a patch for this issue during I worked on deferred dport probing.
> 
> https://lore.kernel.org/linux-cxl/20250107035708.1134954-4-ming.li@zohomail.com/
> 
> I am not sure if we should define it as a bug, my understanding is
> that it will never happen with current implementation, it only happens
> when we are trying to delay the switch decoders initialization.

Agree. This is not a fix. The current implementation is lucky that ports
are always registered after decoders. So there is zero exposure to this
issue for current kernels.

Dave, please just include this as a prep patch in the same series that
invalidates the port child device ordering assumption.

Re: [PATCH] cxl/region: Add decoder check to check_commit_order()

[Dave Jiang]

On 4/30/25 10:29 PM, dan.j.williams@intel.com wrote:
> Li Ming wrote:
>> On 5/1/2025 5:36 AM, Dave Jiang wrote:
>>> check_commit_order() attempts to convert a device to a decoder without
>>> making sure the device is a decoder. So far this has been working due
>>> to pure luck. Issue discovered while doing deferred dport probing when
>>> child ports are now in the midst of decoders due to ordering change
>>> of child port additions. Add a check before attempting to do decoder
>>> conversion.
>>>
>>> Fixes: 105b6235ad0f ("cxl/port: Prevent out-of-order decoder allocation")
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>> I also have a patch for this issue during I worked on deferred dport probing.
>>
>> https://lore.kernel.org/linux-cxl/20250107035708.1134954-4-ming.li@zohomail.com/
>>
>> I am not sure if we should define it as a bug, my understanding is
>> that it will never happen with current implementation, it only happens
>> when we are trying to delay the switch decoders initialization.
> 
> Agree. This is not a fix. The current implementation is lucky that ports
> are always registered after decoders. So there is zero exposure to this
> issue for current kernels.
> 
> Dave, please just include this as a prep patch in the same series that
> invalidates the port child device ordering assumption.

will do