Skip to contentSkip to conversation

gadget: f_thor: Fix memory leaks of usb request and its buffer

PATCH Superseded Seung-Woo Kim 2 participants 5 messages May 23, 2018 – May 25, 2018 1 message not in the archive USB

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): CGME20180523072552epcas1p3b1bdbc7e0f49053fe3ce15117e61b429@epcas1p3.samsung.com

#1 lore raw

PATCH 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer

There are memory leaks of usb request and its buffer for ep0, in_ep, and out ep. Fix memory leaks of usb request and its buffer.

---
 drivers/usb/gadget/f_thor.c |   34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)
drivers/usb/gadget/f_thor.c +25 -9
index c8eda05..ec8bd50 100644
@@ -752,6 +752,13 @@ int thor_handle(void)
 	return 0;
 }
 
+static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
+{
+	if (req->buf)
+		free(req->buf);
+	usb_ep_free_request(ep, req);
+}
+
 static int thor_func_bind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct usb_gadget *gadget = c->cdev->gadget;
@@ -860,21 +867,18 @@ static int thor_func_bind(struct usb_configuration *c, struct usb_function *f)
 	return 0;
 
  fail:
+	if (dev->req)
+		free_ep_req(gadget->ep0, dev->req);
 	free(dev);
 	return status;
 }
 
-static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-{
-	free(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static void thor_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_thor *f_thor = func_to_thor(f);
 	struct thor_dev *dev = f_thor->dev;
 
+	free_ep_req(dev->gadget->ep0, dev->req);
 	free(dev);
 	memset(thor_func, 0, sizeof(*thor_func));
 	thor_func = NULL;
@@ -895,9 +899,9 @@ static void thor_func_disable(struct usb_function *f)
 	}
 
 	if (dev->out_ep->driver_data) {
-		free(dev->out_req->buf);
+		/* buf of out_req is set with thor_set_dma(), so just clear */
 		dev->out_req->buf = NULL;
-		usb_ep_free_request(dev->out_ep, dev->out_req);
+		free_ep_req(dev->out_ep, dev->out_req);
 		usb_ep_disable(dev->out_ep);
 		dev->out_ep->driver_data = NULL;
 	}
@@ -940,17 +944,29 @@ static int thor_eps_setup(struct usb_function *f)
 	debug("(d)bEndpointAddress: 0x%x\n", d->bEndpointAddress);
 
 	result = usb_ep_enable(ep, d);
-	if (result)
+	if (result) {
+		free_ep_req(dev->in_ep, dev->in_req);
+		dev->in_req = NULL;
+		usb_ep_disable(dev->in_ep);
 		goto exit;
+	}
 
 	ep->driver_data = cdev; /* claim */
 	req = thor_start_ep(ep);
 	if (!req) {
 		usb_ep_disable(ep);
+		free_ep_req(dev->in_ep, dev->in_req);
+		dev->in_req = NULL;
+		usb_ep_disable(dev->in_ep);
 		result = -EIO;
 		goto exit;
 	}
 
+	/* buf of out_req will be set with thor_set_dma(), so clear it */
+	free(req->buf);
+	req->buf = NULL;
+	req->length = 0;
+
 	dev->out_req = req;
 	/* ACM control EP */
 	ep = dev->int_ep;
Signature · 1 line
1.7.9.5
#2 lore raw

PATCH v2 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer

There are memory leaks of usb request and its buffer for ep0, in_ep, and out ep. Fix memory leaks of usb request and its buffer.

---
Change from v1
- remove allocation of out_ep request instead of allocating and freeing
- fix use error path instead of duplicated error handling code

---
 drivers/usb/gadget/f_thor.c |   45 ++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 16 deletions(-)
drivers/usb/gadget/f_thor.c +29 -16
index c8eda05..02d6844 100644
@@ -752,6 +752,13 @@ int thor_handle(void)
 	return 0;
 }
 
+static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
+{
+	if (req->buf)
+		free(req->buf);
+	usb_ep_free_request(ep, req);
+}
+
 static int thor_func_bind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct usb_gadget *gadget = c->cdev->gadget;
