Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1511479 > unrolled thread

[PATCH] usb: gadget: mv_u3d: add check for dma mapping error

Started byAlexey Khoroshilov <khoroshilov@ispras.ru>
First post2016-10-29 01:20 +0200
Last post2016-11-03 19:10 +0100
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: gadget: mv_u3d: add check for dma mapping error Alexey Khoroshilov <khoroshilov@ispras.ru> - 2016-10-29 01:20 +0200
    Re: [PATCH] usb: gadget: mv_u3d: add check for dma mapping error Felipe Balbi <balbi@kernel.org> - 2016-11-03 10:00 +0100
      [PATCH v2 2/2] usb: gadget: mv_u3d: mv_u3d_start_queue() refactoring Alexey Khoroshilov <khoroshilov@ispras.ru> - 2016-11-03 14:20 +0100
      [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error Alexey Khoroshilov <khoroshilov@ispras.ru> - 2016-11-03 14:20 +0100
        Re: [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error Felipe Balbi <balbi@kernel.org> - 2016-11-03 14:40 +0100
          Re: [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping  error Alexey Khoroshilov <khoroshilov@ispras.ru> - 2016-11-03 19:10 +0100

#1511479 — [PATCH] usb: gadget: mv_u3d: add check for dma mapping error

FromAlexey Khoroshilov <khoroshilov@ispras.ru>
Date2016-10-29 01:20 +0200
Subject[PATCH] usb: gadget: mv_u3d: add check for dma mapping error
Message-ID<sxoBk-6pf-11@gated-at.bofh.it>
mv_u3d_req_to_trb() does not check for dma mapping errors.

By the way, the patch improves readability of mv_u3d_start_queue()
by rearranging its code with two semantic modifications:
- assignment zero to ep->processing if usb_gadget_map_request() fails;
- propagation of error code from mv_u3d_req_to_trb() instead of 
  hardcoded -ENOMEM.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/usb/gadget/udc/mv_u3d_core.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index b9e19a591322..8d726bd767fd 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
 					req->trb_head->trb_hw,
 					trb_num * sizeof(*trb_hw),
 					DMA_BIDIRECTIONAL);
+		if (dma_mapping_error(u3d->gadget.dev.parent,
+					req->trb_head->trb_dma)) {
+			kfree(req->trb_head->trb_hw);
+			kfree(req->trb_head);
+			return -EFAULT;
+		}
 
 		req->chain = 1;
 	}
@@ -487,30 +493,32 @@ mv_u3d_start_queue(struct mv_u3d_ep *ep)
 	ret = usb_gadget_map_request(&u3d->gadget, &req->req,
 					mv_u3d_ep_dir(ep));
 	if (ret)
-		return ret;
+		goto break_processing;
 
 	req->req.status = -EINPROGRESS;
 	req->req.actual = 0;
 	req->trb_count = 0;
 
-	/* build trbs and push them to device queue */
-	if (!mv_u3d_req_to_trb(req)) {
-		ret = mv_u3d_queue_trb(ep, req);
-		if (ret) {
-			ep->processing = 0;
-			return ret;
-		}
-	} else {
-		ep->processing = 0;
+	/* build trbs */
+	ret = mv_u3d_req_to_trb(req);
+	if (ret) {
 		dev_err(u3d->dev, "%s, mv_u3d_req_to_trb fail\n", __func__);
-		return -ENOMEM;
+		goto break_processing;
 	}
 
+	/* and push them to device queue */
+	ret = mv_u3d_queue_trb(ep, req);
+	if (ret)
+		goto break_processing;
+
 	/* irq handler advances the queue */
-	if (req)
-		list_add_tail(&req->queue, &ep->queue);
+	list_add_tail(&req->queue, &ep->queue);
 
 	return 0;
+
+break_processing:
+	ep->processing = 0;
+	return ret;
 }
 
 static int mv_u3d_ep_enable(struct usb_ep *_ep,
-- 
2.7.4

[toc] | [next] | [standalone]

#1514399

FromFelipe Balbi <balbi@kernel.org>
Date2016-11-03 10:00 +0100
Message-ID<szm2m-1A3-9@gated-at.bofh.it>
In reply to#1511479

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alexey Khoroshilov <khoroshilov@ispras.ru> writes:
> mv_u3d_req_to_trb() does not check for dma mapping errors.
>
> By the way, the patch improves readability of mv_u3d_start_queue()
> by rearranging its code with two semantic modifications:
> - assignment zero to ep->processing if usb_gadget_map_request() fails;
> - propagation of error code from mv_u3d_req_to_trb() instead of 
>   hardcoded -ENOMEM.

cleanups and fixes should be done separately.

> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/usb/gadget/udc/mv_u3d_core.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
> index b9e19a591322..8d726bd767fd 100644
> --- a/drivers/usb/gadget/udc/mv_u3d_core.c
> +++ b/drivers/usb/gadget/udc/mv_u3d_core.c
> @@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
>  					req->trb_head->trb_hw,
>  					trb_num * sizeof(*trb_hw),
>  					DMA_BIDIRECTIONAL);
> +		if (dma_mapping_error(u3d->gadget.dev.parent,
> +					req->trb_head->trb_dma)) {
> +			kfree(req->trb_head->trb_hw);
> +			kfree(req->trb_head);
> +			return -EFAULT;
> +		}
>  
>  		req->chain = 1;
>  	}

