Re: [PATCH v3] dmatest: prevent memory leakage on error path in thread

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2014-08-21 at 09:56 -0700, Dan Williams wrote:
> Apologies for the long delay on this, I've been distracted from
> dmaengine for quite a while...
> 
> On Thu, Jun 26, 2014 at 7:56 AM, Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > When we fail to allocate memory for thread->srcs or thread->dsts and src_cnt or
> > dst_cnt great than 1 we leak memory on error path. This patch fixes the issue.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> > Since v2:
> > - properly testing when noverify is unset (my bad I didn't do that previously)
> >  drivers/dma/dmatest.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> > index 740013d..aa0ac14 100644
> > --- a/drivers/dma/dmatest.c
> > +++ b/drivers/dma/dmatest.c
> > @@ -441,7 +441,7 @@ static int dmatest_func(void *data)
> >                 src_cnt = min_odd(params->pq_sources | 1, dma_maxpq(dev, 0));
> >                 dst_cnt = 2;
> >
> > -               pq_coefs = kmalloc(params->pq_sources+1, GFP_KERNEL);
> > +               pq_coefs = kmalloc(params->pq_sources + 1, GFP_KERNEL);
> >                 if (!pq_coefs)
> >                         goto err_thread_type;
> >
> > @@ -450,7 +450,7 @@ static int dmatest_func(void *data)
> >         } else
> >                 goto err_thread_type;
> >
> > -       thread->srcs = kcalloc(src_cnt+1, sizeof(u8 *), GFP_KERNEL);
> > +       thread->srcs = kcalloc(src_cnt + 1, sizeof(u8 *), GFP_KERNEL);
> 
> I'd still prefer to not see these whitespace changes in this patch,
> checkpatch doesn't care.

It's minor, no problem with that.

> 
> >         if (!thread->srcs)
> >                 goto err_srcs;
> >         for (i = 0; i < src_cnt; i++) {
> > @@ -458,9 +458,8 @@ static int dmatest_func(void *data)
> >                 if (!thread->srcs[i])
> >                         goto err_srcbuf;
> >         }
> > -       thread->srcs[i] = NULL;
> 
> Hmm, yes, this is already taken care of by the side effect of
> kcalloc() zeroing all entries, but I like how it makes it explicit
> that the last value in the array has special meaning.  Leaving
> unnecessary changes out of "fix" patches also makes them easier to
> backport to older kernels (less opportunities for collisions).

I got it.

> 
> >
> > -       thread->dsts = kcalloc(dst_cnt+1, sizeof(u8 *), GFP_KERNEL);
> > +       thread->dsts = kcalloc(dst_cnt + 1, sizeof(u8 *), GFP_KERNEL);
> >         if (!thread->dsts)
> >                 goto err_dsts;
> >         for (i = 0; i < dst_cnt; i++) {
> > @@ -468,7 +467,6 @@ static int dmatest_func(void *data)
> >                 if (!thread->dsts[i])
> >                         goto err_dstbuf;
> >         }
> > -       thread->dsts[i] = NULL;
> 
> Same comment here, dma_init_dsts() looks for a NULL entry as the
> termination sentinel which is not obvious... also, I suspect this is
> the reason v1 and v2 accidentally deleted the +1 when allocating
> "dsts" and "srcs"
> 
> >
> >         set_user_nice(current, 10);
> >
> > @@ -751,14 +749,17 @@ static int dmatest_func(void *data)
> >         runtime = ktime_us_delta(ktime_get(), ktime);
> >
> >         ret = 0;
> > -       for (i = 0; thread->dsts[i]; i++)
> > -               kfree(thread->dsts[i]);
> > +
> > +       i = dst_cnt + 1;
> >  err_dstbuf:
> > +       while (--i >= 0)
> > +               kfree(thread->dsts[i]);
> 
> What was wrong with old method of looping until we find a NULL entry?
> Mirrors the init case and no need to initialize i, in fact
> initializing i to dst_cnt+1 means we needlessly call kfree() on a
> known NULL pointer.

Actually nothing wrong with the method.
Summarize all comments together the patch degenerate into moving of two
labels. Right?

> 
> >         kfree(thread->dsts);
> >  err_dsts:
> > -       for (i = 0; thread->srcs[i]; i++)
> > -               kfree(thread->srcs[i]);
> > +       i = src_cnt + 1;
> >  err_srcbuf:
> > +       while (--i >= 0)
> > +               kfree(thread->srcs[i]);
> 
> Same comment.


-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux