Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751782AbdHPPYy (ORCPT ); Wed, 16 Aug 2017 11:24:54 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:49902 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751531AbdHPPYx (ORCPT ); Wed, 16 Aug 2017 11:24:53 -0400 Date: Wed, 16 Aug 2017 11:24:52 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Alexey Khoroshilov cc: Felipe Balbi , Greg Kroah-Hartman , Krzysztof Opasiak , Anton Vasilyev , , , Subject: Re: Inconsistency in usb_add_gadget_udc_release() interface In-Reply-To: <1502833167-19354-1-git-send-email-khoroshilov@ispras.ru> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1772 Lines: 67 On Wed, 16 Aug 2017, Alexey Khoroshilov wrote: > Hello, > > usb_add_gadget_udc_release() gets release() argument that allows to > release user resources. > > As far as I can see, the release() is called on error paths > of usb_add_gadget_udc_release() as a result of > put_device(&gadget->dev); > except for the only path going via err1. > > As a result a caller of the usb_add_gadget_udc_release() have no chance > to know if the release() was invoked or not. > > It may lead to memory leaks (drivers/usb/gadget/udc/snps_udc_core.c) > or to double free (drivers/usb/gadget/udc/fsl_udc_core.c). > > Is my reading correct? If so, should we always call release() on error paths? How about this (untested)? Alan Stern Index: usb-4.x/drivers/usb/gadget/udc/core.c =================================================================== --- usb-4.x.orig/drivers/usb/gadget/udc/core.c +++ usb-4.x/drivers/usb/gadget/udc/core.c @@ -1137,10 +1137,6 @@ int usb_add_gadget_udc_release(struct de struct usb_udc *udc; int ret = -ENOMEM; - udc = kzalloc(sizeof(*udc), GFP_KERNEL); - if (!udc) - goto err1; - dev_set_name(&gadget->dev, "gadget"); INIT_WORK(&gadget->work, usb_gadget_state_work); gadget->dev.parent = parent; @@ -1150,7 +1146,13 @@ int usb_add_gadget_udc_release(struct de else gadget->dev.release = usb_udc_nop_release; - ret = device_register(&gadget->dev); + device_initialize(&gadget->dev); + + udc = kzalloc(sizeof(*udc), GFP_KERNEL); + if (!udc) + goto err1; + + ret = device_add(&gadget->dev); if (ret) goto err2; @@ -1197,10 +1199,10 @@ err3: device_del(&gadget->dev); err2: - put_device(&gadget->dev); kfree(udc); err1: + put_device(&gadget->dev); return ret; } EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);