* [PATCH] drivers: base: core: do not put noninitialized devices @ 2010-11-19 18:41 Vasiliy Kulikov 2010-11-19 19:02 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Vasiliy Kulikov @ 2010-11-19 18:41 UTC (permalink / raw) To: kernel-janitors; +Cc: Greg Kroah-Hartman, linux-kernel If kobject_set_name_vargs() fails then put_device() frees device with zero kobj->state_initialized. This leads to WARN(). Divide device_register() call to device_initialize() call before kobject_set_name_vargs() and device_add() call after it. Signed-off-by: Vasiliy Kulikov <segoon@openwall.com> --- Compile tested only. drivers/base/core.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/base/core.c b/drivers/base/core.c index 46ff6c2..833ccf3 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1424,11 +1424,12 @@ struct device *device_create_vargs(struct class *class, struct device *parent, dev->release = device_create_release; dev_set_drvdata(dev, drvdata); + device_initialize(dev); retval = kobject_set_name_vargs(&dev->kobj, fmt, args); if (retval) goto error; - retval = device_register(dev); + retval = device_add(dev); if (retval) goto error; -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: base: core: do not put noninitialized devices 2010-11-19 18:41 [PATCH] drivers: base: core: do not put noninitialized devices Vasiliy Kulikov @ 2010-11-19 19:02 ` Greg KH 2010-11-19 19:14 ` Vasiliy Kulikov 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2010-11-19 19:02 UTC (permalink / raw) To: Vasiliy Kulikov; +Cc: kernel-janitors, linux-kernel On Fri, Nov 19, 2010 at 09:41:40PM +0300, Vasiliy Kulikov wrote: > If kobject_set_name_vargs() fails then put_device() frees > device with zero kobj->state_initialized. This leads to WARN(). Have you seen this happen? > Divide device_register() call to device_initialize() call before > kobject_set_name_vargs() and device_add() call after it. > > Signed-off-by: Vasiliy Kulikov <segoon@openwall.com> > --- > Compile tested only. I'd prefer not to change this unless you are seeing problems with the current code. How did kobject_set_name_vargs() fail for you? thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: base: core: do not put noninitialized devices 2010-11-19 19:02 ` Greg KH @ 2010-11-19 19:14 ` Vasiliy Kulikov 2010-11-19 19:17 ` Vasiliy Kulikov 2010-11-19 20:57 ` Greg KH 0 siblings, 2 replies; 6+ messages in thread From: Vasiliy Kulikov @ 2010-11-19 19:14 UTC (permalink / raw) To: Greg KH; +Cc: kernel-janitors, linux-kernel Hi Greg, On Fri, Nov 19, 2010 at 11:02 -0800, Greg KH wrote: > On Fri, Nov 19, 2010 at 09:41:40PM +0300, Vasiliy Kulikov wrote: > > If kobject_set_name_vargs() fails then put_device() frees > > device with zero kobj->state_initialized. This leads to WARN(). > > Have you seen this happen? No, I've just analized the code. Without device_initialize() ->kobj is not initialized: kobject_init(&dev->kobj, &device_ktype) calls kobject_init_internal(kobj) calls kobj->state_initialized = 1; kobject_put() calls WARN if state_initialized == 0: void kobject_put(struct kobject *kobj) { if (kobj) { if (!kobj->state_initialized) WARN(1, KERN_WARNING "kobject: '%s' (%p): is not " "initialized, yet kobject_put() is being " "called.\n", kobject_name(kobj), kobj); I got the stack dump with similar code: struct device *dev = kzalloc(sizeof(*dev), GFP_KERNEL); put_device(dev); > I'd prefer not to change this unless you are seeing problems with the > current code. > > How did kobject_set_name_vargs() fail for you? int kobject_set_name_vargs(struct kobject *kobj, const char *fmt, va_list vargs) { [...] kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); if (!kobj->name) return -ENOMEM; char *kvasprintf(gfp_t gfp, const char *fmt, va_list ap) { [...] p = kmalloc(len+1, gfp); if (!p) return NULL; Unlikely, but may fail in OOM situation. Thanks, -- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: base: core: do not put noninitialized devices 2010-11-19 19:14 ` Vasiliy Kulikov @ 2010-11-19 19:17 ` Vasiliy Kulikov 2010-11-19 20:57 ` Greg KH 1 sibling, 0 replies; 6+ messages in thread From: Vasiliy Kulikov @ 2010-11-19 19:17 UTC (permalink / raw) To: Greg KH; +Cc: kernel-janitors, linux-kernel On Fri, Nov 19, 2010 at 22:14 +0300, Vasiliy Kulikov wrote: > No, I've just analized the code. Without device_initialize() ->kobj is > not initialized: I mean 'not set' instead of 'not initialized' - it is already kzalloc()'ed :) Thanks, -- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: base: core: do not put noninitialized devices 2010-11-19 19:14 ` Vasiliy Kulikov 2010-11-19 19:17 ` Vasiliy Kulikov @ 2010-11-19 20:57 ` Greg KH 2010-11-20 9:00 ` Vasiliy Kulikov 1 sibling, 1 reply; 6+ messages in thread From: Greg KH @ 2010-11-19 20:57 UTC (permalink / raw) To: Vasiliy Kulikov; +Cc: kernel-janitors, linux-kernel On Fri, Nov 19, 2010 at 10:14:25PM +0300, Vasiliy Kulikov wrote: > Hi Greg, > > On Fri, Nov 19, 2010 at 11:02 -0800, Greg KH wrote: > > On Fri, Nov 19, 2010 at 09:41:40PM +0300, Vasiliy Kulikov wrote: > > > If kobject_set_name_vargs() fails then put_device() frees > > > device with zero kobj->state_initialized. This leads to WARN(). > > > > Have you seen this happen? > > No, I've just analized the code. Without device_initialize() ->kobj is > not initialized: > > kobject_init(&dev->kobj, &device_ktype) calls > > kobject_init_internal(kobj) calls > > kobj->state_initialized = 1; > > kobject_put() calls WARN if state_initialized == 0: > > void kobject_put(struct kobject *kobj) > { > if (kobj) { > if (!kobj->state_initialized) > WARN(1, KERN_WARNING "kobject: '%s' (%p): is not " > "initialized, yet kobject_put() is being " > "called.\n", kobject_name(kobj), kobj); > > > I got the stack dump with similar code: > > struct device *dev = kzalloc(sizeof(*dev), GFP_KERNEL); > put_device(dev); Sure, that's illegal code, and we want to warn about that. So I would say, if an error happens, we want to see this message so the code should stay as-is. thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: base: core: do not put noninitialized devices 2010-11-19 20:57 ` Greg KH @ 2010-11-20 9:00 ` Vasiliy Kulikov 0 siblings, 0 replies; 6+ messages in thread From: Vasiliy Kulikov @ 2010-11-20 9:00 UTC (permalink / raw) To: Greg KH; +Cc: kernel-janitors, linux-kernel On Fri, Nov 19, 2010 at 12:57 -0800, Greg KH wrote: > On Fri, Nov 19, 2010 at 10:14:25PM +0300, Vasiliy Kulikov wrote: > > kobject_put() calls WARN if state_initialized == 0: > > > > void kobject_put(struct kobject *kobj) > > { > > if (kobj) { > > if (!kobj->state_initialized) > > WARN(1, KERN_WARNING "kobject: '%s' (%p): is not " > > "initialized, yet kobject_put() is being " > > "called.\n", kobject_name(kobj), kobj); > > > > > > I got the stack dump with similar code: > > > > struct device *dev = kzalloc(sizeof(*dev), GFP_KERNEL); > > put_device(dev); > > Sure, that's illegal code, You might misunderstood me, see this part of device_create_vargs(): struct device *device_create_vargs(...) { ... dev = kzalloc(sizeof(*dev), GFP_KERNEL); <<< if (!dev) ... dev->devt = devt; dev->class = class; dev->parent = parent; dev->release = device_create_release; dev_set_drvdata(dev, drvdata); retval = kobject_set_name_vargs(&dev->kobj, fmt, args); <<< if (retval) goto error; <<< ... error: put_device(dev); <<< ... } It is device_create_vargs()'s mistake, not the caller. > and we want to warn about that. So I would > say, if an error happens, we want to see this message so the code should > stay as-is. If you mean "if memomy allocation for name fails then we want to see message about allocation failure" then maybe use pr_err("No mem for device name\n") instead of confusing "kobject is not initialized, yet kobject_put() is being called"? Thanks, -- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-20 9:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-11-19 18:41 [PATCH] drivers: base: core: do not put noninitialized devices Vasiliy Kulikov 2010-11-19 19:02 ` Greg KH 2010-11-19 19:14 ` Vasiliy Kulikov 2010-11-19 19:17 ` Vasiliy Kulikov 2010-11-19 20:57 ` Greg KH 2010-11-20 9:00 ` Vasiliy Kulikov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox