[PATCH] dmaengine: ioatdma: fix unprotected timer deletion

[PATCH] dmaengine: ioatdma: fix unprotected timer deletion

[Dave Jiang]

When ioat_free_chan_resources() gets called, ioat_stop() is called without
chan->cleanup_lock. ioat_stop modifies IOAT_RUN bit.  It needs to be
protected by cleanup_lock. Also, in the __cleanup() path, if IOAT_RUN is
cleared, we should not touch the timer again. We observed that the timer
routine was run after timer was deleted.

Fixes: 3372de5813e ("dmaengine: ioatdma: removal of dma_v3.c and relevant ioat3
references")

Reported-by: Fan Du <fan.du@intel.com>
Tested-by: Fan Du <fan.du@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/ioat/dma.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
index f373a139e0c3..78598ba5c73b 100644
--- a/drivers/dma/ioat/dma.c
+++ b/drivers/dma/ioat/dma.c
@@ -138,11 +138,14 @@ void ioat_stop(struct ioatdma_chan *ioat_chan)
 	struct pci_dev *pdev = ioat_dma->pdev;
 	int chan_id = chan_num(ioat_chan);
 	struct msix_entry *msix;
+	unsigned long flags;
 
-	/* 1/ stop irq from firing tasklets
-	 * 2/ stop the tasklet from re-arming irqs
-	 */
+	spin_lock_irqsave(&ioat_chan->cleanup_lock, flags);
 	clear_bit(IOAT_RUN, &ioat_chan->state);
+	spin_unlock_irqrestore(&ioat_chan->cleanup_lock, flags);
+
+	/* flush inflight timers */
+	del_timer_sync(&ioat_chan->timer);
 
 	/* flush inflight interrupts */
 	switch (ioat_dma->irq_mode) {
@@ -158,9 +161,6 @@ void ioat_stop(struct ioatdma_chan *ioat_chan)
 		break;
 	}
 
-	/* flush inflight timers */
-	del_timer_sync(&ioat_chan->timer);
-
 	/* flush inflight tasklet runs */
 	tasklet_kill(&ioat_chan->cleanup_task);
 
@@ -652,7 +652,9 @@ static void __cleanup(struct ioatdma_chan *ioat_chan, dma_addr_t phys_complete)
 	if (active - i == 0) {
 		dev_dbg(to_dev(ioat_chan), "%s: cancel completion timeout\n",
 			__func__);
-		mod_timer(&ioat_chan->timer, jiffies + IDLE_TIMEOUT);
+
+		if (test_bit(IOAT_RUN, &ioat_chan->state))
+			mod_timer(&ioat_chan->timer, jiffies + IDLE_TIMEOUT);
 	}
 
 	/* microsecond delay by sysfs variable  per pending descriptor */

Re: [PATCH] dmaengine: ioatdma: fix unprotected timer deletion

[Dave Jiang]

On 5/9/19 4:37 PM, Dave Jiang wrote:
> When ioat_free_chan_resources() gets called, ioat_stop() is called without
> chan->cleanup_lock. ioat_stop modifies IOAT_RUN bit.  It needs to be
> protected by cleanup_lock. Also, in the __cleanup() path, if IOAT_RUN is
> cleared, we should not touch the timer again. We observed that the timer
> routine was run after timer was deleted.
> 
> Fixes: 3372de5813e ("dmaengine: ioatdma: removal of dma_v3.c and relevant ioat3
> references")
> 
> Reported-by: Fan Du <fan.du@intel.com>
> Tested-by: Fan Du <fan.du@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Vinod, can you hold off on this please? There may be more changes. Thanks.

> ---
>  drivers/dma/ioat/dma.c |   16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
> index f373a139e0c3..78598ba5c73b 100644
> --- a/drivers/dma/ioat/dma.c
> +++ b/drivers/dma/ioat/dma.c
> @@ -138,11 +138,14 @@ void ioat_stop(struct ioatdma_chan *ioat_chan)
>  	struct pci_dev *pdev = ioat_dma->pdev;
>  	int chan_id = chan_num(ioat_chan);
>  	struct msix_entry *msix;
> +	unsigned long flags;
>  
> -	/* 1/ stop irq from firing tasklets
> -	 * 2/ stop the tasklet from re-arming irqs
> -	 */
> +	spin_lock_irqsave(&ioat_chan->cleanup_lock, flags);
>  	clear_bit(IOAT_RUN, &ioat_chan->state);
> +	spin_unlock_irqrestore(&ioat_chan->cleanup_lock, flags);
> +
> +	/* flush inflight timers */
> +	del_timer_sync(&ioat_chan->timer);
>  
>  	/* flush inflight interrupts */
>  	switch (ioat_dma->irq_mode) {
> @@ -158,9 +161,6 @@ void ioat_stop(struct ioatdma_chan *ioat_chan)
>  		break;
>  	}
>  
> -	/* flush inflight timers */
> -	del_timer_sync(&ioat_chan->timer);
> -
>  	/* flush inflight tasklet runs */
>  	tasklet_kill(&ioat_chan->cleanup_task);
>  
> @@ -652,7 +652,9 @@ static void __cleanup(struct ioatdma_chan *ioat_chan, dma_addr_t phys_complete)
>  	if (active - i == 0) {
>  		dev_dbg(to_dev(ioat_chan), "%s: cancel completion timeout\n",
>  			__func__);
> -		mod_timer(&ioat_chan->timer, jiffies + IDLE_TIMEOUT);
> +
> +		if (test_bit(IOAT_RUN, &ioat_chan->state))
> +			mod_timer(&ioat_chan->timer, jiffies + IDLE_TIMEOUT);
>  	}
>  
>  	/* microsecond delay by sysfs variable  per pending descriptor */
> 

Re: [PATCH] dmaengine: ioatdma: fix unprotected timer deletion

[Vinod Koul]

On 16-05-19, 09:11, Dave Jiang wrote:
> 
> 
> On 5/9/19 4:37 PM, Dave Jiang wrote:
> > When ioat_free_chan_resources() gets called, ioat_stop() is called without
> > chan->cleanup_lock. ioat_stop modifies IOAT_RUN bit.  It needs to be
> > protected by cleanup_lock. Also, in the __cleanup() path, if IOAT_RUN is
> > cleared, we should not touch the timer again. We observed that the timer
> > routine was run after timer was deleted.
> > 
> > Fixes: 3372de5813e ("dmaengine: ioatdma: removal of dma_v3.c and relevant ioat3
> > references")
> > 
> > Reported-by: Fan Du <fan.du@intel.com>
> > Tested-by: Fan Du <fan.du@intel.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Vinod, can you hold off on this please? There may be more changes. Thanks.

Okay dropped

-- 
~Vinod