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.
> 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).
>
> - 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.
> 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.
--
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
