| Message ID | 1511191578-1781-1-git-send-email-awallis@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 11/20/2017 10:26 AM, Adam Wallis wrote: > Commit adfa543e7314 ("dmatest: don't use set_freezable_with_signal()") > introduced a bug (that is in fact documented by the patch commit text) > that leaves behind a dangling pointer. Since the done_wait structure is > allocated on the stack, future invocations to the DMATEST can produce > undesirable results (e.g., corrupted spinlocks). > > Commit a9df21e34b42 ("dmaengine: dmatest: warn user when dma test times > out") attempted to WARN the user that the stack was likely corrupted but > did not fix the actual issue. > > This patch fixes the issue by pushing the wait queue and callback > structs into the the thread structure. If a failure occurs due to time, > dmaengine_terminate_all will force the callback to safely call > wake_up_all() without possibility of using a freed pointer. > > Cc: stable@vger.kernel.org FYI, I removed the other CC's to stable, since I am actively working on upstreaming the dependency into all the stable branches. > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=197605 > Fixes: adfa543e7314 ("dmatest: don't use set_freezable_with_signal()") > Reviewed-by: Sinan Kaya <okaya@codeaurora.org> > Suggested-by: Shunyong Yang <shunyong.yang@hxt-semitech.com> > Signed-off-by: Adam Wallis <awallis@codeaurora.org> > --- > changes from v3: Added check to thread wait variable if terminate_all fails > changes from v2: Added "Fixes" tag > changes from v1: Added pre-req patches for stable > > drivers/dma/dmatest.c | 55 +++++++++++++++++++++++++++++---------------------- > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c > index 47edc7f..45a4ee9 100644 [..]
Hi Adam,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on next-20171122]
[cannot apply to v4.14]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Adam-Wallis/dmaengine-dmatest-move-callback-wait-queue-to-thread-context/20171123-101707
config: i386-randconfig-x001-201747 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from include/linux/delay.h:22,
from drivers//dma/dmatest.c:13:
drivers//dma/dmatest.c: In function 'dmatest_callback':
include/linux/compiler.h:319:38: error: call to '__compiletime_assert_358' declared with attribute error: pointer type mismatch in container_of()
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^
include/linux/compiler.h:299:4: note: in definition of macro '__compiletime_assert'
prefix ## suffix(); \
^~~~~~
include/linux/compiler.h:319:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:47:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/kernel.h:930:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~~~~~~~~~~~~~
>> drivers//dma/dmatest.c:358:3: note: in expansion of macro 'container_of'
container_of(done, struct dmatest_thread, done_wait);
^~~~~~~~~~~~
vim +/container_of +358 drivers//dma/dmatest.c
352
353
354 static void dmatest_callback(void *arg)
355 {
356 struct dmatest_done *done = arg;
357 struct dmatest_thread *thread =
> 358 container_of(done, struct dmatest_thread, done_wait);
359 if (!thread->done) {
360 done->done = true;
361 wake_up_all(done->wait);
362 } else {
363 /*
364 * If thread->done, it means that this callback occurred
365 * after the parent thread has cleaned up. This can
366 * happen in the case that driver doesn't implement
367 * the terminate_all() functionality and a dma operation
368 * did not occur within the timeout period
369 */
370 WARN(1, "dmatest: Kernel memory may be corrupted!!\n");
371 }
372 }
373
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Adam,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on next-20171122]
[cannot apply to v4.14]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Adam-Wallis/dmaengine-dmatest-move-callback-wait-queue-to-thread-context/20171123-101707
config: tile-allmodconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile
All errors (new ones prefixed by >>):
drivers//dma/dmatest.c: In function 'dmatest_callback':
>> drivers//dma/dmatest.c:358:3: error: call to '__compiletime_assert_358' declared with attribute error: pointer type mismatch in container_of()
vim +/__compiletime_assert_358 +358 drivers//dma/dmatest.c
352
353
354 static void dmatest_callback(void *arg)
355 {
356 struct dmatest_done *done = arg;
357 struct dmatest_thread *thread =
> 358 container_of(done, struct dmatest_thread, done_wait);
359 if (!thread->done) {
360 done->done = true;
361 wake_up_all(done->wait);
362 } else {
363 /*
364 * If thread->done, it means that this callback occurred
365 * after the parent thread has cleaned up. This can
366 * happen in the case that driver doesn't implement
367 * the terminate_all() functionality and a dma operation
368 * did not occur within the timeout period
369 */
370 WARN(1, "dmatest: Kernel memory may be corrupted!!\n");
371 }
372 }
373
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c index 47edc7f..45a4ee9 100644 --- a/drivers/dma/dmatest.c +++ b/drivers/dma/dmatest.c @@ -155,6 +155,12 @@ struct dmatest_params { #define PATTERN_COUNT_MASK 0x1f #define PATTERN_MEMSET_IDX 0x01 +/* poor man's completion - we want to use wait_event_freezable() on it */ +struct dmatest_done { + bool done; + wait_queue_head_t *wait; +}; + struct dmatest_thread { struct list_head node; struct dmatest_info *info; @@ -165,6 +171,8 @@ struct dmatest_thread { u8 **dsts; u8 **udsts; enum dma_transaction_type type; + wait_queue_head_t done_wait; + struct dmatest_done test_done; bool done; }; @@ -342,18 +350,25 @@ static unsigned int dmatest_verify(u8 **bufs, unsigned int start, return error_count; } -/* poor man's completion - we want to use wait_event_freezable() on it */ -struct dmatest_done { - bool done; - wait_queue_head_t *wait; -}; static void dmatest_callback(void *arg) { struct dmatest_done *done = arg; - - done->done = true; - wake_up_all(done->wait); + struct dmatest_thread *thread = + container_of(done, struct dmatest_thread, done_wait); + if (!thread->done) { + done->done = true; + wake_up_all(done->wait); + } else { + /* + * If thread->done, it means that this callback occurred + * after the parent thread has cleaned up. This can + * happen in the case that driver doesn't implement + * the terminate_all() functionality and a dma operation + * did not occur within the timeout period + */ + WARN(1, "dmatest: Kernel memory may be corrupted!!\n"); + } } static unsigned int min_odd(unsigned int x, unsigned int y) @@ -424,9 +439,8 @@ static unsigned long long dmatest_KBs(s64 runtime, unsigned long long len) */ static int dmatest_func(void *data) { - DECLARE_WAIT_QUEUE_HEAD_ONSTACK(done_wait); struct dmatest_thread *thread = data; - struct dmatest_done done = { .wait = &done_wait }; + struct dmatest_done *done = &thread->test_done; struct dmatest_info *info; struct dmatest_params *params; struct dma_chan *chan; @@ -673,9 +687,9 @@ static int dmatest_func(void *data) continue; } - done.done = false; + done->done = false; tx->callback = dmatest_callback; - tx->callback_param = &done; + tx->callback_param = done; cookie = tx->tx_submit(tx); if (dma_submit_error(cookie)) { @@ -688,21 +702,12 @@ static int dmatest_func(void *data) } dma_async_issue_pending(chan); - wait_event_freezable_timeout(done_wait, done.done, + wait_event_freezable_timeout(thread->done_wait, done->done, msecs_to_jiffies(params->timeout)); status = dma_async_is_tx_complete(chan, cookie, NULL, NULL); - if (!done.done) { - /* - * We're leaving the timed out dma operation with - * dangling pointer to done_wait. To make this - * correct, we'll need to allocate wait_done for - * each test iteration and perform "who's gonna - * free it this time?" dancing. For now, just - * leave it dangling. - */ - WARN(1, "dmatest: Kernel stack may be corrupted!!\n"); + if (!done->done) { dmaengine_unmap_put(um); result("test timed out", total_tests, src_off, dst_off, len, 0); @@ -789,7 +794,7 @@ static int dmatest_func(void *data) dmatest_KBs(runtime, total_len), ret); /* terminate all transfers on specified channels */ - if (ret) + if (ret || failed_tests) dmaengine_terminate_all(chan); thread->done = true; @@ -849,6 +854,8 @@ static int dmatest_add_threads(struct dmatest_info *info, thread->info = info; thread->chan = dtc->chan; thread->type = type; + thread->test_done.wait = &thread->done_wait; + init_waitqueue_head(&thread->done_wait); smp_wmb(); thread->task = kthread_create(dmatest_func, thread, "%s-%s%u", dma_chan_name(chan), op, i);
Commit adfa543e7314 ("dmatest: don't use set_freezable_with_signal()") introduced a bug (that is in fact documented by the patch commit text) that leaves behind a dangling pointer. Since the done_wait structure is allocated on the stack, future invocations to the DMATEST can produce undesirable results (e.g., corrupted spinlocks). Commit a9df21e34b42 ("dmaengine: dmatest: warn user when dma test times out") attempted to WARN the user that the stack was likely corrupted but did not fix the actual issue. This patch fixes the issue by pushing the wait queue and callback structs into the the thread structure. If a failure occurs due to time, dmaengine_terminate_all will force the callback to safely call wake_up_all() without possibility of using a freed pointer. Cc: stable@vger.kernel.org Bug: https://bugzilla.kernel.org/show_bug.cgi?id=197605 Fixes: adfa543e7314 ("dmatest: don't use set_freezable_with_signal()") Reviewed-by: Sinan Kaya <okaya@codeaurora.org> Suggested-by: Shunyong Yang <shunyong.yang@hxt-semitech.com> Signed-off-by: Adam Wallis <awallis@codeaurora.org> --- changes from v3: Added check to thread wait variable if terminate_all fails changes from v2: Added "Fixes" tag changes from v1: Added pre-req patches for stable drivers/dma/dmatest.c | 55 +++++++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 24 deletions(-)