dmaengine Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] dmaengine: xilinx_dma: Add missing check for empty list
@ 2020-03-03 13:05 Sebastian von Ohr
  2020-03-06 13:34 ` Vinod Koul
  0 siblings, 1 reply; 4+ messages in thread
From: Sebastian von Ohr @ 2020-03-03 13:05 UTC (permalink / raw)
  To: vkoul, dmaengine; +Cc: Sebastian von Ohr

The DMA transfer might finish just after checking the state with
dma_cookie_status, but before the lock is acquired. Not checking
for an empty list in xilinx_dma_tx_status may result in reading
random data or data corruption when desc is written to. This can
be reliably triggered by using dma_sync_wait to wait for DMA
completion.

Signed-off-by: Sebastian von Ohr <vonohr@smaract.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index a9c5d5cc9f2b..5d5f1d0ce16c 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1229,16 +1229,16 @@ static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
 		return ret;
 
 	spin_lock_irqsave(&chan->lock, flags);
-
-	desc = list_last_entry(&chan->active_list,
-			       struct xilinx_dma_tx_descriptor, node);
-	/*
-	 * VDMA and simple mode do not support residue reporting, so the
-	 * residue field will always be 0.
-	 */
-	if (chan->has_sg && chan->xdev->dma_config->dmatype != XDMA_TYPE_VDMA)
-		residue = xilinx_dma_get_residue(chan, desc);
-
+	if (!list_empty(&chan->active_list)) {
+		desc = list_last_entry(&chan->active_list,
+				       struct xilinx_dma_tx_descriptor, node);
+		/*
+		 * VDMA and simple mode do not support residue reporting, so the
+		 * residue field will always be 0.
+		 */
+		if (chan->has_sg && chan->xdev->dma_config->dmatype != XDMA_TYPE_VDMA)
+			residue = xilinx_dma_get_residue(chan, desc);
+	}
 	spin_unlock_irqrestore(&chan->lock, flags);
 
 	dma_set_residue(txstate, residue);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] dmaengine: xilinx_dma: Add missing check for empty list
  2020-03-03 13:05 [PATCH] dmaengine: xilinx_dma: Add missing check for empty list Sebastian von Ohr
@ 2020-03-06 13:34 ` Vinod Koul
  2020-03-06 13:57   ` Radhey Shyam Pandey
  0 siblings, 1 reply; 4+ messages in thread
From: Vinod Koul @ 2020-03-06 13:34 UTC (permalink / raw)
  To: Sebastian von Ohr, Appana Durga Kedareswara rao,
	Radhey Shyam Pandey, Michal Simek
  Cc: dmaengine

On 03-03-20, 14:05, Sebastian von Ohr wrote:
> The DMA transfer might finish just after checking the state with
> dma_cookie_status, but before the lock is acquired. Not checking
> for an empty list in xilinx_dma_tx_status may result in reading
> random data or data corruption when desc is written to. This can
> be reliably triggered by using dma_sync_wait to wait for DMA
> completion.

Appana, Radhey can you please test this..?

> 
> Signed-off-by: Sebastian von Ohr <vonohr@smaract.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index a9c5d5cc9f2b..5d5f1d0ce16c 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1229,16 +1229,16 @@ static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
>  		return ret;
>  
>  	spin_lock_irqsave(&chan->lock, flags);
> -
> -	desc = list_last_entry(&chan->active_list,
> -			       struct xilinx_dma_tx_descriptor, node);
> -	/*
> -	 * VDMA and simple mode do not support residue reporting, so the
> -	 * residue field will always be 0.
> -	 */
> -	if (chan->has_sg && chan->xdev->dma_config->dmatype != XDMA_TYPE_VDMA)
> -		residue = xilinx_dma_get_residue(chan, desc);
> -
> +	if (!list_empty(&chan->active_list)) {
> +		desc = list_last_entry(&chan->active_list,
> +				       struct xilinx_dma_tx_descriptor, node);
> +		/*
> +		 * VDMA and simple mode do not support residue reporting, so the
> +		 * residue field will always be 0.
> +		 */
> +		if (chan->has_sg && chan->xdev->dma_config->dmatype != XDMA_TYPE_VDMA)
> +			residue = xilinx_dma_get_residue(chan, desc);
> +	}
>  	spin_unlock_irqrestore(&chan->lock, flags);
>  
>  	dma_set_residue(txstate, residue);
> -- 
> 2.17.1

-- 
~Vinod

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] dmaengine: xilinx_dma: Add missing check for empty list
  2020-03-06 13:34 ` Vinod Koul
@ 2020-03-06 13:57   ` Radhey Shyam Pandey
  2020-03-11  9:16     ` Vinod Koul
  0 siblings, 1 reply; 4+ messages in thread
From: Radhey Shyam Pandey @ 2020-03-06 13:57 UTC (permalink / raw)
  To: Vinod Koul, Sebastian von Ohr, Appana Durga Kedareswara Rao,
	Michal Simek
  Cc: dmaengine

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Friday, March 6, 2020 7:04 PM
> To: Sebastian von Ohr <vonohr@smaract.com>; Appana Durga Kedareswara
> Rao <appanad@xilinx.com>; Radhey Shyam Pandey <radheys@xilinx.com>;
> Michal Simek <michals@xilinx.com>
> Cc: dmaengine@vger.kernel.org
> Subject: Re: [PATCH] dmaengine: xilinx_dma: Add missing check for empty list

