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: <20250224144155.omzrmls7hpjqw6yl@thinkpad>
Date: Mon, 24 Feb 2025 20:11:55 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Anand Moon <linux.amoon@...il.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	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

On Mon, Feb 24, 2025 at 07:33:37PM +0530, Anand Moon wrote:
> 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.

What do you mean by 'unhandled IRQs'? There is a dummy IRQ handler invoked to
handle these IRQs. Even your starfive_event_handler() that you proposed was
doing the same thing.

> Eliminating the dummy code eliminated the memory leak logs.

Sorry, this is not a valid justification. But as I said before, the change
itself (removing the dummy irq handler and related code) looks good to me as I
see no need for that. But I cannot accept it as a fix for the memleak.

- Mani

-- 
மணிவண்ணன் சதாசிவம்