[PATCH] dma: sh: usb-dmac: Fix residue after the commit 24461d9792c2

[PATCH] dma: sh: usb-dmac: Fix residue after the commit 24461d9792c2

[Yoshihiro Shimoda]

This driver assumed that freed descriptors have "done_cookie".
But, after the commit 24461d9792c2 ("dmaengine: virt-dma: Fix
access after free in vchan_complete()"), since the desc is freed
after callback function was called, this driver could not
match any done_cookie when a client driver (renesas_usbhs driver)
calls dmaengine_tx_status() in the callback function.
So, add to check both descriptor types (freed and got) to fix
the issue.

Reported-by: Hien Dang <hien.dang.eb@renesas.com>
Fixes: 24461d9792c2 ("dmaengine: virt-dma: Fix access after free in vchan_complete()")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/dma/sh/usb-dmac.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/sh/usb-dmac.c b/drivers/dma/sh/usb-dmac.c
index b218a01..c0adc1c8 100644
--- a/drivers/dma/sh/usb-dmac.c
+++ b/drivers/dma/sh/usb-dmac.c
@@ -488,16 +488,17 @@ static u32 usb_dmac_chan_get_residue_if_complete(struct usb_dmac_chan *chan,
 						 dma_cookie_t cookie)
 {
 	struct usb_dmac_desc *desc;
-	u32 residue = 0;
 
+	list_for_each_entry_reverse(desc, &chan->desc_got, node) {
+		if (desc->done_cookie == cookie)
+			return desc->residue;
+	}
 	list_for_each_entry_reverse(desc, &chan->desc_freed, node) {
-		if (desc->done_cookie == cookie) {
-			residue = desc->residue;
-			break;
-		}
+		if (desc->done_cookie == cookie)
+			return desc->residue;
 	}
 
-	return residue;
+	return 0;
 }
 
 static u32 usb_dmac_chan_get_residue(struct usb_dmac_chan *chan,
-- 
2.7.4

RE: [PATCH] dma: sh: usb-dmac: Fix residue after the commit 24461d9792c2

[Yoshihiro Shimoda]

Hi Vinod,

> From: Yoshihiro Shimoda, Sent: Thursday, May 21, 2020 8:46 PM
> 
> This driver assumed that freed descriptors have "done_cookie".
> But, after the commit 24461d9792c2 ("dmaengine: virt-dma: Fix
> access after free in vchan_complete()"), since the desc is freed
> after callback function was called, this driver could not
> match any done_cookie when a client driver (renesas_usbhs driver)
> calls dmaengine_tx_status() in the callback function.
> So, add to check both descriptor types (freed and got) to fix
> the issue.
> 
> Reported-by: Hien Dang <hien.dang.eb@renesas.com>
> Fixes: 24461d9792c2 ("dmaengine: virt-dma: Fix access after free in vchan_complete()")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

I'm afraid, but would you review this patch?
I confirmed this patch still can be applied to v5.8-rc1.

Best regards,
Yoshihiro Shimoda

> ---
>  drivers/dma/sh/usb-dmac.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/sh/usb-dmac.c b/drivers/dma/sh/usb-dmac.c
> index b218a01..c0adc1c8 100644
> --- a/drivers/dma/sh/usb-dmac.c
> +++ b/drivers/dma/sh/usb-dmac.c
> @@ -488,16 +488,17 @@ static u32 usb_dmac_chan_get_residue_if_complete(struct usb_dmac_chan *chan,
>  						 dma_cookie_t cookie)
>  {
>  	struct usb_dmac_desc *desc;
> -	u32 residue = 0;
> 
> +	list_for_each_entry_reverse(desc, &chan->desc_got, node) {
> +		if (desc->done_cookie == cookie)
> +			return desc->residue;
> +	}
>  	list_for_each_entry_reverse(desc, &chan->desc_freed, node) {
> -		if (desc->done_cookie == cookie) {
> -			residue = desc->residue;
> -			break;
> -		}
> +		if (desc->done_cookie == cookie)
> +			return desc->residue;
>  	}
> 
> -	return residue;
> +	return 0;
>  }
> 
>  static u32 usb_dmac_chan_get_residue(struct usb_dmac_chan *chan,
> --
> 2.7.4

Re: [PATCH] dma: sh: usb-dmac: Fix residue after the commit 24461d9792c2

[Vinod Koul]

On 21-05-20, 20:46, Yoshihiro Shimoda wrote:
> This driver assumed that freed descriptors have "done_cookie".
> But, after the commit 24461d9792c2 ("dmaengine: virt-dma: Fix
> access after free in vchan_complete()"), since the desc is freed
> after callback function was called, this driver could not
> match any done_cookie when a client driver (renesas_usbhs driver)
> calls dmaengine_tx_status() in the callback function.

Hmmm, I am not sure about this, why should we try to match! cookie is
monotonically increasing number so if you see that current cookie
completed is > requested you should return DMA_COMPLETE

The below case of checking residue should not even get executed

> So, add to check both descriptor types (freed and got) to fix
> the issue.
> 
> Reported-by: Hien Dang <hien.dang.eb@renesas.com>
> Fixes: 24461d9792c2 ("dmaengine: virt-dma: Fix access after free in vchan_complete()")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/dma/sh/usb-dmac.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/sh/usb-dmac.c b/drivers/dma/sh/usb-dmac.c
> index b218a01..c0adc1c8 100644
> --- a/drivers/dma/sh/usb-dmac.c
> +++ b/drivers/dma/sh/usb-dmac.c
> @@ -488,16 +488,17 @@ static u32 usb_dmac_chan_get_residue_if_complete(struct usb_dmac_chan *chan,
>  						 dma_cookie_t cookie)
>  {
>  	struct usb_dmac_desc *desc;
> -	u32 residue = 0;
>  
> +	list_for_each_entry_reverse(desc, &chan->desc_got, node) {
> +		if (desc->done_cookie == cookie)
> +			return desc->residue;
> +	}
>  	list_for_each_entry_reverse(desc, &chan->desc_freed, node) {
> -		if (desc->done_cookie == cookie) {
> -			residue = desc->residue;
> -			break;
> -		}
> +		if (desc->done_cookie == cookie)
> +			return desc->residue;
>  	}
>  
> -	return residue;
> +	return 0;
>  }
>  
>  static u32 usb_dmac_chan_get_residue(struct usb_dmac_chan *chan,
> -- 
> 2.7.4

-- 
~Vinod

RE: [PATCH] dma: sh: usb-dmac: Fix residue after the commit 24461d9792c2

[Yoshihiro Shimoda]

Hi Vinod,

> From: Vinod Koul, Sent: Wednesday, June 17, 2020 1:56 AM
> 
> On 21-05-20, 20:46, Yoshihiro Shimoda wrote:
> > This driver assumed that freed descriptors have "done_cookie".
> > But, after the commit 24461d9792c2 ("dmaengine: virt-dma: Fix
> > access after free in vchan_complete()"), since the desc is freed
> > after callback function was called, this driver could not
> > match any done_cookie when a client driver (renesas_usbhs driver)
> > calls dmaengine_tx_status() in the callback function.
> 
> Hmmm, I am not sure about this, why should we try to match! cookie is
> monotonically increasing number so if you see that current cookie
> completed is > requested you should return DMA_COMPLETE

The reason is this hardware is possible to stop the transfer even if
all transfer length is not received. This is related to one of USB
specification which allows to stop when getting a short packet.
So, a client driver has to get residue even if DMA_COMPLETE.

> The below case of checking residue should not even get executed

I see...
So, I'm thinking the current implementation was a tricky because we didn't
have dma_async_tx_callback_result when I wrote this usb-dmac driver.
I'll try this to fix the issue.

Best regards,
Yoshihiro Shimoda

> > So, add to check both descriptor types (freed and got) to fix
> > the issue.
> >
> > Reported-by: Hien Dang <hien.dang.eb@renesas.com>
> > Fixes: 24461d9792c2 ("dmaengine: virt-dma: Fix access after free in vchan_complete()")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/dma/sh/usb-dmac.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/dma/sh/usb-dmac.c b/drivers/dma/sh/usb-dmac.c
> > index b218a01..c0adc1c8 100644
> > --- a/drivers/dma/sh/usb-dmac.c
> > +++ b/drivers/dma/sh/usb-dmac.c
> > @@ -488,16 +488,17 @@ static u32 usb_dmac_chan_get_residue_if_complete(struct usb_dmac_chan *chan,
> >  						 dma_cookie_t cookie)
> >  {
> >  	struct usb_dmac_desc *desc;
> > -	u32 residue = 0;
> >
> > +	list_for_each_entry_reverse(desc, &chan->desc_got, node) {
> > +		if (desc->done_cookie == cookie)
> > +			return desc->residue;
> > +	}
> >  	list_for_each_entry_reverse(desc, &chan->desc_freed, node) {
> > -		if (desc->done_cookie == cookie) {
> > -			residue = desc->residue;
> > -			break;
> > -		}
> > +		if (desc->done_cookie == cookie)
> > +			return desc->residue;
> >  	}
> >
> > -	return residue;
> > +	return 0;
> >  }
> >
> >  static u32 usb_dmac_chan_get_residue(struct usb_dmac_chan *chan,
> > --
> > 2.7.4
> 
> --
> ~Vinod

Re: [PATCH] dma: sh: usb-dmac: Fix residue after the commit 24461d9792c2

[Vinod Koul]

On 18-06-20, 00:56, Yoshihiro Shimoda wrote:
> Hi Vinod,
> 
> > From: Vinod Koul, Sent: Wednesday, June 17, 2020 1:56 AM
> > 
> > On 21-05-20, 20:46, Yoshihiro Shimoda wrote:
> > > This driver assumed that freed descriptors have "done_cookie".
> > > But, after the commit 24461d9792c2 ("dmaengine: virt-dma: Fix
> > > access after free in vchan_complete()"), since the desc is freed
> > > after callback function was called, this driver could not
> > > match any done_cookie when a client driver (renesas_usbhs driver)
> > > calls dmaengine_tx_status() in the callback function.
> > 
> > Hmmm, I am not sure about this, why should we try to match! cookie is
> > monotonically increasing number so if you see that current cookie
> > completed is > requested you should return DMA_COMPLETE
> 
> The reason is this hardware is possible to stop the transfer even if
> all transfer length is not received. This is related to one of USB
> specification which allows to stop when getting a short packet.
> So, a client driver has to get residue even if DMA_COMPLETE.

We have additional dma_async_tx_callback_result callback to indicate the
residue in these cases, please use that

> > The below case of checking residue should not even get executed
> 
> I see...
> So, I'm thinking the current implementation was a tricky because we didn't
> have dma_async_tx_callback_result when I wrote this usb-dmac driver.
> I'll try this to fix the issue.

Right :)

Also please use tag dmaengine: .. for these patches

-- 
~Vinod