Received-SPF: pass (google.com: domain of linux-kernel+bounces-87893-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: ie8e14432:Fastmail
Date: Fri, 1 Mar 2024 13:40:24 +0900
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Edmund Raile <edmund.raile@proton.me>
Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] firewire: ohci: prevent leak of left-over IRQ on
 unbind
Message-ID: <20240301044024.GA37429@workstation.local>
Mail-Followup-To: Edmund Raile <edmund.raile@proton.me>,
	linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
References: <20240229101236.8074-1-edmund.raile@proton.me>
 <20240229144723.13047-2-edmund.raile@proton.me>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240229144723.13047-2-edmund.raile@proton.me>

Hi,

Thanks for your taking the issue, and sending the patch. But I have a
concern about the change, since the allocated IRQ should be released as
the part of managed device resource[1]:

(include/linux/interrupt.h)
devm_request_irq()
  (kernel/irq/devres.c)
  ->devm_request_threaded_irq()
    ->devres_alloc(devm_irq_release)

(kernel/irq/devres.c)
devrm_irq_release()
->free_irq()

In my opinion, the devres mechanism releases the allocated memory when
releasing the data of associated device structure. In our case, it is
the data of pci_dev structure (precisely, the data of device structure
embedded in it). In the call trace of your commit comment, the release
should be done in:

(drivers/base/dd.c)
device_release_driver_internal()
->__device_release_driver()
  ->device_unbind_cleanup()
    (drivers/base/devres.c)
    ->devres_release_all(dev);
      ->release_nodes()
        (kernel/irq/devres.c)
	->free_irq()

However, you encounter the leak actually. I think we have another cause
for the leak, but never figured it out. Anyway, I'll further investigate
the issue.


[1] https://docs.kernel.org/driver-api/driver-model/devres.html

On Thu, Feb 29, 2024 at 02:47:59PM +0000, Edmund Raile wrote:
> 
> Commit 5a95f1ded28691e6 ("firewire: ohci: use devres for requested IRQ")
> also removed the call to free_irq() in pci_remove(), leading to a
> leftover irq of devm_request_irq() at pci_disable_msi() in pci_remove()
> when unbinding the driver from the device
> 
> remove_proc_entry: removing non-empty directory 'irq/136', leaking at
> least 'firewire_ohci'
> Call Trace:
>  ? remove_proc_entry+0x19c/0x1c0
>  ? __warn+0x81/0x130
>  ? remove_proc_entry+0x19c/0x1c0
>  ? report_bug+0x171/0x1a0
>  ? console_unlock+0x78/0x120
>  ? handle_bug+0x3c/0x80
>  ? exc_invalid_op+0x17/0x70
>  ? asm_exc_invalid_op+0x1a/0x20
>  ? remove_proc_entry+0x19c/0x1c0
>  unregister_irq_proc+0xf4/0x120
>  free_desc+0x3d/0xe0
>  ? kfree+0x29f/0x2f0
>  irq_free_descs+0x47/0x70
>  msi_domain_free_locked.part.0+0x19d/0x1d0
>  msi_domain_free_irqs_all_locked+0x81/0xc0
>  pci_free_msi_irqs+0x12/0x40
>  pci_disable_msi+0x4c/0x60
>  pci_remove+0x9d/0xc0 [firewire_ohci
>      01b483699bebf9cb07a3d69df0aa2bee71db1b26]
>  pci_device_remove+0x37/0xa0
>  device_release_driver_internal+0x19f/0x200
>  unbind_store+0xa1/0xb0
> 
> remove irq with devm_free_irq() before pci_disable_msi()
> also remove it in fail_msi: of pci_probe() as this would lead to
> an identical leak
> 
> Fixes: 5a95f1ded28691e6 ("firewire: ohci: use devres for requested IRQ")
> 
> Signed-off-by: Edmund Raile <edmund.raile@proton.me>
> 
> ---
> 
> Using FW643 with vfio-pci required unbinding from firewire_ohci,
> doing so currently produces a memory leak due to a leftover irq
> which this patch removes.
> 
> The irq can be observed while the driver is loaded and bound:
> find /proc/irq -type d -name "firewire_ohci"
> 
> Is it a good idea to submit a patch to devm_request_irq() in
> include/linux/interrupt.h to add the function comment
> /*
>  * counterpart: devm_free_irq()
>  */
> so LSPs show that hint?
> 
> v2 change: corrected patch title
> 
>  drivers/firewire/ohci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 9db9290c3269..7bc71f4be64a 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -3773,6 +3773,7 @@ static int pci_probe(struct pci_dev *dev,
>  	return 0;
>  
>   fail_msi:
> +	devm_free_irq(&dev->dev, dev->irq, ohci);
>  	pci_disable_msi(dev);
>  
>  	return err;
> @@ -3800,6 +3801,7 @@ static void pci_remove(struct pci_dev *dev)
>  
>  	software_reset(ohci);
>  
> +	devm_free_irq(&dev->dev, dev->irq, ohci);
>  	pci_disable_msi(dev);
>  
>  	dev_notice(&dev->dev, "removing fw-ohci device\n");
> -- 
> 2.43.0


Thanks

Takashi Sakamoto