@@ -860,21 +867,18 @@ static int thor_func_bind(struct usb_configuration *c, struct usb_function *f)
 	return 0;
 
  fail:
+	if (dev->req)
+		free_ep_req(gadget->ep0, dev->req);
 	free(dev);
 	return status;
 }
 
-static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-{
-	free(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static void thor_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_thor *f_thor = func_to_thor(f);
 	struct thor_dev *dev = f_thor->dev;
 
+	free_ep_req(dev->gadget->ep0, dev->req);
 	free(dev);
 	memset(thor_func, 0, sizeof(*thor_func));
 	thor_func = NULL;
@@ -895,8 +899,6 @@ static void thor_func_disable(struct usb_function *f)
 	}
 
 	if (dev->out_ep->driver_data) {
-		free(dev->out_req->buf);
-		dev->out_req->buf = NULL;
 		usb_ep_free_request(dev->out_ep, dev->out_req);
 		usb_ep_disable(dev->out_ep);
 		dev->out_ep->driver_data = NULL;
@@ -924,14 +926,13 @@ static int thor_eps_setup(struct usb_function *f)
 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err;
 
 	ep->driver_data = cdev; /* claim */
 	req = thor_start_ep(ep);
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_in_ep;
 	}
 
 	dev->in_req = req;
@@ -941,22 +942,34 @@ static int thor_eps_setup(struct usb_function *f)
 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err_free_in_req;
 
 	ep->driver_data = cdev; /* claim */
-	req = thor_start_ep(ep);
+	req = usb_ep_alloc_request(ep, 0);
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_out_ep;
 	}
 
+	req->complete = thor_rx_tx_complete;
 	dev->out_req = req;
 	/* ACM control EP */
 	ep = dev->int_ep;
 	ep->driver_data = cdev;	/* claim */
 
- exit:
+	return 0;
+
+ err_disable_out_ep:
+	usb_ep_disable(dev->out_ep);
+
+ err_free_in_req:
+	free_ep_req(dev->in_ep, dev->in_req);
+	dev->in_req = NULL;
+
+ err_disable_in_ep:
+	usb_ep_disable(dev->in_ep);
+
+ err:
 	return result;
 }
 
Signature · 1 line
1.7.9.5
#3 lore raw

PATCH v2 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer

Hi Seung-Woo,

Quoted 44 lines

There are memory leaks of usb request and its buffer for ep0, in_ep, and out ep. Fix memory leaks of usb request and its buffer.

---
Change from v1
- remove allocation of out_ep request instead of allocating and freeing
- fix use error path instead of duplicated error handling code

---
 drivers/usb/gadget/f_thor.c |   45

++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 16 deletions(-)

drivers/usb/gadget/f_thor.c +9 -2
index c8eda05..02d6844 100644
@@ -752,6 +752,13 @@ int thor_handle(void)
 	return 0;
 }
 
+static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
+{
+	if (req->buf)
+		free(req->buf);
+	usb_ep_free_request(ep, req);
+}
+
 static int thor_func_bind(struct usb_configuration *c, struct
 usb_function *f) {
 	struct usb_gadget *gadget = c->cdev->gadget;
@@ -860,21 +867,18 @@ static int thor_func_bind(struct
 usb_configuration *c, struct usb_function *f) return 0;
 
  fail:
+	if (dev->req)
+		free_ep_req(gadget->ep0, dev->req);
 	free(dev);
 	return status;
 }
 
