Skip to content

Commit

Permalink
dmaengine: dmatest: move callback wait queue to thread context
Browse files Browse the repository at this point in the history
Commit adfa543 ("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 a9df21e ("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: adfa543 ("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>
  • Loading branch information
Adam Wallis authored and Vinod Koul committed Dec 11, 2017
1 parent 62a277d commit 6f6a23a
Showing 1 changed file with 31 additions and 24 deletions.
55 changes: 31 additions & 24 deletions drivers/dma/dmatest.c
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@ MODULE_PARM_DESC(run, "Run the test (default: false)");
#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;
Expand All @@ -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;
};

Expand Down Expand Up @@ -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(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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 6f6a23a

Please sign in to comment.