| Message ID | 1528968239-2447-2-git-send-email-climbbb.kim@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 14 Jun 2018, Jaejoong Kim wrote: > The reference count is initialized in fsg_alloc_inst(). So if > fsg_alloc_inst() fails, we need to add kref_put() to free an > allocated memory. > > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> > --- > drivers/usb/gadget/function/f_mass_storage.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index acecd13..83eeafe 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -3392,6 +3392,7 @@ static struct usb_function_instance *fsg_alloc_inst(void) > release_buffers: > fsg_common_free_buffers(opts->common); > release_opts: > + fsg_common_put(opts->common); > kfree(opts); > return ERR_PTR(rc); > } I'm not at all sure about this. Michal is a lot more familiar with this part of the code than I am. It looks like there's a memory leak if fsg_common_release() isn't called here. But if it is called at this point, it might not behave properly. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-06-22 2:23 GMT+01:00 Jaejoong Kim <climbbb.kim@gmail.com>: > Hi Michal, > > Could you check this patch? Sorry about long delay, I haven’t settled in my new place yet (which is also why I’m using Gmail). > 2018년 6월 14일 (목) 오후 11:55, Alan Stern <stern@rowland.harvard.edu>님이 작성: >> >> On Thu, 14 Jun 2018, Jaejoong Kim wrote: >> >> > The reference count is initialized in fsg_alloc_inst(). So if >> > fsg_alloc_inst() fails, we need to add kref_put() to free an >> > allocated memory. >> > >> > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> >> > --- >> > drivers/usb/gadget/function/f_mass_storage.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c >> > b/drivers/usb/gadget/function/f_mass_storage.c >> > index acecd13..83eeafe 100644 >> > --- a/drivers/usb/gadget/function/f_mass_storage.c >> > +++ b/drivers/usb/gadget/function/f_mass_storage.c >> > @@ -3392,6 +3392,7 @@ static struct usb_function_instance >> > *fsg_alloc_inst(void) >> > release_buffers: >> > fsg_common_free_buffers(opts->common); >> > release_opts: >> > + fsg_common_put(opts->common); >> > kfree(opts); >> > return ERR_PTR(rc); >> > } This looks legitimate but in the current form could lead to dereference of an uninitialized pointer. Furthermore, calling free_buffers is no longer necessary. To solve the dereference problem, add commo->buffhds = NULL; to fsg_common_setup (or add memset(common, 0, sizeof *common) to the path without kzalloc). With that, the the two error handling labels can be collapsed to one: error: fsg_common_put(opts->common); kfree(opts); return ERR_PTR(rc); >> >> I'm not at all sure about this. Michal is a lot more familiar with >> this part of the code than I am. >> >> It looks like there's a memory leak if fsg_common_release() isn't >> called here. But if it is called at this point, it might not behave >> properly. >> >> Alan Stern >> > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index acecd13..83eeafe 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -3392,6 +3392,7 @@ static struct usb_function_instance *fsg_alloc_inst(void) release_buffers: fsg_common_free_buffers(opts->common); release_opts: + fsg_common_put(opts->common); kfree(opts); return ERR_PTR(rc); }
The reference count is initialized in fsg_alloc_inst(). So if fsg_alloc_inst() fails, we need to add kref_put() to free an allocated memory. Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> --- drivers/usb/gadget/function/f_mass_storage.c | 1 + 1 file changed, 1 insertion(+)