-static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-{

drivers/usb/gadget/f_thor.c static int thor_func_bind(struct · line 881

Quoted 10 lines
drivers/usb/gadget/f_thor.c +1 -4
-	free(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static void thor_unbind(struct usb_configuration *c, struct
 usb_function *f) {
 	struct f_thor *f_thor = func_to_thor(f);
 	struct thor_dev *dev = f_thor->dev;
 
+	free_ep_req(dev->gadget->ep0, dev->req);

Till this change - no issues.

drivers/usb/gadget/f_thor.c static void thor_func_disable(struct usb_function · line 899

Quoted 9 lines
drivers/usb/gadget/f_thor.c +0 -2
 	free(dev);
 	memset(thor_func, 0, sizeof(*thor_func));
 	thor_func = NULL;
@@ -895,8 +899,6 @@ static void thor_func_disable(struct usb_function
 *f) }
 
 	if (dev->out_ep->driver_data) {
-		free(dev->out_req->buf);
-		dev->out_req->buf = NULL;

I think that this setting (to NULL) was needed to be able to ctrl+C from thor command and then run it again (as some code checks if buf is NULL).

Quoted 21 lines
drivers/usb/gadget/f_thor.c +2 -3
 		usb_ep_free_request(dev->out_ep, dev->out_req);
 		usb_ep_disable(dev->out_ep);
 		dev->out_ep->driver_data = NULL;
@@ -924,14 +926,13 @@ static int thor_eps_setup(struct usb_function
 *f) 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err;
 
 	ep->driver_data = cdev; /* claim */
 	req = thor_start_ep(ep);
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_in_ep;
 	}
 
 	dev->in_req = req;

drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function · line 948

Quoted 9 lines
drivers/usb/gadget/f_thor.c +2 -2
@@ -941,22 +942,34 @@ static int thor_eps_setup(struct usb_function
 *f) 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err_free_in_req;
 
 	ep->driver_data = cdev; /* claim */
-	req = thor_start_ep(ep);
+	req = usb_ep_alloc_request(ep, 0);

Is this safe to replace thor_start_ep() - which tunes the ep params - with generic function?

( I do see the req->complete = thor_rx_tx_complete below ).

If the thor_start_ep can be replaced with generic code, then maybe we can remove it?

Quoted 20 lines
drivers/usb/gadget/f_thor.c +8 -3
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_out_ep;
 	}
 
+	req->complete = thor_rx_tx_complete;
 	dev->out_req = req;
 	/* ACM control EP */
 	ep = dev->int_ep;
 	ep->driver_data = cdev;	/* claim */
 
- exit:
+	return 0;
+
+ err_disable_out_ep:
+	usb_ep_disable(dev->out_ep);
+
+ err_free_in_req:

drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function *f) · line 975

Quoted 10 lines
drivers/usb/gadget/f_thor.c +7 -0
+	free_ep_req(dev->in_ep, dev->in_req);
+	dev->in_req = NULL;
+
+ err_disable_in_ep:
+	usb_ep_disable(dev->in_ep);
+
+ err:
 	return result;
 }
 

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de -------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180525/618f82d7/attachment.sig>

#4 lore raw

PATCH v2 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer

Hello Lukasz,

Quoted 65 lines · Lukasz Majewski

Hi Seung-Woo,

Quoted 44 lines

There are memory leaks of usb request and its buffer for ep0, in_ep, and out ep. Fix memory leaks of usb request and its buffer.

---
Change from v1
- remove allocation of out_ep request instead of allocating and freeing
- fix use error path instead of duplicated error handling code

---
 drivers/usb/gadget/f_thor.c |   45

++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 16 deletions(-)

drivers/usb/gadget/f_thor.c +9 -2
index c8eda05..02d6844 100644
@@ -752,6 +752,13 @@ int thor_handle(void)
 	return 0;
 }
 
+static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
+{
+	if (req->buf)
+		free(req->buf);
+	usb_ep_free_request(ep, req);
+}
+
 static int thor_func_bind(struct usb_configuration *c, struct
 usb_function *f) {
 	struct usb_gadget *gadget = c->cdev->gadget;
@@ -860,21 +867,18 @@ static int thor_func_bind(struct
 usb_configuration *c, struct usb_function *f) return 0;
 
  fail:
+	if (dev->req)
+		free_ep_req(gadget->ep0, dev->req);
 	free(dev);
 	return status;
 }
 
