* [PATCH 04/14] memstick: core: fix device_register() error handling @ 2010-09-19 12:54 Vasiliy Kulikov 2010-09-21 22:20 ` Andrew Morton 0 siblings, 1 reply; 13+ messages in thread From: Vasiliy Kulikov @ 2010-09-19 12:54 UTC (permalink / raw) To: kernel-janitors; +Cc: Andrew Morton, Tejun Heo, Jiri Slaby, linux-kernel If device_register() fails then call put_device(). See comment to device_register. Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> --- compile tested. drivers/memstick/core/memstick.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c index c00fe82..4303b7e 100644 --- a/drivers/memstick/core/memstick.c +++ b/drivers/memstick/core/memstick.c @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work) if (!host->card) { host->card = card; if (device_register(&card->dev)) { + put_device(&card->dev); kfree(host->card); host->card = NULL; } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-19 12:54 [PATCH 04/14] memstick: core: fix device_register() error handling Vasiliy Kulikov @ 2010-09-21 22:20 ` Andrew Morton 2010-09-21 22:49 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: Andrew Morton @ 2010-09-21 22:20 UTC (permalink / raw) To: Vasiliy Kulikov Cc: kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, Greg KH On Sun, 19 Sep 2010 16:54:49 +0400 Vasiliy Kulikov <segooon@gmail.com> wrote: > If device_register() fails then call put_device(). > See comment to device_register. > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> > --- > compile tested. > > drivers/memstick/core/memstick.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c > index c00fe82..4303b7e 100644 > --- a/drivers/memstick/core/memstick.c > +++ b/drivers/memstick/core/memstick.c > @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work) > if (!host->card) { > host->card = card; > if (device_register(&card->dev)) { > + put_device(&card->dev); > kfree(host->card); > host->card = NULL; > } A failed device_register() takes a bogus ref on the not-registered device? It's no surprise that people are getting this wrong. The principle of least surprise says: fix device_register()! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-21 22:20 ` Andrew Morton @ 2010-09-21 22:49 ` Greg KH 2010-09-22 8:53 ` Kay Sievers 2010-09-22 9:58 ` Boaz Harrosh 0 siblings, 2 replies; 13+ messages in thread From: Greg KH @ 2010-09-21 22:49 UTC (permalink / raw) To: Andrew Morton, Kay Sievers Cc: Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter, Boaz Harrosh On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote: > On Sun, 19 Sep 2010 16:54:49 +0400 > Vasiliy Kulikov <segooon@gmail.com> wrote: > > > If device_register() fails then call put_device(). > > See comment to device_register. > > > > Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> > > --- > > compile tested. > > > > drivers/memstick/core/memstick.c | 1 + > > 1 files changed, 1 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c > > index c00fe82..4303b7e 100644 > > --- a/drivers/memstick/core/memstick.c > > +++ b/drivers/memstick/core/memstick.c > > @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work) > > if (!host->card) { > > host->card = card; > > if (device_register(&card->dev)) { > > + put_device(&card->dev); > > kfree(host->card); > > host->card = NULL; > > } > > A failed device_register() takes a bogus ref on the not-registered > device? It's no surprise that people are getting this wrong. > > The principle of least surprise says: fix device_register()! One might think that, but it's a bit more difficult. How does device_register know it should destroy the device if it fails? Here's how it works: - device_register is just a wrapper around device_initialize() and device_add() - device_initialize() can't do anything wrong, so it's safe, BUT, at this point in time, the reference for the device is incremented, so any caller must now drop the reference and properly free stuff. - device_add() does a lot. Hm, I guess, because we "know" in device_register() that we must drop something if device_add() fails, then I guess it's not being consistant with it's own calls... So, something as simple as this? diff --git a/drivers/base/core.c b/drivers/base/core.c index d1b2c9a..4ba8599 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1084,14 +1084,16 @@ name_error: * have a clearly defined need to use and refcount the device * before it is added to the hierarchy. * - * NOTE: _Never_ directly free @dev after calling this function, even - * if it returned an error! Always use put_device() to give up the - * reference initialized in this function instead. */ int device_register(struct device *dev) { + int retval; + device_initialize(dev); - return device_add(dev); + retval = device_add(dev); + if (retval) + put_device(dev); + return retval; } /** Kay, what am I missing here, why can't we just do this? Hm, the side-affect might be that if device_register() fails, NO ONE had better touch that device again, as it might have just been freed from the system. I wonder if that will cause problems... thanks, greg k-h ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-21 22:49 ` Greg KH @ 2010-09-22 8:53 ` Kay Sievers 2010-09-22 10:02 ` Boaz Harrosh 2010-09-22 15:50 ` Greg KH 2010-09-22 9:58 ` Boaz Harrosh 1 sibling, 2 replies; 13+ messages in thread From: Kay Sievers @ 2010-09-22 8:53 UTC (permalink / raw) To: Greg KH Cc: Andrew Morton, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter, Boaz Harrosh On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote: > int device_register(struct device *dev) > { > + int retval; > + > device_initialize(dev); > - return device_add(dev); > + retval = device_add(dev); > + if (retval) > + put_device(dev); > + return retval; > } > Kay, what am I missing here, why can't we just do this? Hm, the > side-affect might be that if device_register() fails, NO ONE had better > touch that device again, as it might have just been freed from the > system. I wonder if that will cause problems... That looks right, besides that there might be callers already doing this. Which needs to be checked. I never liked this pretty useless "convenience API", which just wraps two simple functions and the first one can never fail anyway. We better remove that device_register() stuff entirely in the long run, it's not doing any good. At the kobject level we killed the same stuff already long ago. Kay ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 8:53 ` Kay Sievers @ 2010-09-22 10:02 ` Boaz Harrosh 2010-09-22 15:47 ` Greg KH 2010-09-22 15:50 ` Greg KH 1 sibling, 1 reply; 13+ messages in thread From: Boaz Harrosh @ 2010-09-22 10:02 UTC (permalink / raw) To: Kay Sievers Cc: Greg KH, Andrew Morton, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter On 09/22/2010 10:53 AM, Kay Sievers wrote: > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote: > >> int device_register(struct device *dev) >> { >> + int retval; >> + >> device_initialize(dev); >> - return device_add(dev); >> + retval = device_add(dev); >> + if (retval) >> + put_device(dev); >> + return retval; >> } > >> Kay, what am I missing here, why can't we just do this? Hm, the >> side-affect might be that if device_register() fails, NO ONE had better >> touch that device again, as it might have just been freed from the >> system. I wonder if that will cause problems... > > That looks right, besides that there might be callers already doing > this. Which needs to be checked. > > I never liked this pretty useless "convenience API", which just wraps > two simple functions and the first one can never fail anyway. > > We better remove that device_register() stuff entirely in the long > run, it's not doing any good. At the kobject level we killed the same > stuff already long ago. > That would be fine, and ping me when you do it, I'll help with my driver. But don't forget to let us have a way to embed a device inside a bigger structure. For meanwhile Please check the patch James sent to add_device that cleans up the allocation of the kobj.name member. (And the comment made there) > Kay Thanks Boaz ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 10:02 ` Boaz Harrosh @ 2010-09-22 15:47 ` Greg KH 2010-09-22 15:56 ` James Bottomley 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2010-09-22 15:47 UTC (permalink / raw) To: Boaz Harrosh Cc: Kay Sievers, Andrew Morton, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote: > On 09/22/2010 10:53 AM, Kay Sievers wrote: > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote: > > > >> int device_register(struct device *dev) > >> { > >> + int retval; > >> + > >> device_initialize(dev); > >> - return device_add(dev); > >> + retval = device_add(dev); > >> + if (retval) > >> + put_device(dev); > >> + return retval; > >> } > > > >> Kay, what am I missing here, why can't we just do this? Hm, the > >> side-affect might be that if device_register() fails, NO ONE had better > >> touch that device again, as it might have just been freed from the > >> system. I wonder if that will cause problems... > > > > That looks right, besides that there might be callers already doing > > this. Which needs to be checked. > > > > I never liked this pretty useless "convenience API", which just wraps > > two simple functions and the first one can never fail anyway. > > > > We better remove that device_register() stuff entirely in the long > > run, it's not doing any good. At the kobject level we killed the same > > stuff already long ago. > > > > That would be fine, and ping me when you do it, I'll help with my > driver. But don't forget to let us have a way to embed a device inside > a bigger structure. That's what you should be doing anyway, so it shouldn't be a problem. > For meanwhile Please check the patch James sent to add_device that cleans > up the allocation of the kobj.name member. (And the comment made there) See my other email why that isn't a good idea. thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 15:47 ` Greg KH @ 2010-09-22 15:56 ` James Bottomley 2010-09-22 16:20 ` Greg KH 0 siblings, 1 reply; 13+ messages in thread From: James Bottomley @ 2010-09-22 15:56 UTC (permalink / raw) To: Greg KH Cc: Boaz Harrosh, Kay Sievers, Andrew Morton, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, Dan Carpenter On Wed, 2010-09-22 at 08:47 -0700, Greg KH wrote: > On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote: > > On 09/22/2010 10:53 AM, Kay Sievers wrote: > > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote: > > > > > >> int device_register(struct device *dev) > > >> { > > >> + int retval; > > >> + > > >> device_initialize(dev); > > >> - return device_add(dev); > > >> + retval = device_add(dev); > > >> + if (retval) > > >> + put_device(dev); > > >> + return retval; > > >> } > > > > > >> Kay, what am I missing here, why can't we just do this? Hm, the > > >> side-affect might be that if device_register() fails, NO ONE had better > > >> touch that device again, as it might have just been freed from the > > >> system. I wonder if that will cause problems... > > > > > > That looks right, besides that there might be callers already doing > > > this. Which needs to be checked. > > > > > > I never liked this pretty useless "convenience API", which just wraps > > > two simple functions and the first one can never fail anyway. > > > > > > We better remove that device_register() stuff entirely in the long > > > run, it's not doing any good. At the kobject level we killed the same > > > stuff already long ago. > > > > > > > That would be fine, and ping me when you do it, I'll help with my > > driver. But don't forget to let us have a way to embed a device inside > > a bigger structure. > > That's what you should be doing anyway, so it shouldn't be a problem. > > > For meanwhile Please check the patch James sent to add_device that cleans > > up the allocation of the kobj.name member. (And the comment made there) > > See my other email why that isn't a good idea. What other email? ... neither you nor Kay replied after either Boaz's patch or the corrected one were posted. James ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 15:56 ` James Bottomley @ 2010-09-22 16:20 ` Greg KH 2010-09-22 16:23 ` James Bottomley 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2010-09-22 16:20 UTC (permalink / raw) To: James Bottomley Cc: Boaz Harrosh, Kay Sievers, Andrew Morton, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, Dan Carpenter On Wed, Sep 22, 2010 at 10:56:36AM -0500, James Bottomley wrote: > On Wed, 2010-09-22 at 08:47 -0700, Greg KH wrote: > > On Wed, Sep 22, 2010 at 12:02:30PM +0200, Boaz Harrosh wrote: > > > On 09/22/2010 10:53 AM, Kay Sievers wrote: > > > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote: > > > > > > > >> int device_register(struct device *dev) > > > >> { > > > >> + int retval; > > > >> + > > > >> device_initialize(dev); > > > >> - return device_add(dev); > > > >> + retval = device_add(dev); > > > >> + if (retval) > > > >> + put_device(dev); > > > >> + return retval; > > > >> } > > > > > > > >> Kay, what am I missing here, why can't we just do this? Hm, the > > > >> side-affect might be that if device_register() fails, NO ONE had better > > > >> touch that device again, as it might have just been freed from the > > > >> system. I wonder if that will cause problems... > > > > > > > > That looks right, besides that there might be callers already doing > > > > this. Which needs to be checked. > > > > > > > > I never liked this pretty useless "convenience API", which just wraps > > > > two simple functions and the first one can never fail anyway. > > > > > > > > We better remove that device_register() stuff entirely in the long > > > > run, it's not doing any good. At the kobject level we killed the same > > > > stuff already long ago. > > > > > > > > > > That would be fine, and ping me when you do it, I'll help with my > > > driver. But don't forget to let us have a way to embed a device inside > > > a bigger structure. > > > > That's what you should be doing anyway, so it shouldn't be a problem. > > > > > For meanwhile Please check the patch James sent to add_device that cleans > > > up the allocation of the kobj.name member. (And the comment made there) > > > > See my other email why that isn't a good idea. > > What other email? ... neither you nor Kay replied after either Boaz's > patch or the corrected one were posted. See my other email a minute ago in response to this thread. thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 16:20 ` Greg KH @ 2010-09-22 16:23 ` James Bottomley 0 siblings, 0 replies; 13+ messages in thread From: James Bottomley @ 2010-09-22 16:23 UTC (permalink / raw) To: Greg KH Cc: Boaz Harrosh, Kay Sievers, Andrew Morton, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, Dan Carpenter On Wed, 2010-09-22 at 09:20 -0700, Greg KH wrote: > > What other email? ... neither you nor Kay replied after either Boaz's > > patch or the corrected one were posted. > > See my other email a minute ago in response to this thread. Can we start again? You've been replying to a patch to do a put_device() in device_register() which everyone agrees looks wrong. The proposal to fix all of this is to free the kobject name and NULL it out in device_add(). That fixes all the places you and kay broke when you added the variable length name stuff. James ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 8:53 ` Kay Sievers 2010-09-22 10:02 ` Boaz Harrosh @ 2010-09-22 15:50 ` Greg KH 2010-09-23 12:10 ` Vasiliy Kulikov 1 sibling, 1 reply; 13+ messages in thread From: Greg KH @ 2010-09-22 15:50 UTC (permalink / raw) To: Kay Sievers Cc: Andrew Morton, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter, Boaz Harrosh On Wed, Sep 22, 2010 at 10:53:21AM +0200, Kay Sievers wrote: > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote: > > > int device_register(struct device *dev) > > { > > + int retval; > > + > > device_initialize(dev); > > - return device_add(dev); > > + retval = device_add(dev); > > + if (retval) > > + put_device(dev); > > + return retval; > > } > > > Kay, what am I missing here, why can't we just do this? Hm, the > > side-affect might be that if device_register() fails, NO ONE had better > > touch that device again, as it might have just been freed from the > > system. I wonder if that will cause problems... > > That looks right, besides that there might be callers already doing > this. Which needs to be checked. Yes, it would be. I'll go through the tree. > I never liked this pretty useless "convenience API", which just wraps > two simple functions and the first one can never fail anyway. Agreed. > We better remove that device_register() stuff entirely in the long > run, it's not doing any good. At the kobject level we killed the same > stuff already long ago. It's one of the things on my "to change" list. thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 15:50 ` Greg KH @ 2010-09-23 12:10 ` Vasiliy Kulikov 0 siblings, 0 replies; 13+ messages in thread From: Vasiliy Kulikov @ 2010-09-23 12:10 UTC (permalink / raw) To: Greg KH Cc: Kay Sievers, Andrew Morton, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter, Boaz Harrosh On Wed, Sep 22, 2010 at 08:50 -0700, Greg KH wrote: > On Wed, Sep 22, 2010 at 10:53:21AM +0200, Kay Sievers wrote: > > On Wed, Sep 22, 2010 at 00:49, Greg KH <greg@kroah.com> wrote: > > > > > int device_register(struct device *dev) > > > { > > > + int retval; > > > + > > > device_initialize(dev); > > > - return device_add(dev); > > > + retval = device_add(dev); > > > + if (retval) > > > + put_device(dev); > > > + return retval; > > > } > > > > > Kay, what am I missing here, why can't we just do this? Hm, the > > > side-affect might be that if device_register() fails, NO ONE had better > > > touch that device again, as it might have just been freed from the > > > system. I wonder if that will cause problems... > > > > That looks right, besides that there might be callers already doing > > this. Which needs to be checked. > > Yes, it would be. I'll go through the tree. As I see with this in-device_register patch we should check for 2 things: 1) nobody should call put_device() because of failed device_register(). 2) dev has to be already got, other words its ref counter should not be zero. Correct? -- Vasiliy ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-21 22:49 ` Greg KH 2010-09-22 8:53 ` Kay Sievers @ 2010-09-22 9:58 ` Boaz Harrosh 2010-09-22 15:46 ` Greg KH 1 sibling, 1 reply; 13+ messages in thread From: Boaz Harrosh @ 2010-09-22 9:58 UTC (permalink / raw) To: Greg KH Cc: Andrew Morton, Kay Sievers, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter On 09/22/2010 12:49 AM, Greg KH wrote: > On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote: >> On Sun, 19 Sep 2010 16:54:49 +0400 >> Vasiliy Kulikov <segooon@gmail.com> wrote: >> >>> If device_register() fails then call put_device(). >>> See comment to device_register. >>> >>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> >>> --- >>> compile tested. >>> >>> drivers/memstick/core/memstick.c | 1 + >>> 1 files changed, 1 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c >>> index c00fe82..4303b7e 100644 >>> --- a/drivers/memstick/core/memstick.c >>> +++ b/drivers/memstick/core/memstick.c >>> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work) >>> if (!host->card) { >>> host->card = card; >>> if (device_register(&card->dev)) { >>> + put_device(&card->dev); >>> kfree(host->card); >>> host->card = NULL; >>> } >> >> A failed device_register() takes a bogus ref on the not-registered >> device? It's no surprise that people are getting this wrong. >> >> The principle of least surprise says: fix device_register()! > > One might think that, but it's a bit more difficult. > > How does device_register know it should destroy the device if it fails? > > Here's how it works: > - device_register is just a wrapper around device_initialize() and > device_add() > - device_initialize() can't do anything wrong, so it's safe, BUT, > at this point in time, the reference for the device is > incremented, so any caller must now drop the reference and > properly free stuff. > - device_add() does a lot. > > Hm, I guess, because we "know" in device_register() that we must drop > something if device_add() fails, then I guess it's not being consistant > with it's own calls... > > So, something as simple as this? > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index d1b2c9a..4ba8599 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -1084,14 +1084,16 @@ name_error: > * have a clearly defined need to use and refcount the device > * before it is added to the hierarchy. > * > - * NOTE: _Never_ directly free @dev after calling this function, even > - * if it returned an error! Always use put_device() to give up the > - * reference initialized in this function instead. > */ > int device_register(struct device *dev) > { > + int retval; > + > device_initialize(dev); > - return device_add(dev); > + retval = device_add(dev); > + if (retval) > + put_device(dev); This is all theoretically sound. But it will crash my device driver and a dozen more like mine. Because the passed *dev is an embedded structure inside a bigger driver-specific structure. (Hence the call to device_register). The kref inside the *dev is used to also govern the lifetime of the outer struct. With a registered release and the use of a container_of. Now up until now, and was fine before the set_name changes. If device_register failed the build-up function would just deallocate the outer struct together with the inner device. If you call put_device(dev) here then the release function will be called. Which is a double free. And also the release function is coded to expect a fully setup device, though calling device_unregister, and all the other resources contained in the outer struct. So in short it would work but not with current code, and not with out a major restructuring. What about the patch James sent to just free the name string and be done with it? > + return retval; > } > > /** > > > > Kay, what am I missing here, why can't we just do this? Hm, the > side-affect might be that if device_register() fails, NO ONE had better > touch that device again, as it might have just been freed from the > system. I wonder if that will cause problems... > Exactly. Lets please let us recap the scope here. device_register is specifically for Embedded devices where the life time rules are "delicate" to say the least. All we have is the name string memory leak. Can we fix that and not force a major Kernel re-write. > thanks, > > greg k-h Thanks Boaz ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 04/14] memstick: core: fix device_register() error handling 2010-09-22 9:58 ` Boaz Harrosh @ 2010-09-22 15:46 ` Greg KH 0 siblings, 0 replies; 13+ messages in thread From: Greg KH @ 2010-09-22 15:46 UTC (permalink / raw) To: Boaz Harrosh Cc: Andrew Morton, Kay Sievers, Vasiliy Kulikov, kernel-janitors, Tejun Heo, Jiri Slaby, linux-kernel, James Bottomley, Dan Carpenter On Wed, Sep 22, 2010 at 11:58:48AM +0200, Boaz Harrosh wrote: > On 09/22/2010 12:49 AM, Greg KH wrote: > > On Tue, Sep 21, 2010 at 03:20:31PM -0700, Andrew Morton wrote: > >> On Sun, 19 Sep 2010 16:54:49 +0400 > >> Vasiliy Kulikov <segooon@gmail.com> wrote: > >> > >>> If device_register() fails then call put_device(). > >>> See comment to device_register. > >>> > >>> Signed-off-by: Vasiliy Kulikov <segooon@gmail.com> > >>> --- > >>> compile tested. > >>> > >>> drivers/memstick/core/memstick.c | 1 + > >>> 1 files changed, 1 insertions(+), 0 deletions(-) > >>> > >>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c > >>> index c00fe82..4303b7e 100644 > >>> --- a/drivers/memstick/core/memstick.c > >>> +++ b/drivers/memstick/core/memstick.c > >>> @@ -465,6 +465,7 @@ static void memstick_check(struct work_struct *work) > >>> if (!host->card) { > >>> host->card = card; > >>> if (device_register(&card->dev)) { > >>> + put_device(&card->dev); > >>> kfree(host->card); > >>> host->card = NULL; > >>> } > >> > >> A failed device_register() takes a bogus ref on the not-registered > >> device? It's no surprise that people are getting this wrong. > >> > >> The principle of least surprise says: fix device_register()! > > > > One might think that, but it's a bit more difficult. > > > > How does device_register know it should destroy the device if it fails? > > > > Here's how it works: > > - device_register is just a wrapper around device_initialize() and > > device_add() > > - device_initialize() can't do anything wrong, so it's safe, BUT, > > at this point in time, the reference for the device is > > incremented, so any caller must now drop the reference and > > properly free stuff. > > - device_add() does a lot. > > > > Hm, I guess, because we "know" in device_register() that we must drop > > something if device_add() fails, then I guess it's not being consistant > > with it's own calls... > > > > So, something as simple as this? > > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index d1b2c9a..4ba8599 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -1084,14 +1084,16 @@ name_error: > > * have a clearly defined need to use and refcount the device > > * before it is added to the hierarchy. > > * > > - * NOTE: _Never_ directly free @dev after calling this function, even > > - * if it returned an error! Always use put_device() to give up the > > - * reference initialized in this function instead. > > */ > > int device_register(struct device *dev) > > { > > + int retval; > > + > > device_initialize(dev); > > - return device_add(dev); > > + retval = device_add(dev); > > + if (retval) > > + put_device(dev); > > This is all theoretically sound. But it will crash my device driver > and a dozen more like mine. Then they are broken :) > Because the passed *dev is an embedded structure inside a bigger > driver-specific structure. (Hence the call to device_register). So, you would like to keep the way the code currently works in that if device_register() fails, you are responsible for calling put_device() on your own when finished? > The kref inside the *dev is used to also govern the lifetime of > the outer struct. With a registered release and the use of a > container_of. That's as it should be. > Now up until now, and was fine before the set_name changes. > If device_register failed the build-up function would just > deallocate the outer struct together with the inner device. > If you call put_device(dev) here then the release function > will be called. Which is a double free. And also the release > function is coded to expect a fully setup device, though > calling device_unregister, and all the other resources contained > in the outer struct. Well, you don't do the double free then :) > So in short it would work but not with current code, and not > with out a major restructuring. Do you have an example of a driver that would break so I can look at it? > What about the patch James sent to just free the name string > and be done with it? Don't poke into the kobject structure from the driver core, that's not nice and not allowed. > > + return retval; > > } > > > > /** > > > > > > > > Kay, what am I missing here, why can't we just do this? Hm, the > > side-affect might be that if device_register() fails, NO ONE had better > > touch that device again, as it might have just been freed from the > > system. I wonder if that will cause problems... > > > > Exactly. Lets please let us recap the scope here. device_register > is specifically for Embedded devices where the life time rules are > "delicate" to say the least. All we have is the name string memory > leak. Can we fix that and not force a major Kernel re-write. device_register was never written for embedded people at all, so don't think that. And why are your lifetime rules any more "delicate" than anyone elses? Is it because the code is perhaps broken? :) thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-09-23 12:10 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-19 12:54 [PATCH 04/14] memstick: core: fix device_register() error handling Vasiliy Kulikov 2010-09-21 22:20 ` Andrew Morton 2010-09-21 22:49 ` Greg KH 2010-09-22 8:53 ` Kay Sievers 2010-09-22 10:02 ` Boaz Harrosh 2010-09-22 15:47 ` Greg KH 2010-09-22 15:56 ` James Bottomley 2010-09-22 16:20 ` Greg KH 2010-09-22 16:23 ` James Bottomley 2010-09-22 15:50 ` Greg KH 2010-09-23 12:10 ` Vasiliy Kulikov 2010-09-22 9:58 ` Boaz Harrosh 2010-09-22 15:46 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox