Oh! precisely what you are saying ..
On Fri, Oct 10, 2014 at 6:21 PM, pavi1729 ivap <pavitra1729@xxxxxxxxx> wrote:
> Rob,
> Got your point, will submit a new patch with a new error label.
> Also I just found that in f_hid.c f_rndis.c and f_uac2.c do need the
> freeing function.
> Will clean it up and send the patch.
>
> Thanks,
> Pavitra
>
> On Fri, Oct 10, 2014 at 6:05 PM, Robert Baldyga <r.baldyga@xxxxxxxxxxx> wrote:
>> Hi,
>>
>> On 10/10/2014 01:51 PM, pavi1729 ivap wrote:
>>>>From 3272817673910c63682e8b91ce0efaef190a399a Mon Sep 17 00:00:00 2001
>>> From: Pavitra <pavitra1729@xxxxxxxxx>
>>> Date: Fri, 10 Oct 2014 16:05:30 +0530
>>> Subject: [PATCH] usb: gadget: function: Remove redundant
>>> usb_free_all_descriptors
>>>
>>> Removed the usb_free_all_descriptors call in *_bind functions
>>> as this call is already present in usb_assign_descriptors.
>>> usb_assign_descriptors is the only call where usb descriptor
>>> allocation happens, also in case of error freeing of the
>>> allocated memory takes place in the same call. Hence the
>>> call in the *_bind functions are redundant.
>>> Also the presence of usb_free_all_descriptors in the *_bind
>>> functions might result in double-free corruption of the
>>> allocated mmeory.
>>
>> Double-free corruption is good point in this place, but your patch
>> doesn't solve this problem properly. If usb_assign_descriptors() returns
>> without error and one of following calls fails, we need to call
>> usb_free_all_descriptors() in error handling code.
>>
>> There should be rather two error labels: one where you goto on errors
>> before usb_assign_descriptors() call, and another one where you goto on
>> errors after it. And in second case usb_free_all_descriptors() is
>> called, because descriptors are already allocated and you need to free them.
>>
>> I also see that in some drivers usb_assign_descriptors() is the last
>> call in bind() and then removing usb_free_all_descriptors() from error
>> handling code makes sense.
>>
>> Best regards,
>> Robert Baldyga
>>
>>>
>>> Signed-off-by: Pavitra <pavitra1729@xxxxxxxxx>
>>> ---
>>> drivers/usb/gadget/function/f_eem.c | 1 -
>>> drivers/usb/gadget/function/f_hid.c | 2 --
>>> drivers/usb/gadget/function/f_ncm.c | 1 -
>>> drivers/usb/gadget/function/f_phonet.c | 1 -
>>> drivers/usb/gadget/function/f_rndis.c | 1 -
>>> drivers/usb/gadget/function/f_subset.c | 1 -
>>> drivers/usb/gadget/function/f_uac2.c | 1 -
>>> 7 files changed, 8 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_eem.c
>>> b/drivers/usb/gadget/function/f_eem.c
>>> index 4d8b236..c9e90de 100644
>>> --- a/drivers/usb/gadget/function/f_eem.c
>>> +++ b/drivers/usb/gadget/function/f_eem.c
>>> @@ -325,7 +325,6 @@ static int eem_bind(struct usb_configuration *c,
>>> struct usb_function *f)
>>> return 0;
>>>
>>> fail:
>>> - usb_free_all_descriptors(f);
>>> if (eem->port.out_ep)
>>> eem->port.out_ep->driver_data = NULL;
>>> if (eem->port.in_ep)
>>> diff --git a/drivers/usb/gadget/function/f_hid.c
>>> b/drivers/usb/gadget/function/f_hid.c
>>> index a95290a..5c67d28 100644
>>> --- a/drivers/usb/gadget/function/f_hid.c
>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>> @@ -652,8 +652,6 @@ static void hidg_unbind(struct usb_configuration
>>> *c, struct usb_function *f)
>>> kfree(hidg->req->buf);
>>> usb_ep_free_request(hidg->in_ep, hidg->req);
>>>
>>> - usb_free_all_descriptors(f);
>>> -
>>> kfree(hidg->report_desc);
>>> kfree(hidg);
>>> }
>>> diff --git a/drivers/usb/gadget/function/f_ncm.c
>>> b/drivers/usb/gadget/function/f_ncm.c
>>> index bcdc882..38a9279 100644
>>> --- a/drivers/usb/gadget/function/f_ncm.c
>>> +++ b/drivers/usb/gadget/function/f_ncm.c
>>> @@ -1453,7 +1453,6 @@ static int ncm_bind(struct usb_configuration *c,
>>> struct usb_function *f)
>>> return 0;
>>>
>>> fail:
>>> - usb_free_all_descriptors(f);
>>> if (ncm->notify_req) {
>>> kfree(ncm->notify_req->buf);
>>> usb_ep_free_request(ncm->notify, ncm->notify_req);
>>> diff --git a/drivers/usb/gadget/function/f_phonet.c
>>> b/drivers/usb/gadget/function/f_phonet.c
>>> index b9cfc15..74ddcac 100644
>>> --- a/drivers/usb/gadget/function/f_phonet.c
>>> +++ b/drivers/usb/gadget/function/f_phonet.c
>>> @@ -571,7 +571,6 @@ err_req:
>>> for (i = 0; i < phonet_rxq_size && fp->out_reqv[i]; i++)
>>> usb_ep_free_request(fp->out_ep, fp->out_reqv[i]);
>>> err:
>>> - usb_free_all_descriptors(f);
>>> if (fp->out_ep)
>>> fp->out_ep->driver_data = NULL;
>>> if (fp->in_ep)
>>> diff --git a/drivers/usb/gadget/function/f_rndis.c
>>> b/drivers/usb/gadget/function/f_rndis.c
>>> index ddb09dc..e257eff 100644
>>> --- a/drivers/usb/gadget/function/f_rndis.c
>>> +++ b/drivers/usb/gadget/function/f_rndis.c
>>> @@ -820,7 +820,6 @@ rndis_bind(struct usb_configuration *c, struct
>>> usb_function *f)
>>> fail:
>>> kfree(f->os_desc_table);
>>> f->os_desc_n = 0;
>>> - usb_free_all_descriptors(f);
>>>
>>> if (rndis->notify_req) {
>>> kfree(rndis->notify_req->buf);
>>> diff --git a/drivers/usb/gadget/function/f_subset.c
>>> b/drivers/usb/gadget/function/f_subset.c
>>> index 1ea8baf..e3dfa67 100644
>>> --- a/drivers/usb/gadget/function/f_subset.c
>>> +++ b/drivers/usb/gadget/function/f_subset.c
>>> @@ -380,7 +380,6 @@ geth_bind(struct usb_configuration *c, struct
>>> usb_function *f)
>>> return 0;
>>>
>>> fail:
>>> - usb_free_all_descriptors(f);
>>> /* we might as well release our claims on endpoints */
>>> if (geth->port.out_ep)
>>> geth->port.out_ep->driver_data = NULL;
>>> diff --git a/drivers/usb/gadget/function/f_uac2.c
>>> b/drivers/usb/gadget/function/f_uac2.c
>>> index 3ed89ec..c294fb9 100644
>>> --- a/drivers/usb/gadget/function/f_uac2.c
>>> +++ b/drivers/usb/gadget/function/f_uac2.c
>>> @@ -1012,7 +1012,6 @@ afunc_bind(struct usb_configuration *cfg, struct
>>> usb_function *fn)
>>> err:
>>> kfree(agdev->uac2.p_prm.rbuf);
>>> kfree(agdev->uac2.c_prm.rbuf);
>>> - usb_free_all_descriptors(fn);
>>> if (agdev->in_ep)
>>> agdev->in_ep->driver_data = NULL;
>>> if (agdev->out_ep)
>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