-static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-{

drivers/usb/gadget/f_thor.c static int thor_func_bind(struct · line 881

Quoted 10 lines
drivers/usb/gadget/f_thor.c +1 -4
-	free(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static void thor_unbind(struct usb_configuration *c, struct
 usb_function *f) {
 	struct f_thor *f_thor = func_to_thor(f);
 	struct thor_dev *dev = f_thor->dev;
 
+	free_ep_req(dev->gadget->ep0, dev->req);

Till this change - no issues.

Quoted 6 lines
drivers/usb/gadget/f_thor.c +0 -0
 	free(dev);
 	memset(thor_func, 0, sizeof(*thor_func));
 	thor_func = NULL;
@@ -895,8 +899,6 @@ static void thor_func_disable(struct usb_function
 *f) }
 
Quoted 7 lines

drivers/usb/gadget/f_thor.c static void thor_func_disable(struct usb_function *f) · line 899

Quoted 3 lines
drivers/usb/gadget/f_thor.c +0 -2
 	if (dev->out_ep->driver_data) {
-		free(dev->out_req->buf);
-		dev->out_req->buf = NULL;

I think that this setting (to NULL) was needed to be able to ctrl+C from thor command and then run it again (as some code checks if buf is NULL).

From the comment about usb_ep_free_request(), it frees request object. So, it looks not required. Actually, dev->out_req = NULL; is more necessary, but in my test, ctrl-c or thor communication failure also flow till thor_unbind() where dev is also freed.

Quoted 28 lines
Quoted 28 lines
drivers/usb/gadget/f_thor.c +3 -4
 		usb_ep_free_request(dev->out_ep, dev->out_req);
 		usb_ep_disable(dev->out_ep);
 		dev->out_ep->driver_data = NULL;
@@ -924,14 +926,13 @@ static int thor_eps_setup(struct usb_function
 *f) 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err;
 
 	ep->driver_data = cdev; /* claim */
 	req = thor_start_ep(ep);
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_in_ep;
 	}
 
 	dev->in_req = req;
@@ -941,22 +942,34 @@ static int thor_eps_setup(struct usb_function
 *f) 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err_free_in_req;
 
Quoted 6 lines

drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function *f) · line 948

Quoted 3 lines
drivers/usb/gadget/f_thor.c +1 -1
 	ep->driver_data = cdev; /* claim */
-	req = thor_start_ep(ep);
+	req = usb_ep_alloc_request(ep, 0);

Is this safe to replace thor_start_ep() - which tunes the ep params - with generic function?

It is safe, because there is no tuning ep param. The function has 3 steps including usb_ep_alloc_request() and allocating buffer with memalign() and setting complete() callback to thor_rx_tx_complete(). For out_req, buffer allocation is not required because buffer for out_req is always set from thor_set_dma() usually with dfu_buffer before rx.

Quoted 5 lines

( I do see the req->complete = thor_rx_tx_complete below ).

If the thor_start_ep can be replaced with generic code, then maybe we can remove it?

It is possible to replace in_req case. If you prefer that, I will send v3 after replacing thor_start_ep() usage with generic functions.

Best Regards,
- Seung-Woo Kim

Quoted 45 lines
Quoted 21 lines
drivers/usb/gadget/f_thor.c +8 -3
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_out_ep;
 	}
 
+	req->complete = thor_rx_tx_complete;
 	dev->out_req = req;
 	/* ACM control EP */
 	ep = dev->int_ep;
 	ep->driver_data = cdev;	/* claim */
 
- exit:
+	return 0;
+
+ err_disable_out_ep:
+	usb_ep_disable(dev->out_ep);
+
+ err_free_in_req:

drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function *f) · line 975

Quoted 10 lines
drivers/usb/gadget/f_thor.c +7 -0
+	free_ep_req(dev->in_ep, dev->in_req);
+	dev->in_req = NULL;
+
+ err_disable_in_ep:
+	usb_ep_disable(dev->in_ep);
+
+ err:
 	return result;
 }
 

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

Signature · 3 lines
Seung-Woo Kim
Samsung Research
--
#5 lore raw

PATCH v3 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer

In reply to Seung-Woo Kim

There are memory leaks of usb request and its buffer for ep0, in_ep, and out ep. Fix memory leaks of usb request and its buffer.

---
Change from v2
- replace only once used local function, thor_start_ep(), with generic functions as Lukasz commented