this is one patch: add dma_mapping_error() check

AKA $subject :-p

> @@ -487,30 +493,32 @@ mv_u3d_start_queue(struct mv_u3d_ep *ep)
>  	ret = usb_gadget_map_request(&u3d->gadget, &req->req,
>  					mv_u3d_ep_dir(ep));
>  	if (ret)
> -		return ret;
> +		goto break_processing;
>  
>  	req->req.status = -EINPROGRESS;
>  	req->req.actual = 0;
>  	req->trb_count = 0;
>  
> -	/* build trbs and push them to device queue */
> -	if (!mv_u3d_req_to_trb(req)) {
> -		ret = mv_u3d_queue_trb(ep, req);
> -		if (ret) {
> -			ep->processing = 0;
> -			return ret;
> -		}
> -	} else {
> -		ep->processing = 0;
> +	/* build trbs */
> +	ret = mv_u3d_req_to_trb(req);
> +	if (ret) {
>  		dev_err(u3d->dev, "%s, mv_u3d_req_to_trb fail\n", __func__);
> -		return -ENOMEM;
> +		goto break_processing;
>  	}
>  
> +	/* and push them to device queue */
> +	ret = mv_u3d_queue_trb(ep, req);
> +	if (ret)
> +		goto break_processing;
> +
>  	/* irq handler advances the queue */
> -	if (req)
> -		list_add_tail(&req->queue, &ep->queue);
> +	list_add_tail(&req->queue, &ep->queue);
>  
>  	return 0;
> +
> +break_processing:
> +	ep->processing = 0;
> +	return ret;
>  }
>  
>  static int mv_u3d_ep_enable(struct usb_ep *_ep,

this is another, unrelated patch. Please split

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1514521 — [PATCH v2 2/2] usb: gadget: mv_u3d: mv_u3d_start_queue() refactoring

FromAlexey Khoroshilov <khoroshilov@ispras.ru>
Date2016-11-03 14:20 +0100
Subject[PATCH v2 2/2] usb: gadget: mv_u3d: mv_u3d_start_queue() refactoring
Message-ID<szq5X-4mP-7@gated-at.bofh.it>
In reply to#1514399
The patch improves readability of mv_u3d_start_queue()
by rearranging its code with two semantic modifications:
- assignment zero to ep->processing if usb_gadget_map_request() fails;
- propagation of error code from mv_u3d_req_to_trb() instead of
  hardcoded -ENOMEM.

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/usb/gadget/udc/mv_u3d_core.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index 6f3be0ba9ac8..8d726bd767fd 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -493,30 +493,32 @@ mv_u3d_start_queue(struct mv_u3d_ep *ep)
 	ret = usb_gadget_map_request(&u3d->gadget, &req->req,
 					mv_u3d_ep_dir(ep));
 	if (ret)
-		return ret;
+		goto break_processing;
 
 	req->req.status = -EINPROGRESS;
 	req->req.actual = 0;
 	req->trb_count = 0;
 
-	/* build trbs and push them to device queue */
-	if (!mv_u3d_req_to_trb(req)) {
-		ret = mv_u3d_queue_trb(ep, req);
-		if (ret) {
-			ep->processing = 0;
-			return ret;
-		}
-	} else {
-		ep->processing = 0;
+	/* build trbs */
+	ret = mv_u3d_req_to_trb(req);
+	if (ret) {
 		dev_err(u3d->dev, "%s, mv_u3d_req_to_trb fail\n", __func__);
-		return -ENOMEM;
+		goto break_processing;
 	}
 
+	/* and push them to device queue */
+	ret = mv_u3d_queue_trb(ep, req);
+	if (ret)
+		goto break_processing;
+
 	/* irq handler advances the queue */
-	if (req)
-		list_add_tail(&req->queue, &ep->queue);
+	list_add_tail(&req->queue, &ep->queue);
 
 	return 0;
+
+break_processing:
+	ep->processing = 0;
+	return ret;
 }
 
 static int mv_u3d_ep_enable(struct usb_ep *_ep,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1514522 — [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error

FromAlexey Khoroshilov <khoroshilov@ispras.ru>
Date2016-11-03 14:20 +0100
Subject[PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error
Message-ID<szq5X-4mP-9@gated-at.bofh.it>
In reply to#1514399
mv_u3d_req_to_trb() does not check for dma mapping errors.

Found by Linux Driver Verification project (linuxtesting.org).

v2: split fix and clenup to separate patches.
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/usb/gadget/udc/mv_u3d_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index b9e19a591322..6f3be0ba9ac8 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
 					req->trb_head->trb_hw,
 					trb_num * sizeof(*trb_hw),
 					DMA_BIDIRECTIONAL);
+		if (dma_mapping_error(u3d->gadget.dev.parent,
+					req->trb_head->trb_dma)) {
+			kfree(req->trb_head->trb_hw);
+			kfree(req->trb_head);
+			return -EFAULT;
+		}
 
 		req->chain = 1;
 	}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1514528 — Re: [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error

FromFelipe Balbi <balbi@kernel.org>
Date2016-11-03 14:40 +0100
SubjectRe: [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error
Message-ID<szqpj-4t0-17@gated-at.bofh.it>
In reply to#1514522

[Multipart message — attachments visible in raw view] — view raw

Hi,

Alexey Khoroshilov <khoroshilov@ispras.ru> writes:
> mv_u3d_req_to_trb() does not check for dma mapping errors.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> v2: split fix and clenup to separate patches.

I'll fix this time when applying, but keep in mind we don't want these
notes in the commit log. They should come after the tearline (---)
below, together with the diffstat ;-)

> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/usb/gadget/udc/mv_u3d_core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
> index b9e19a591322..6f3be0ba9ac8 100644
> --- a/drivers/usb/gadget/udc/mv_u3d_core.c
> +++ b/drivers/usb/gadget/udc/mv_u3d_core.c
> @@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
>  					req->trb_head->trb_hw,
>  					trb_num * sizeof(*trb_hw),
>  					DMA_BIDIRECTIONAL);
> +		if (dma_mapping_error(u3d->gadget.dev.parent,
> +					req->trb_head->trb_dma)) {
> +			kfree(req->trb_head->trb_hw);
> +			kfree(req->trb_head);
> +			return -EFAULT;
> +		}
>  
>  		req->chain = 1;
>  	}
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1514760 — Re: [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error

FromAlexey Khoroshilov <khoroshilov@ispras.ru>
Date2016-11-03 19:10 +0100
SubjectRe: [PATCH v2 1/2] usb: gadget: mv_u3d: add check for dma mapping error
Message-ID<szuCB-7nG-11@gated-at.bofh.it>
In reply to#1514528
On 03.11.2016 16:34, Felipe Balbi wrote:
> 
> Hi,
> 
> Alexey Khoroshilov <khoroshilov@ispras.ru> writes:
>> mv_u3d_req_to_trb() does not check for dma mapping errors.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> v2: split fix and clenup to separate patches.
> 
> I'll fix this time when applying, but keep in mind we don't want these
> notes in the commit log. They should come after the tearline (---)
> below, together with the diffstat ;-)

ok, thank you

--
Alexey

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web