Re: [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq

[Hans de Goede <hdegoede@redhat.com>]

Hi Lihze,

On 2/6/22 15:07, lizhe wrote:
> Memory allocated with this function is automatically
> freed on driver detach
> 
> Signed-off-by: lizhe <sensor1010@163.com>

You must use your real name (first-name + last-name) as author,
as well as in the Signed-off-by line, see:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 1c1ebf3dad43..aca59c6fdcbc 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -66,7 +66,7 @@ static inline int pciehp_request_irq(struct controller *ctrl)
>  	}
>  
>  	/* Installs the interrupt handler */
> -	retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist,
> +	retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
>  				      IRQF_SHARED, "pciehp", ctrl);
>  	if (retval)
>  		ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n",
> @@ -78,8 +78,6 @@ static inline void pciehp_free_irq(struct controller *ctrl)
>  {
>  	if (pciehp_poll_mode)
>  		kthread_stop(ctrl->poll_thread);
> -	else
> -		free_irq(ctrl->pcie->irq, ctrl);
>  }
>  
>  static int pcie_poll_cmd(struct controller *ctrl, int timeout)
> 

You cannot just go and change a single allocation into a devm managed
resource, esp. not a request_irq call.

Changing this into a devm_allocation means that the irq now will not be
free-ed until after pciehp_remove() has completed and that function calls:
pciehp_release_ctrl(ctrl); which releases the memory the ctrl pointer points
to and that same memory / pointer is used by pciehp_isr.

So after your patch it is possible for the IRQ to trigger after
pciehp_release_ctrl(ctrl) has free-ed the memory (and before the devm
framework calls free_irq() disabling the IRQ), causing pciehp_isr
to reference free-ed memory, leading to a crash.

Since this patch introduces a bug we cannot accept it, nack.

Regards,

Hans