Change from v1
- remove allocation of out_ep request instead of allocating and freeing
- fix use error path instead of duplicated error handling code
---

---
 drivers/usb/gadget/f_thor.c |   65 +++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 33 deletions(-)
drivers/usb/gadget/f_thor.c +32 -33
index c8eda05..1aa6be4 100644
@@ -620,22 +620,6 @@ static void thor_rx_tx_complete(struct usb_ep *ep, struct usb_request *req)
 	      status, req->actual, req->length);
 }
 
-static struct usb_request *thor_start_ep(struct usb_ep *ep)
-{
-	struct usb_request *req;
-
-	req = alloc_ep_req(ep, THOR_PACKET_SIZE);
-	debug("%s: ep:%p req:%p\n", __func__, ep, req);
-
-	if (!req)
-		return NULL;
-
-	memset(req->buf, 0, req->length);
-	req->complete = thor_rx_tx_complete;
-
-	return req;
-}
-
 static void thor_setup_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	if (req->status || req->actual != req->length)
@@ -752,6 +736,13 @@ int thor_handle(void)
 	return 0;
 }
 
+static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
+{
+	if (req->buf)
+		free(req->buf);
+	usb_ep_free_request(ep, req);
+}
+
 static int thor_func_bind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct usb_gadget *gadget = c->cdev->gadget;
@@ -860,21 +851,18 @@ static int thor_func_bind(struct usb_configuration *c, struct usb_function *f)
 	return 0;
 
  fail:
+	if (dev->req)
+		free_ep_req(gadget->ep0, dev->req);
 	free(dev);
 	return status;
 }
 
-static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
-{
-	free(req->buf);
-	usb_ep_free_request(ep, req);
-}
-
 static void thor_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_thor *f_thor = func_to_thor(f);
 	struct thor_dev *dev = f_thor->dev;
 
+	free_ep_req(dev->gadget->ep0, dev->req);
 	free(dev);
 	memset(thor_func, 0, sizeof(*thor_func));
 	thor_func = NULL;
@@ -895,8 +883,6 @@ static void thor_func_disable(struct usb_function *f)
 	}
 
 	if (dev->out_ep->driver_data) {
-		free(dev->out_req->buf);
-		dev->out_req->buf = NULL;
 		usb_ep_free_request(dev->out_ep, dev->out_req);
 		usb_ep_disable(dev->out_ep);
 		dev->out_ep->driver_data = NULL;
@@ -924,16 +910,17 @@ static int thor_eps_setup(struct usb_function *f)
 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err;
 
 	ep->driver_data = cdev; /* claim */
-	req = thor_start_ep(ep);
+	req = alloc_ep_req(ep, THOR_PACKET_SIZE);
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_in_ep;
 	}
 
+	memset(req->buf, 0, req->length);
+	req->complete = thor_rx_tx_complete;
 	dev->in_req = req;
 	ep = dev->out_ep;
 	d = ep_desc(gadget, &hs_out_desc, &fs_out_desc);
@@ -941,22 +928,34 @@ static int thor_eps_setup(struct usb_function *f)
 
 	result = usb_ep_enable(ep, d);
 	if (result)
-		goto exit;
+		goto err_free_in_req;
 
 	ep->driver_data = cdev; /* claim */
-	req = thor_start_ep(ep);
+	req = usb_ep_alloc_request(ep, 0);
 	if (!req) {
-		usb_ep_disable(ep);
 		result = -EIO;
-		goto exit;
+		goto err_disable_out_ep;
 	}
 
+	req->complete = thor_rx_tx_complete;
 	dev->out_req = req;
 	/* ACM control EP */
 	ep = dev->int_ep;
 	ep->driver_data = cdev;	/* claim */
 
- exit:
+	return 0;
+
+ err_disable_out_ep:
+	usb_ep_disable(dev->out_ep);
+
+ err_free_in_req:
+	free_ep_req(dev->in_ep, dev->in_req);
+	dev->in_req = NULL;
+
+ err_disable_in_ep:
+	usb_ep_disable(dev->in_ep);
+
+ err:
 	return result;
 }
 
Signature · 1 line
1.7.9.5