| From 6f6a23a213be51728502b88741ba6a10cda2441d Mon Sep 17 00:00:00 2001 |
| From: Adam Wallis <awallis@codeaurora.org> |
| Date: Mon, 27 Nov 2017 10:45:01 -0500 |
| Subject: dmaengine: dmatest: move callback wait queue to thread context |
| |
| From: Adam Wallis <awallis@codeaurora.org> |
| |
| commit 6f6a23a213be51728502b88741ba6a10cda2441d upstream. |
| |
| 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. |
| |
| 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> |
| Signed-off-by: Vinod Koul <vinod.koul@intel.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/dma/dmatest.c | 55 ++++++++++++++++++++++++++++---------------------- |
| 1 file changed, 31 insertions(+), 24 deletions(-) |
| |
| --- a/drivers/dma/dmatest.c |
| +++ b/drivers/dma/dmatest.c |
| @@ -158,6 +158,12 @@ MODULE_PARM_DESC(run, "Run the test (def |
| #define PATTERN_OVERWRITE 0x20 |
| #define PATTERN_COUNT_MASK 0x1f |
| |
| +/* 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; |
| @@ -166,6 +172,8 @@ struct dmatest_thread { |
| u8 **srcs; |
| u8 **dsts; |
| enum dma_transaction_type type; |
| + wait_queue_head_t done_wait; |
| + struct dmatest_done test_done; |
| bool done; |
| }; |
| |
| @@ -326,18 +334,25 @@ static unsigned int dmatest_verify(u8 ** |
| 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(arg, 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) |
| @@ -408,9 +423,8 @@ static unsigned long long dmatest_KBs(s6 |
| */ |
| 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; |
| @@ -637,9 +651,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)) { |
| @@ -652,21 +666,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); |
| @@ -747,7 +752,7 @@ err_thread_type: |
| 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; |
| @@ -807,6 +812,8 @@ static int dmatest_add_threads(struct dm |
| 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); |