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