Re: [PATCH v2] dmaegine: virt-dma : Fix multi-user with vchan

Jie Hai <haijie1@xxxxxxxxxx> · Sat, 29 Jun 2024 10:00:34 +0800

On 2024/6/20 22:38, Frank Li wrote:
On Thu, Jun 20, 2024 at 10:53:53AM +0800, Jie Hai wrote:
List desc_allocated was introduced for the case of a transfer
submitted multiple times. But elegating descriptors on the list
causes other problems.

For example, in the multi-thread scenario, which tasks are
continuously created and submitted by each thread. If one of
the threads calls dmaengine_terminate_all, for dirvers using
vchan_get_all_descriptors, all descriptors will be freed. If
there's another thread submitting a transfer A by
vchan_tx_submit, the following results may be generated:
1. desc A is freeing -> visit wrong address of node prep/next.
2. desc A is freed -> visit invalid address of A.

In the above case, calltrace is generated and the system is
suspended. This can be tested by dmatest.

What's test steps to reproduce this problem?

Frank
Thanks for your review.
The operations are as follows:
  modprobe hisi_dma
  modprobe dmatest
  echo 0 > /sys/module/dmatest/parameters/iterations
  echo "dma0chan0" > /sys/module/dmatest/parameters/channel
  echo 20 > /sys/module/dmatest/parameters/threads_per_chan
  echo 1 > /sys/module/dmatest/parameters/run
wait for a while and stop the test by:
  echo 0 > /sys/module/dmatest/parameters/run

This patch removes desc_allocated from vchan_get_all_descriptors,
and add new function 'vchan_get_all_allocated_descs' to get all
descriptors ever allocated.

And apply vchan_get_all_allocated_descs to free chan resource and
vchan_get_all_descriptors to terminate all transfers, respectively.
This avoids freeing up descriptors in use by other threads.

Signed-off-by: Jie Hai <haijie1@xxxxxxxxxx>
---
  drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c |  2 +-
  drivers/dma/fsl-edma-common.c           |  2 +-
  drivers/dma/fsl-qdma.c                  |  2 +-
  drivers/dma/sf-pdma/sf-pdma.c           |  2 +-
  drivers/dma/virt-dma.h                  | 20 ++++++++++++++++++--
  5 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
index 36384d019263..efdecf15e1b3 100644
--- a/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
+++ b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
@@ -71,7 +71,7 @@ static void dpaa2_qdma_free_chan_resources(struct dma_chan *chan)
  	LIST_HEAD(head);
  
  	spin_lock_irqsave(&dpaa2_chan->vchan.lock, flags);
-	vchan_get_all_descriptors(&dpaa2_chan->vchan, &head);
+	vchan_get_all_allocated_descs(&dpaa2_chan->vchan, &head);
  	spin_unlock_irqrestore(&dpaa2_chan->vchan.lock, flags);
  
  	vchan_dma_desc_free_list(&dpaa2_chan->vchan, &head);
diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
index 3af430787315..1e0ad87eb7fa 100644
--- a/drivers/dma/fsl-edma-common.c
+++ b/drivers/dma/fsl-edma-common.c
@@ -828,7 +828,7 @@ void fsl_edma_free_chan_resources(struct dma_chan *chan)
  	if (edma->drvdata->dmamuxs)
  		fsl_edma_chan_mux(fsl_chan, 0, false);
  	fsl_chan->edesc = NULL;
-	vchan_get_all_descriptors(&fsl_chan->vchan, &head);
+	vchan_get_all_allocated_descs(&fsl_chan->vchan, &head);
  	fsl_edma_unprep_slave_dma(fsl_chan);
  	spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags);
  
diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
index 5005e138fc23..7af428db404e 100644
--- a/drivers/dma/fsl-qdma.c
+++ b/drivers/dma/fsl-qdma.c
@@ -316,7 +316,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan *chan)
  	LIST_HEAD(head);
  
  	spin_lock_irqsave(&fsl_chan->vchan.lock, flags);
-	vchan_get_all_descriptors(&fsl_chan->vchan, &head);
+	vchan_get_all_allocated_descs(&fsl_chan->vchan, &head);
  	spin_unlock_irqrestore(&fsl_chan->vchan.lock, flags);
  
  	vchan_dma_desc_free_list(&fsl_chan->vchan, &head);
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
index 428473611115..4dc8a8c8ad80 100644
--- a/drivers/dma/sf-pdma/sf-pdma.c
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -147,7 +147,7 @@ static void sf_pdma_free_chan_resources(struct dma_chan *dchan)
  	sf_pdma_disable_request(chan);
  	kfree(chan->desc);
  	chan->desc = NULL;
-	vchan_get_all_descriptors(&chan->vchan, &head);
+	vchan_get_all_allocated_descs(&chan->vchan, &head);
  	sf_pdma_disclaim_chan(chan);
  	spin_unlock_irqrestore(&chan->vchan.lock, flags);
  	vchan_dma_desc_free_list(&chan->vchan, &head);
diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
index 59d9eabc8b67..4492641b79f6 100644
--- a/drivers/dma/virt-dma.h
+++ b/drivers/dma/virt-dma.h
@@ -187,13 +187,29 @@ static inline void vchan_get_all_descriptors(struct virt_dma_chan *vc,
  {
  	lockdep_assert_held(&vc->lock);
  
-	list_splice_tail_init(&vc->desc_allocated, head);
  	list_splice_tail_init(&vc->desc_submitted, head);
  	list_splice_tail_init(&vc->desc_issued, head);
  	list_splice_tail_init(&vc->desc_completed, head);
  	list_splice_tail_init(&vc->desc_terminated, head);
  }
  
+/**
+ * vchan_get_all_allocated_descs - obtain all descriptors
+ * @vc: virtual channel to get descriptors from
+ * @head: list of descriptors found
+ *
+ * vc.lock must be held by caller
+ *
+ * Removes all descriptors from internal lists, and provides a list of all
+ * descriptors found
+ */
+static inline void vchan_get_all_allocated_descs(struct virt_dma_chan *vc,
+	struct list_head *head)
+{
+	list_splice_tail_init(&vc->desc_allocated, head);
+	vchan_get_all_descriptors(vc, head);
+}
+
  static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
  {
  	struct virt_dma_desc *vd;
@@ -201,7 +217,7 @@ static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
  	LIST_HEAD(head);
  
  	spin_lock_irqsave(&vc->lock, flags);
-	vchan_get_all_descriptors(vc, &head);
+	vchan_get_all_allocated_descs(vc, &head);
  	list_for_each_entry(vd, &head, node)
  		dmaengine_desc_clear_reuse(&vd->tx);
  	spin_unlock_irqrestore(&vc->lock, flags);
--
2.33.0

.







