linux-kernel - Re: [PATCH v1] PCI: starfive: Fix kmemleak in StarFive PCIe driver's IRQ handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANAwSgSdEr0X0F1AFAUfJoEjT1a63nj5Ar-ZfmehfhnE0=v+CA@mail.gmail.com>
Date: Mon, 24 Feb 2025 19:33:37 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Kevin Xie <kevin.xie@...rfivetech.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Conor Dooley <conor.dooley@...rochip.com>, Minda Chen <minda.chen@...rfivetech.com>, 
	"open list:PCIE DRIVER FOR STARFIVE JH71x0" <linux-pci@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] PCI: starfive: Fix kmemleak in StarFive PCIe driver's
 IRQ handling

Hi Manivannan

On Mon, 24 Feb 2025 at 17:24, Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org> wrote:
>
> On Mon, Feb 24, 2025 at 03:38:29PM +0530, Anand Moon wrote:
> > Hi Manivannan
> >
> > On Mon, 24 Feb 2025 at 13:31, Manivannan Sadhasivam
> > <manivannan.sadhasivam@...aro.org> wrote:
> > >
> > > On Thu, Feb 20, 2025 at 03:53:31PM +0530, Anand Moon wrote:
> > >
> > > [...]
> > >
> > > > Following the change fix this warning in a kernel memory leak.
> > > > Would you happen to have any comments on these changes?
> > > >
> > > > diff --git a/drivers/pci/controller/plda/pcie-plda-host.c
> > > > b/drivers/pci/controller/plda/pcie-plda-host.c
> > > > index 4153214ca410..5a72a5a33074 100644
> > > > --- a/drivers/pci/controller/plda/pcie-plda-host.c
> > > > +++ b/drivers/pci/controller/plda/pcie-plda-host.c
> > > > @@ -280,11 +280,6 @@ static u32 plda_get_events(struct plda_pcie_rp *port)
> > > >         return events;
> > > >  }
> > > >
> > > > -static irqreturn_t plda_event_handler(int irq, void *dev_id)
> > > > -{
> > > > -       return IRQ_HANDLED;
> > > > -}
> > > > -
> > > >  static void plda_handle_event(struct irq_desc *desc)
> > > >  {
> > > >         struct plda_pcie_rp *port = irq_desc_get_handler_data(desc);
> > > > @@ -454,13 +449,10 @@ int plda_init_interrupts(struct platform_device *pdev,
> > > >
> > > >                 if (event->request_event_irq)
> > > >                         ret = event->request_event_irq(port, event_irq, i);
> > > > -               else
> > > > -                       ret = devm_request_irq(dev, event_irq,
> > > > -                                              plda_event_handler,
> > > > -                                              0, NULL, port);
> > >
> > > This change is not related to the memleak. But I'd like to have it in a separate
> > > patch since this code is absolutely not required, rather pointless.
> > >
> > Yes, remove these changes to fix the memory leak issue I observed.
> >
>
> Sorry, I don't get you. This specific code change of removing 'devm_request_irq'
> is not supposed to fix memleak.
>
> If you are seeing the memleak getting fixed because of it, then something is
> wrong with the irq implementation. You need to figure it out.

Declaring request_event_irq in the host controller facilitates the
creation of a dedicated IRQ event handler.
In its absence, a dummy devm_request_irq was employed, but this
resulted in unhandled IRQs and subsequent memory leaks.
Eliminating the dummy code eliminated the memory leak logs.
>
> > > >
> > > >                 if (ret) {
> > > >                         dev_err(dev, "failed to request IRQ %d\n", event_irq);
> > > > +                       irq_dispose_mapping(event_irq);
> > >
> > > So the issue overall is that the 'devm_request_irq' fails on your platform
> > > because the interrupts are not defined in DT and then the IRQ is not disposed in
> > > the error path?
> > >
> > On microchip PCIe driver utilizes interrupt resources from its
> > "bridge" and "cntrl"
> > components, accessed via ioremap, to handle hardware events.
> > These resources are absent in the StarFive PCIe controller.
> >
> > While the StarFive JH-7110 datasheet [1] indicates support for PME, MSI, and INT
> > messages and specific implementation details for leveraging these interrupts are
> > unavailable.
> >
> > As per StarFive JH-7110 Datasheet PCI support [1]
> > 1 Power Management Event (PME message) ◦
> > 2  MSI (up to 32) and INT message support
> >
> > [1] https://doc-en.rvspace.org/JH7110/PDF/JH7110I_DS.pdf
> >
>
> Fine.
>
> > > In that case, none of the error paths below for_each_set_bit() loop is disposing
> > > the IRQs. Also, calling 'irq_dispose_mapping()' only once is not going to
> > > dispose all mappings that were created before in the loop.
> >
> > I was looking at how the IRQ error path will clean up IRQ in case of failure
> > hence, I added this in case of failure to unmap IRQ events,
> > I will drop this if not required.
>
> Absolutely not. These are fixing the irq leaks. But if it is not related to the
> 'memleak' you observed, then these should be part of a separate patch.
>
Ok will drop this.
> - Mani
>
Thank
-Anand
> --
> மணிவண்ணன் சதாசிவம்