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 Fri, Aug 22, 2014 at 2:07 AM, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> 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?

I believe so, yes.
--
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