Re: [PATCH 2/4] EDAC/altera: use flag to differentiate 64-bit platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] A syntax error prevents compilation due to a typographical error in variable initialization.

Pre-existing issues:
- [High] The IRQ domain is leaked and chained IRQ handlers are left dangling on probe error paths.
- [High] The driver registers a panic notifier but never unregisters it, leading to a Use-After-Free on unbind.
--

--- Patch [2]: [PATCH 2/4] EDAC/altera: use flag to differentiate 64-bit platforms ---
commit 4db36eaf83f8ef9e44a35979b3fccecc08fabc69
Author: Dinh Nguyen <dinguyen@kernel.org>
Subject: EDAC/altera: use flag to differentiate 64-bit platforms

This commit introduces a flag to differentiate platforms like Stratix10 and
Agilex5 by storing it in the driver match data. It updates the device setup
and probe functions to check this flag instead of the boolean is_s10 field.

> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -1507,7 +1507,7 @@ static int altr_portb_setup(struct altr_edac_device_dev *device)
>  	int edac_idx, rc;
>  	struct device_node *np;
>  	const struct edac_device_prv_data *prv = &a10_sdmmceccb_data;
> -	bool is_s10 = device->edac->is_s10;
> +	unsigned long flag = {unsigned long)device->edac->flag;

[Severity: Low]
Is there a syntax error in this type cast? Using a curly brace instead of a
parenthesis causes a build failure.

[ ... ]

> @@ -2152,8 +2152,8 @@ static int altr_edac_a10_probe(struct platform_device *pdev)
>  	irq_set_chained_handler_and_data(edac->sb_irq,
>  					 altr_edac_a10_irq_handler,
>  					 edac);
> -	if (edac->is_s10)
> -	{
> +
> +	if (edac->flag == SOCFPGA_S10) {

[Severity: High]
This is a pre-existing issue, but does this probe error path leak the chained
IRQ handler and IRQ domain?

In altr_edac_a10_probe(), irq_set_chained_handler_and_data() is called for
sb_irq. If a later step fails, like fetching db_irq:

	edac->db_irq = platform_get_irq(pdev, 1);
	if (edac->db_irq < 0)
		return edac->db_irq;

The probe function returns the error directly without unregistering the chained
handler. Since the edac structure is devm-allocated, it gets freed on probe
failure.

If the interrupt triggers later, could this cause a use-after-free when the
handler executes with a dangling edac pointer?

>  		int dberror, err_addr;
>  
>  		edac->panic_notifier.notifier_call = s10_edac_dberr_handler;
>  		atomic_notifier_chain_register(&panic_notifier_list,
>  					       &edac->panic_notifier);

[Severity: High]
This is a pre-existing issue, but does this driver ever unregister its panic
notifier?

The edac structure is devm-allocated, but I don't see a devm action or a 
.remove callback to unregister the panic_notifier.

If the driver is unbound via sysfs, devres will free the edac structure,
leaving a dangling pointer in panic_notifier_list.

Could a subsequent panic attempt to call the freed notifier block and trigger
a nested panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720201425.1538771-1-dinguyen@kernel.org?part=2