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

Groups > linux.kernel > #1266674 > unrolled thread

[PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests "Felipe F. Tonello" <eu@felipetonello.com> - 2015-11-10 19:00 +0100
    Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue  out requests Robert Baldyga <r.baldyga@samsung.com> - 2015-11-13 09:40 +0100
      Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue  out requests Felipe Ferreri Tonello <eu@felipetonello.com> - 2015-11-16 12:10 +0100
        Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue  out requests Robert Baldyga <r.baldyga@samsung.com> - 2015-11-16 12:50 +0100
          Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue  out requests Felipe Ferreri Tonello <eu@felipetonello.com> - 2015-11-25 14:10 +0100

#1266674 — [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

This patch fixes a memory leak that occurs when an endpoint fails to enqueue
the request. If that happens the complete function will never be called, thus
never freeing the request.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index f36db2d..76ea53c 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		if (err) {
 			ERROR(midi, "%s queue req: %d\n",
 				    midi->out_ep->name, err);
+			free_ep_req(midi->out_ep, req);
 		}
 	}
 
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1268694 — Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

Hi Felipe,

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
> the request. If that happens the complete function will never be called, thus
> never freeing the request.
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/usb/gadget/function/f_midi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index f36db2d..76ea53c 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  		if (err) {
>  			ERROR(midi, "%s queue req: %d\n",
>  				    midi->out_ep->name, err);
> +			free_ep_req(midi->out_ep, req);
>  		}
>  	}
>  
> 

There is one more thing I haven't noticed before. We can have situation
when all requests were allocated successfully, but their allocation
failed. What we get then is set_alt() returning 0, while no request is
allocated, hence the function is, in fact, inactive.

Best regards,
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1270005 — Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

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

Hi Robert,

On 13/11/15 08:31, Robert Baldyga wrote:
> Hi Felipe,
> 
> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>> the request. If that happens the complete function will never be called, thus
>> never freeing the request.
>>
>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>> index f36db2d..76ea53c 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>  		if (err) {
>>  			ERROR(midi, "%s queue req: %d\n",
>>  				    midi->out_ep->name, err);
>> +			free_ep_req(midi->out_ep, req);
>>  		}
>>  	}
>>  
>>
> 
> There is one more thing I haven't noticed before. We can have situation
> when all requests were allocated successfully, but their allocation
> failed. What we get then is set_alt() returning 0, while no request is
> allocated, hence the function is, in fact, inactive.

Right. So in this case should we return some error? We can restrict the
function to work iff allocates the 'qlen' number of allocations,
otherwise returns an error and frees all other requests (IN and OUT).

-- 
Felipe

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

#1270033 — Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

On 11/16/2015 12:08 PM, Felipe Ferreri Tonello wrote:
> Hi Robert,
> 
> On 13/11/15 08:31, Robert Baldyga wrote:
>> Hi Felipe,
>>
>> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>>> the request. If that happens the complete function will never be called, thus
>>> never freeing the request.
>>>
>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>> ---
>>>  drivers/usb/gadget/function/f_midi.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>> index f36db2d..76ea53c 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>  		if (err) {
>>>  			ERROR(midi, "%s queue req: %d\n",
>>>  				    midi->out_ep->name, err);
>>> +			free_ep_req(midi->out_ep, req);
>>>  		}
>>>  	}
>>>  
>>>
>>
>> There is one more thing I haven't noticed before. We can have situation
>> when all requests were allocated successfully, but their allocation
>> failed. What we get then is set_alt() returning 0, while no request is
>> allocated, hence the function is, in fact, inactive.
> 
> Right. So in this case should we return some error? We can restrict the
> function to work iff allocates the 'qlen' number of allocations,
> otherwise returns an error and frees all other requests (IN and OUT).
> 

Yes, IMO it's a proper solution. When user sets qlen to given value he
expects that exact number of requests to be allocated and enqueued, and
if we cannot do that we should consider this as an error.

--
Best regards,
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1277375 — Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

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

Hi Robert,

On 16/11/15 11:43, Robert Baldyga wrote:
> On 11/16/2015 12:08 PM, Felipe Ferreri Tonello wrote:
>> Hi Robert,
>>
>> On 13/11/15 08:31, Robert Baldyga wrote:
>>> Hi Felipe,
>>>
>>> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>>>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>>>> the request. If that happens the complete function will never be called, thus
>>>> never freeing the request.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
>>>> ---
>>>>  drivers/usb/gadget/function/f_midi.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
>>>> index f36db2d..76ea53c 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>>>>  		if (err) {
>>>>  			ERROR(midi, "%s queue req: %d\n",
>>>>  				    midi->out_ep->name, err);
>>>> +			free_ep_req(midi->out_ep, req);
>>>>  		}
>>>>  	}
>>>>  
>>>>
>>>
>>> There is one more thing I haven't noticed before. We can have situation
>>> when all requests were allocated successfully, but their allocation
>>> failed. What we get then is set_alt() returning 0, while no request is
>>> allocated, hence the function is, in fact, inactive.
>>
>> Right. So in this case should we return some error? We can restrict the
>> function to work iff allocates the 'qlen' number of allocations,
>> otherwise returns an error and frees all other requests (IN and OUT).
>>
> 
> Yes, IMO it's a proper solution. When user sets qlen to given value he
> expects that exact number of requests to be allocated and enqueued, and
> if we cannot do that we should consider this as an error.
> 

Ok. I will do that in a new patch on v6 since this patch is correct
anyway and Blabi already applied to his test branch.

Felipe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web