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
