Re: [PATCH v19 13/14] PCI/CXL: Mask/Unmask CXL protocol errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Setting `ras = NULL` on devm_add_action_or_reset failure causes a deterministic kernel panic on subsequent uncorrectable errors.
- [High] Root port `dport->regs.ras` is exposed to concurrent error handlers before it is mapped, leading to a kernel panic.
--

--- Patch [13]: [PATCH v19 13/14] PCI/CXL: Mask/Unmask CXL protocol errors ---
commit 882744bb847d3cd61f8f100025ea14b41fb68044
Author: Terry Bowman <terry.bowman@amd.com>

PCI/CXL: Mask/Unmask CXL protocol errors

CXL protocol errors must be unmasked to be reported. Add
pci_aer_mask_internal_errors() and pci_aer_unmask_internal_errors() helpers
to mask and unmask internal errors, and register a devm action to restore
the mask on teardown.

> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 0254b7ec64c30..40a8a03070652 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -114,16 +114,64 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work)
[ ... ]
>  static void cxl_dport_map_ras(struct cxl_dport *dport)
>  {
>  	struct cxl_register_map *map = &dport->reg_map;
>  	struct device *dev = dport->dport_dev;
>  
> -	if (!map->component_map.ras.valid)
> +	if (!map->component_map.ras.valid) {
>  		dev_dbg(dev, "RAS registers not found\n");
> -	else if (cxl_map_component_regs(map, &dport->regs.component,
> -					BIT(CXL_CM_CAP_CAP_ID_RAS)))
> +		return;
> +	}
> +
> +	if (cxl_map_component_regs(map, &dport->regs.component,
> +				   BIT(CXL_CM_CAP_CAP_ID_RAS))) {
>  		dev_dbg(dev, "Failed to map RAS capability.\n");
> +		return;
> +	}
> +
> +	if (!dev_is_pci(dev))
> +		return;
> +
> +	cxl_unmask_proto_interrupts(dev);
> +	if (devm_add_action_or_reset(dport_to_host(dport),
> +				     cxl_mask_proto_irqs, dev)) {
> +		dev_warn(dev, "failed to defer CXL proto-irq mask; CXL protocol error reporting disabled\n");
> +		dport->regs.component.ras = NULL;

[Severity: High]
Does setting ras to NULL here risk a kernel panic?

If a CXL protocol error triggers in the brief window between unmasking
and devm_add_action_or_reset() masking it again on failure, the AER IRQ 
handler will enqueue the error to the kfifo.

When the kfifo worker dequeues the error and calls cxl_do_recovery(),
will it encounter a NULL ras_base and unconditionally call 
panic("CXL: UCE with unmapped RAS registers")?

> +	}
>  }
[ ... ]
> @@ -165,8 +211,21 @@ void devm_cxl_port_ras_setup(struct cxl_port *port)
>  
>  	map->host = &port->dev;
>  	if (cxl_map_component_regs(map, &port->regs,
> -				   BIT(CXL_CM_CAP_CAP_ID_RAS)))
> +				   BIT(CXL_CM_CAP_CAP_ID_RAS))) {
>  		dev_dbg(&port->dev, "Failed to map RAS capability\n");
> +		return;
> +	}
> +
> +	dev = is_cxl_endpoint(port) ? port->uport_dev->parent : port->uport_dev;
> +	if (!dev_is_pci(dev))
> +		return;
> +
> +	cxl_unmask_proto_interrupts(dev);
> +	if (devm_add_action_or_reset(&port->dev, cxl_mask_proto_irqs, dev)) {
> +		dev_warn(&port->dev,
> +			 "failed to defer CXL proto-irq mask; CXL protocol error reporting disabled\n");
> +		port->regs.ras = NULL;

[Severity: High]
Can setting ras to NULL here also cause a panic?

Similar to the dport case, if an error is triggered and logged before the
devm action masks the interrupts again, will cxl_do_recovery() panic when it
accesses the NULL ras_base?

> +	}
>  }

[Severity: High]
Can exposing a partially initialized dport cause a panic?

Looking at __devm_cxl_add_dport() in drivers/cxl/core/port.c, the dport is 
published to the xarray before its RAS registers are mapped:

drivers/cxl/core/port.c:__devm_cxl_add_dport() {
	...
	rc = add_dport(port, dport);
	if (rc)
		goto err;
	...
	devm_cxl_dport_ras_setup(dport);
	...
}

If firmware left CXL protocol errors unmasked, or if an error is already
pending in the kfifo, the kfifo consumer can execute concurrently. It
successfully looks up the partially initialized dport in the xarray and
passes it to cxl_do_recovery().

Because the mapping hasn't completed, to_ras_base() returns NULL, resulting
in an immediate panic("CXL: UCE with unmapped RAS registers").

Since cxl_do_recovery() was introduced earlier in this series, does this
make the race fatal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803221810.3685703-1-terry.bowman@amd.com?part=13