Re: [PATCH v2 1/4] dma-mapping: benchmark: fix up kthread-related error handling

From: Robin Murphy
Date: Fri May 10 2024 - 13:35:28 EST


On 2024-05-04 12:47 pm, Fedor Pchelkin wrote:
kthread creation failure is invalidly handled inside do_map_benchmark().
The put_task_struct() calls on the error path are supposed to balance the
get_task_struct() calls which only happen after all the kthreads are
successfully created. Rollback using kthread_stop() for already created
kthreads in case of such failure.

In normal situation call kthread_stop_put() to gracefully stop kthreads
and put their task refcounts. This should be done for all started
kthreads.

Although strictly there were two overlapping bugs here, I'd agree that it really doesn't seem worth the bother of trying to distinguish them. I'm far from a kthread expert, but as best I can tell this looks like it achieves a sensible final state. Modulo one nit below,

Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>

Found by Linux Verification Center (linuxtesting.org).

Fixes: 65789daa8087 ("dma-mapping: add benchmark support for streaming DMA APIs")
Suggested-by: Robin Murphy <robin.murphy@xxxxxxx>
Signed-off-by: Fedor Pchelkin <pchelkin@xxxxxxxxx>
---
kernel/dma/map_benchmark.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
index 02205ab53b7e..2478957cf9f8 100644
--- a/kernel/dma/map_benchmark.c
+++ b/kernel/dma/map_benchmark.c
@@ -118,6 +118,8 @@ static int do_map_benchmark(struct map_benchmark_data *map)
if (IS_ERR(tsk[i])) {
pr_err("create dma_map thread failed\n");
ret = PTR_ERR(tsk[i]);
+ while (--i >= 0)
+ kthread_stop(tsk[i]);

I think this means we'd end up actually starting the threads purely to get them to see the KTHREAD_SHOULD_STOP flag and exit again? Not that I'm too fussed about optimising an unexpected error path, however I can't help but wonder if we might only need a put_task_struct() if we can safely assume that the threads have never been started at this point.

Thanks,
Robin.

goto out;
}
@@ -139,13 +141,17 @@ static int do_map_benchmark(struct map_benchmark_data *map)
msleep_interruptible(map->bparam.seconds * 1000);
- /* wait for the completion of benchmark threads */
+ /* wait for the completion of all started benchmark threads */
for (i = 0; i < threads; i++) {
- ret = kthread_stop(tsk[i]);
- if (ret)
- goto out;
+ int kthread_ret = kthread_stop_put(tsk[i]);
+
+ if (kthread_ret)
+ ret = kthread_ret;
}
+ if (ret)
+ goto out;
+
loops = atomic64_read(&map->loops);
if (likely(loops > 0)) {
u64 map_variance, unmap_variance;
@@ -170,8 +176,6 @@ static int do_map_benchmark(struct map_benchmark_data *map)
}
out:
- for (i = 0; i < threads; i++)
- put_task_struct(tsk[i]);
put_device(map->dev);
kfree(tsk);
return ret;