| Message ID | 20240620025400.3300641-1-haijie1@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2] dmaegine: virt-dma : Fix multi-user with vchan | expand |
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 >
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
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 >> > .
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);
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(-)