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