dmaegine: virt-dma : Fix multi-user with vchan

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

Posted by Jie Hai 1 year, 6 months ago

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.

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@huawei.com>
---
 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

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

Posted by Frank Li 1 year, 5 months ago

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
> 
> 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@huawei.com>
> ---
>  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
>

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

Posted by Jie Hai 1 year, 5 months ago

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@huawei.com>
>> ---
>>   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
>>
> .

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

Posted by Vinod Koul 1 year, 5 months ago

On 20-06-24, 10: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?

I think I have asked in past (dont recall if that was this or some other
one), hopefully will get to hear on this one..

PS: Good hygiene to trim replies

-- 
~Vinod