Minor nit -  Better to also add <...> "in device_tx_status callback "
> 
> On 03-03-20, 14:05, Sebastian von Ohr wrote:
> > The DMA transfer might finish just after checking the state with
> > dma_cookie_status, but before the lock is acquired. Not checking for
> > an empty list in xilinx_dma_tx_status may result in reading random
> > data or data corruption when desc is written to. This can be reliably
> > triggered by using dma_sync_wait to wait for DMA completion.
> 
> Appana, Radhey can you please test this..?

Sure, we will test it. Changes look fine.  Though had a question in mind, 
for a generic fix to this problem, should we make locking mandatory for 
all cookie helper functions? Or is there any limitation?

The framework say for dma_cookie_status says locking is not required. This
scenario is a race condition when the driver calls dma_cookie_status and
it sees it's not completed, but then since there is no locking and dma 
completion comes and it changes cookie state and removes the element 
from active list to done list.  When driver access it in tx_status it  results
in data corruption/crash.
> 
> >
> > Signed-off-by: Sebastian von Ohr <vonohr@smaract.com>
> > ---
> >  drivers/dma/xilinx/xilinx_dma.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > b/drivers/dma/xilinx/xilinx_dma.c index a9c5d5cc9f2b..5d5f1d0ce16c
> > 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -1229,16 +1229,16 @@ static enum dma_status
> xilinx_dma_tx_status(struct dma_chan *dchan,
> >  		return ret;
> >
> >  	spin_lock_irqsave(&chan->lock, flags);
> > -
> > -	desc = list_last_entry(&chan->active_list,
> > -			       struct xilinx_dma_tx_descriptor, node);
> > -	/*
> > -	 * VDMA and simple mode do not support residue reporting, so the
> > -	 * residue field will always be 0.
> > -	 */
> > -	if (chan->has_sg && chan->xdev->dma_config->dmatype !=
> XDMA_TYPE_VDMA)
> > -		residue = xilinx_dma_get_residue(chan, desc);
> > -
> > +	if (!list_empty(&chan->active_list)) {
> > +		desc = list_last_entry(&chan->active_list,
> > +				       struct xilinx_dma_tx_descriptor, node);
> > +		/*
> > +		 * VDMA and simple mode do not support residue reporting,
> so the
> > +		 * residue field will always be 0.
> > +		 */
> > +		if (chan->has_sg && chan->xdev->dma_config->dmatype !=
> XDMA_TYPE_VDMA)
> > +			residue = xilinx_dma_get_residue(chan, desc);
> > +	}
> >  	spin_unlock_irqrestore(&chan->lock, flags);
> >
> >  	dma_set_residue(txstate, residue);
> > --
> > 2.17.1
> 
> --
> ~Vinod

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] dmaengine: xilinx_dma: Add missing check for empty list
  2020-03-06 13:57   ` Radhey Shyam Pandey
@ 2020-03-11  9:16     ` Vinod Koul
  0 siblings, 0 replies; 4+ messages in thread
From: Vinod Koul @ 2020-03-11  9:16 UTC (permalink / raw)
  To: Radhey Shyam Pandey
  Cc: Sebastian von Ohr, Appana Durga Kedareswara Rao, Michal Simek, dmaengine

On 06-03-20, 13:57, Radhey Shyam Pandey wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Friday, March 6, 2020 7:04 PM
> > To: Sebastian von Ohr <vonohr@smaract.com>; Appana Durga Kedareswara
> > Rao <appanad@xilinx.com>; Radhey Shyam Pandey <radheys@xilinx.com>;
> > Michal Simek <michals@xilinx.com>
> > Cc: dmaengine@vger.kernel.org
> > Subject: Re: [PATCH] dmaengine: xilinx_dma: Add missing check for empty list
> 
> Minor nit -  Better to also add <...> "in device_tx_status callback "
> > 
> > On 03-03-20, 14:05, Sebastian von Ohr wrote:
> > > The DMA transfer might finish just after checking the state with
> > > dma_cookie_status, but before the lock is acquired. Not checking for
> > > an empty list in xilinx_dma_tx_status may result in reading random
> > > data or data corruption when desc is written to. This can be reliably
> > > triggered by using dma_sync_wait to wait for DMA completion.
> > 
> > Appana, Radhey can you please test this..?
> 
> Sure, we will test it. Changes look fine.  Though had a question in mind, 
> for a generic fix to this problem, should we make locking mandatory for 
> all cookie helper functions? Or is there any limitation?
> 
> The framework say for dma_cookie_status says locking is not required. This
> scenario is a race condition when the driver calls dma_cookie_status and
> it sees it's not completed, but then since there is no locking and dma 
> completion comes and it changes cookie state and removes the element 
> from active list to done list.  When driver access it in tx_status it  results
> in data corruption/crash.

The expectation is that you would lock while looking at list and then
return.. So you should not have issues..

-- 
~Vinod

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 13:05 [PATCH] dmaengine: xilinx_dma: Add missing check for empty list Sebastian von Ohr
2020-03-06 13:34 ` Vinod Koul
2020-03-06 13:57   ` Radhey Shyam Pandey
2020-03-11  9:16     ` Vinod Koul

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git