Re: [PATCH] PCI: meson: Propagate devm_add_action_or_reset() failure

Re: [PATCH] PCI: meson: Propagate devm_add_action_or_reset() failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing `.remove` callback or `.suppress_bind_attrs = true` leaves PCI subsystem and PHY active after driver unbind.
--

commit fde9e1cd9d05730f72b074fa8edf3a1233d11384
Author: Shuvam Pandey <shuvampandey1@gmail.com>

PCI: meson: Propagate devm_add_action_or_reset() failure

This commit updates meson_pcie_probe_clock() to check the return value
of devm_add_action_or_reset(). If it fails, the error is returned so
that the probe error path can unwind normally.

> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 0694084f612b7..8d495bcc3a41a 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -204,7 +204,9 @@ static inline struct clk *meson_pcie_probe_clock(struct device *dev,
>  		return ERR_PTR(ret);
>  	}
>  
> -	devm_add_action_or_reset(dev, meson_pcie_disable_clock, clk);
> +	ret = devm_add_action_or_reset(dev, meson_pcie_disable_clock, clk);
> +	if (ret)
> +		return ERR_PTR(ret);

This isn't a bug introduced by this patch, but could this driver be unbound
via sysfs, triggering a use-after-free?

The meson_pcie_probe() function registers the PCI host controller via
dw_pcie_host_init(&pci->pp) and powers on the PHY via meson_pcie_power_on().

However, the driver does not implement a .remove callback to call
dw_pcie_host_deinit() and meson_pcie_power_off(), nor does it set
.suppress_bind_attrs = true in its platform_driver struct.

If the driver is unbound via /sys/bus/platform/drivers/meson-pcie/unbind,
the device core will trigger devres cleanup. This frees the mp structure,
unmaps IO memory, disables clocks, and releases resets.

Because the PCI subsystem is never explicitly unregistered via
dw_pcie_host_deinit(), the PCI core retains references to the destroyed root
bus and freed structures.

Can a subsequent access by the PCI subsystem or userspace cause a
use-after-free and system crash?

Additionally, the PHY is left permanently powered on without its required
clocks. Should the driver either implement a proper teardown in a .remove
hook or prevent unbinding by setting .suppress_bind_attrs = true?

>  
>  	return clk;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/177909148011.9588.6639767953842842291@gmail.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] PCI: meson: Propagate devm_add_action_or_reset() failure

[neil.armstrong]

On 5/18/26 10:20, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Missing `.remove` callback or `.suppress_bind_attrs = true` leaves PCI subsystem and PHY active after driver unbind.
> --
> 
> commit fde9e1cd9d05730f72b074fa8edf3a1233d11384
> Author: Shuvam Pandey <shuvampandey1@gmail.com>
> 
> PCI: meson: Propagate devm_add_action_or_reset() failure
> 
> This commit updates meson_pcie_probe_clock() to check the return value
> of devm_add_action_or_reset(). If it fails, the error is returned so
> that the probe error path can unwind normally.
> 
>> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
>> index 0694084f612b7..8d495bcc3a41a 100644
>> --- a/drivers/pci/controller/dwc/pci-meson.c
>> +++ b/drivers/pci/controller/dwc/pci-meson.c
>> @@ -204,7 +204,9 @@ static inline struct clk *meson_pcie_probe_clock(struct device *dev,
>>   		return ERR_PTR(ret);
>>   	}
>>   
>> -	devm_add_action_or_reset(dev, meson_pcie_disable_clock, clk);
>> +	ret = devm_add_action_or_reset(dev, meson_pcie_disable_clock, clk);
>> +	if (ret)
>> +		return ERR_PTR(ret);
> 
> This isn't a bug introduced by this patch, but could this driver be unbound
> via sysfs, triggering a use-after-free?
> 
> The meson_pcie_probe() function registers the PCI host controller via
> dw_pcie_host_init(&pci->pp) and powers on the PHY via meson_pcie_power_on().
> 
> However, the driver does not implement a .remove callback to call
> dw_pcie_host_deinit() and meson_pcie_power_off(), nor does it set
> .suppress_bind_attrs = true in its platform_driver struct.
> 
> If the driver is unbound via /sys/bus/platform/drivers/meson-pcie/unbind,
> the device core will trigger devres cleanup. This frees the mp structure,
> unmaps IO memory, disables clocks, and releases resets.
> 
> Because the PCI subsystem is never explicitly unregistered via
> dw_pcie_host_deinit(), the PCI core retains references to the destroyed root
> bus and freed structures.
> 
> Can a subsequent access by the PCI subsystem or userspace cause a
> use-after-free and system crash?

This probably a bug to fix, but totally unrelated to this change

> 
> Additionally, the PHY is left permanently powered on without its required
> clocks. Should the driver either implement a proper teardown in a .remove
> hook or prevent unbinding by setting .suppress_bind_attrs = true?
> 
>>   
>>   	return clk;
>>   }
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] PCI: meson: Propagate devm_add_action_or_reset() failure

[Neil Armstrong]

On 5/18/26 10:04, Shuvam Pandey wrote:
> meson_pcie_probe_clock() enables a clock and then registers a devres
> action to disable it during teardown. If devm_add_action_or_reset()
> fails, it runs the action immediately, disabling the clock.
> 
> The return value is currently ignored, so on that failure path
> meson_pcie_probe_clock() returns the disabled clock and probe continues.
> Return the error so the existing probe error path unwinds normally.
> 
> Fixes: 9c0ef6d34fdbf ("PCI: amlogic: Add the Amlogic Meson PCIe controller driver")
> Signed-off-by: Shuvam Pandey <shuvampandey1@gmail.com>
> ---
>   drivers/pci/controller/dwc/pci-meson.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 0694084f612b..8d495bcc3a41 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -204,7 +204,9 @@ static inline struct clk *meson_pcie_probe_clock(struct device *dev,
>   		return ERR_PTR(ret);
>   	}
>   
> -	devm_add_action_or_reset(dev, meson_pcie_disable_clock, clk);
> +	ret = devm_add_action_or_reset(dev, meson_pcie_disable_clock, clk);
> +	if (ret)
> +		return ERR_PTR(ret);
>   
>   	return clk;
>   }

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] PCI: meson: Propagate devm_add_action_or_reset() failure

[Manivannan Sadhasivam]

On Mon, 18 May 2026 13:49:40 +0545, Shuvam Pandey wrote:
> meson_pcie_probe_clock() enables a clock and then registers a devres
> action to disable it during teardown. If devm_add_action_or_reset()
> fails, it runs the action immediately, disabling the clock.
> 
> The return value is currently ignored, so on that failure path
> meson_pcie_probe_clock() returns the disabled clock and probe continues.
> Return the error so the existing probe error path unwinds normally.
> 
> [...]

Applied, thanks!

[1/1] PCI: meson: Propagate devm_add_action_or_reset() failure
      commit: b12341b98d5ac52f48ca1390e1e371aed81346c8

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic