| 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
| ||
|
Message-ID: <20200226065057.GT3335@pengutronix.de>
Date: Wed, 26 Feb 2020 07:50:57 +0100
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Sasha Levin <sashal@...nel.org>
Cc: Andreas Tobler <andreas.tobler@...ay.ch>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"stable@...r.kernel.org" <stable@...r.kernel.org>,
Robin Gong <yibin.gong@....com>, Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH 5.4 160/344] dmaengine: imx-sdma: Fix memory leak
On Mon, Feb 24, 2020 at 04:23:19PM -0500, Sasha Levin wrote:
> On Mon, Feb 24, 2020 at 03:57:18PM +0100, Sascha Hauer wrote:
> > On Mon, Feb 24, 2020 at 01:24:04PM +0000, Andreas Tobler wrote:
> > > Hi all,
> > >
> > > On 21.02.20 08:39, Greg Kroah-Hartman wrote:
> > > > From: Sascha Hauer <s.hauer@...gutronix.de>
> > > >
> > > > [ Upstream commit 02939cd167095f16328a1bd5cab5a90b550606df ]
> > > >
> > > > The current descriptor is not on any list of the virtual DMA channel.
> > > > Once sdma_terminate_all() is called when a descriptor is currently
> > > > in flight then this one is forgotten to be freed. We have to call
> > > > vchan_terminate_vdesc() on this descriptor to re-add it to the lists.
> > > > Now that we also free the currently running descriptor we can (and
> > > > actually have to) remove the current descriptor from its list also
> > > > for the cyclic case.
> > > >
> > > > Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
> > > > Reviewed-by: Robin Gong <yibin.gong@....com>
> > > > Tested-by: Robin Gong <yibin.gong@....com>
> > > > Link: https://lore.kernel.org/r/20191216105328.15198-10-s.hauer@pengutronix.de
> > > > Signed-off-by: Vinod Koul <vkoul@...nel.org>
> > > > Signed-off-by: Sasha Levin <sashal@...nel.org>
> > > > ---
> > > > drivers/dma/imx-sdma.c | 19 +++++++++++--------
> > > > 1 file changed, 11 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > > index c27e206a764c3..66f1b2ac5cde4 100644
> > > > --- a/drivers/dma/imx-sdma.c
> > > > +++ b/drivers/dma/imx-sdma.c
> > > > @@ -760,12 +760,8 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
> > > > return;
> > > > }
> > > > sdmac->desc = desc = to_sdma_desc(&vd->tx);
> > > > - /*
> > > > - * Do not delete the node in desc_issued list in cyclic mode, otherwise
> > > > - * the desc allocated will never be freed in vchan_dma_desc_free_list
> > > > - */
> > > > - if (!(sdmac->flags & IMX_DMA_SG_LOOP))
> > > > - list_del(&vd->node);
> > > > +
> > > > + list_del(&vd->node);
> > > >
> > > > sdma->channel_control[channel].base_bd_ptr = desc->bd_phys;
> > > > sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
> > > > @@ -1071,7 +1067,6 @@ static void sdma_channel_terminate_work(struct work_struct *work)
> > > >
> > > > spin_lock_irqsave(&sdmac->vc.lock, flags);
> > > > vchan_get_all_descriptors(&sdmac->vc, &head);
> > > > - sdmac->desc = NULL;
> > > > spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > > > vchan_dma_desc_free_list(&sdmac->vc, &head);
> > > > sdmac->context_loaded = false;
> > > > @@ -1080,11 +1075,19 @@ static void sdma_channel_terminate_work(struct work_struct *work)
> > > > static int sdma_disable_channel_async(struct dma_chan *chan)
> > > > {
> > > > struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&sdmac->vc.lock, flags);
> > > >
> > > > sdma_disable_channel(chan);
> > > >
> > > > - if (sdmac->desc)
> > > > + if (sdmac->desc) {
> > > > + vchan_terminate_vdesc(&sdmac->desc->vd);
> > > > + sdmac->desc = NULL;
> > > > schedule_work(&sdmac->terminate_worker);
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > > >
> > > > return 0;
> > > > }
> > > >
> > >
> > > This patch breaks our imx6 board with the attached trace. Reverting the
> > > patch makes it boot again.
> > > I tried also 5.6-rc3 and it booted too. A closer look into imx-sdma.c
> > > from 5.6-rc3 showed me some details which might have to be backported as
> > > well to make this patch work.
> > > I tried a1ff6a07f5a3951fcac84f064a76d1ad79c10e40 and was somehow
> > > successful. I still have one trace but the board boots now.
> > >
> > > Any insights from the experts?
> >
> > This series should be applied as a whole or not, only 7/9 is optional.
> >
> > It seems I have to avoid the trigger word "fix" in my commit messages or
> > make sure these patches won't apply without their dependencies :-/
>
> Or you could just tag the dependencies so that we could take all of them
> as well? We have a nice "Depends-on:" tag that makes it easy.
>
> As with everything in life, you want to communicate more effectively
> rather than not communicate at all.
Speaking of which, if you want people to use that "Depends-on:" tag you
should spread the word about it. It was used for 30 commits in the whole
Kernel history and Documentation/ doesn't mention it at all.
Anyway, this helps only for patches from which I actually know the
dependencies. I knew them this time, because the whole series only had
the purpose of making ground for the patch. Often enough I don't know
them putting a patch onto a kernel just because it applies cleanly
doesn't seem like a good idea to me.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists