On Wed, Oct 15, 2014 at 10:41 AM, Robert Baldyga <r.baldyga@xxxxxxxxxxx> wrote:
> On 10/13/2014 04:25 PM, pavi1729 Sid wrote:
>> 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 and in case of error, freeing of the
>> allocated memory takes place inside the same call. Hence the
>> call in the *_bind functions are redundant.
>> The presence of usb_free_all_descriptors in the *_bind
>> functions might result in double-free corruption of the
>> allocated memory.
>
> You should update your commit message since it does not describe what
> exactly you do.
Agreed.
>
> Patch doesn't apply to latest next tree. You should rebase it, otherwise
> maintainer wouldn't accept it.
Will do that
>
>>
>> Signed-off-by: Pavitrakumar Managutte <pavitra1729@xxxxxxxxx>
>
> Code looks good for me. You can add my reviewed-by under your sign-off.
> Reviewed-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
Done
>
>> ---
>> drivers/usb/gadget/function/f_eem.c | 1 -
>> drivers/usb/gadget/function/f_hid.c | 5 +++--
>> drivers/usb/gadget/function/f_ncm.c | 1 -
>> drivers/usb/gadget/function/f_obex.c | 1 -
>> drivers/usb/gadget/function/f_phonet.c | 2 +-
>> drivers/usb/gadget/function/f_rndis.c | 5 +++--
>> drivers/usb/gadget/function/f_subset.c | 1 -
>> drivers/usb/gadget/function/f_uac2.c | 10 ++++++----
>> 8 files changed, 13 insertions(+), 13 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..59ab62c 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -621,12 +621,14 @@ static int __init hidg_bind(struct
>> usb_configuration *c, struct usb_function *f)
>> dev = MKDEV(major, hidg->minor);
>> status = cdev_add(&hidg->cdev, dev, 1);
>> if (status)
>> - goto fail;
>> + goto fail_free_descs;
>>
>> device_create(hidg_class, NULL, dev, NULL, "%s%d", "hidg", hidg->minor);
>>
>> return 0;
>>
>> +fail_free_descs:
>> + usb_free_all_descriptors(f);
>> fail:
>> ERROR(f->config->cdev, "hidg_bind FAILED\n");
>> if (hidg->req != NULL) {
>> @@ -635,7 +637,6 @@ fail:
>> usb_ep_free_request(hidg->in_ep, hidg->req);
>> }
>>
>> - usb_free_all_descriptors(f);
>> return status;
>> }
>>
>> 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_obex.c
>> b/drivers/usb/gadget/function/f_obex.c
>> index aebae18..9f7148d 100644
>> --- a/drivers/usb/gadget/function/f_obex.c
>> +++ b/drivers/usb/gadget/function/f_obex.c
>> @@ -391,7 +391,6 @@ static int obex_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 (obex->port.out)
>> obex->port.out->driver_data = NULL;
>> diff --git a/drivers/usb/gadget/function/f_phonet.c
>> b/drivers/usb/gadget/function/f_phonet.c
>> index b9cfc15..1ec8b7f 100644
>> --- a/drivers/usb/gadget/function/f_phonet.c
>> +++ b/drivers/usb/gadget/function/f_phonet.c
>> @@ -570,8 +570,8 @@ static int pn_bind(struct usb_configuration *c,
>> struct usb_function *f)
>> 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);
>> +err:
>> 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..2f0517f 100644
>> --- a/drivers/usb/gadget/function/f_rndis.c
>> +++ b/drivers/usb/gadget/function/f_rndis.c
>> @@ -803,7 +803,7 @@ rndis_bind(struct usb_configuration *c, struct
>> usb_function *f)
>> if (rndis->manufacturer && rndis->vendorID &&
>> rndis_set_param_vendor(rndis->config, rndis->vendorID,
>> rndis->manufacturer))
>> - goto fail;
>> + goto fail_free_descs;
>>
>> /* NOTE: all that is done without knowing or caring about
>> * the network link ... which is unavailable to this code
>> @@ -817,10 +817,11 @@ rndis_bind(struct usb_configuration *c, struct
>> usb_function *f)
>> rndis->notify->name);
>> return 0;
>>
>> +fail_free_descs:
>> + usb_free_all_descriptors(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..b45c39c 100644
>> --- a/drivers/usb/gadget/function/f_uac2.c
>> +++ b/drivers/usb/gadget/function/f_uac2.c
>> @@ -994,7 +994,7 @@ afunc_bind(struct usb_configuration *cfg, struct
>> usb_function *fn)
>> prm->rbuf = kzalloc(prm->max_psize * USB_XFERS, GFP_KERNEL);
>> if (!prm->rbuf) {
>> prm->max_psize = 0;
>> - goto err;
>> + goto err_free_descs;
>> }
>>
>> prm = &agdev->uac2.p_prm;
>> @@ -1002,17 +1002,19 @@ afunc_bind(struct usb_configuration *cfg,
>> struct usb_function *fn)
>> prm->rbuf = kzalloc(prm->max_psize * USB_XFERS, GFP_KERNEL);
>> if (!prm->rbuf) {
>> prm->max_psize = 0;
>> - goto err;
>> + goto err_free_descs;
>> }
>>
>> ret = alsa_uac2_init(agdev);
>> if (ret)
>> - goto err;
>> + goto err_free_descs;
>> return 0;
>> +
>> +err_free_descs:
>> + usb_free_all_descriptors(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
