Re: [PATCH] kobject_set_name_vargs memory leak

From: Dave Young
Date: Mon Jun 29 2009 - 05:53:58 EST

Next message: Avi Kivity: "Re: [PATCHv2] kvm: remove in_range and switch to rwsem for iobus"
Previous message: Johannes Berg: "Re: [2.6.31-rc1] iwlagn (4965): regression when hardware rf switchis used"
In reply to: Sergey Senozhatsky: "Re: [PATCH] kobject_set_name_vargs memory leak"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Jun 28, 2009 at 8:07 PM, Eric W. Biederman<ebiederm@xxxxxxxxxxxx> wrote:
> Kay Sievers <kay.sievers@xxxxxxxx> writes:
>
>> On Sat, 2009-06-27 at 12:39 +0300, Sergey Senozhatsky wrote:
>>
>>> > >> Or something along those lines, I can't remember the exact reasoning
>>> > >> this early in the morning.
>>> > >>
>>> > >> Kay, do you remember?
>>
>> Hmm, yes, I think there was only something to work around during the
>> transition from the static name array, which is gone now. At least I
>> can't see anything we need to care about with the current code.
>>
>>> Sorry, correct one.
>>>
>>> diff --git a/lib/kobject.c b/lib/kobject.c
>>> index b512b74..3ab224b 100644
>>> --- a/lib/kobject.c
>>> +++ b/lib/kobject.c
>>> @@ -215,7 +215,6 @@ static int kobject_add_internal(struct kobject *kobj)
>>> Âint kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âva_list vargs)
>>> Â{
>>> - Â Âconst char *old_name = kobj->name;
>>
>> I guess, that would leak an allocated name, when it is set several times
>> in a row? Something like this?
>
> But setting a kobject's name several times in a row is a bug. ÂYou
> need to call kobject_rename if you are going to change the name.

Agree

>
> So how about we fix the driver core not to do that. ÂStop treating fmt
> as a flag, and make it clear kobject_add should not be passed a name.
>
> I really hate DWIM functions there is no maintaining them.

How about set name in kobject_init? or another kobject_init_with_name?

>
> Something like this patch:
>
> Âblock/blk-sysfs.c Â Â Â Â Â| Â Â6 ++++-
> Âblock/elevator.c Â Â Â Â Â | Â Â4 ++-
> Âdrivers/base/core.c Â Â Â Â| Â 10 ++++++---
> Âdrivers/firmware/memmap.c Â| Â Â3 +-
> Âdrivers/md/md.c Â Â Â Â Â Â| Â Â5 +++-
> Âdrivers/net/iseries_veth.c | Â Â4 +--
> Âdrivers/uio/uio.c Â Â Â Â Â| Â Â9 +++-----
> Âinclude/linux/kobject.h Â Â| Â Â3 --
> Âlib/kobject.c Â Â Â Â Â Â Â| Â 46 ++++++++++++++-------------------------------
> Â9 files changed, 43 insertions(+), 47 deletions(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index b512b74..92da396 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -215,11 +215,9 @@ static int kobject_add_internal(struct kobject *kobj)
> Âint kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âva_list vargs)
> Â{
> - Â Â Â const char *old_name = kobj->name;
> Â Â Â Âchar *s;
>
> - Â Â Â if (kobj->name && !fmt)
> - Â Â Â Â Â Â Â return 0;
> + Â Â Â BUG_ON(kobj->name);
>
> Â Â Â Âkobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> Â Â Â Âif (!kobj->name)
> @@ -229,7 +227,6 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> Â Â Â Âwhile ((s = strchr(kobj->name, '/')))
> Â Â Â Â Â Â Â Âs[0] = '!';
>
> - Â Â Â kfree(old_name);
> Â Â Â Âreturn 0;
> Â}
>
> @@ -296,20 +293,6 @@ error:
> Â}
> ÂEXPORT_SYMBOL(kobject_init);
>
> -static int kobject_add_varg(struct kobject *kobj, struct kobject *parent,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â const char *fmt, va_list vargs)
> -{
> - Â Â Â int retval;
> -
> - Â Â Â retval = kobject_set_name_vargs(kobj, fmt, vargs);
> - Â Â Â if (retval) {
> - Â Â Â Â Â Â Â printk(KERN_ERR "kobject: can not set name properly!\n");
> - Â Â Â Â Â Â Â return retval;
> - Â Â Â }
> - Â Â Â kobj->parent = parent;
> - Â Â Â return kobject_add_internal(kobj);
> -}
> -
> Â/**
> Â* kobject_add - the main kobject add function
> Â* @kobj: the kobject to add
> @@ -335,15 +318,14 @@ static int kobject_add_varg(struct kobject *kobj, struct kobject *parent,
> Â* kobject_uevent() with the UEVENT_ADD parameter to ensure that
> Â* userspace is properly notified of this kobject's creation.
> Â*/
> -int kobject_add(struct kobject *kobj, struct kobject *parent,
> - Â Â Â Â Â Â Â const char *fmt, ...)
> +int kobject_add(struct kobject *kobj, struct kobject *parent)
> Â{
> - Â Â Â va_list args;
> - Â Â Â int retval;
> -
> Â Â Â Âif (!kobj)
> Â Â Â Â Â Â Â Âreturn -EINVAL;
>
> + Â Â Â if (!kobj->name)
> + Â Â Â Â Â Â Â return -EINVAL;
> +
> Â Â Â Âif (!kobj->state_initialized) {
> Â Â Â Â Â Â Â Âprintk(KERN_ERR "kobject '%s' (%p): tried to add an "
> Â Â Â Â Â Â Â Â Â Â Â "uninitialized object, something is seriously wrong.\n",
> @@ -351,11 +333,8 @@ int kobject_add(struct kobject *kobj, struct kobject *parent,
> Â Â Â Â Â Â Â Âdump_stack();
> Â Â Â Â Â Â Â Âreturn -EINVAL;
> Â Â Â Â}
> - Â Â Â va_start(args, fmt);
> - Â Â Â retval = kobject_add_varg(kobj, parent, fmt, args);
> - Â Â Â va_end(args);
> -
> - Â Â Â return retval;
> + Â Â Â kobj->parent = parent;
> + Â Â Â return kobject_add_internal(kobj);
> Â}
> ÂEXPORT_SYMBOL(kobject_add);
>
> @@ -379,9 +358,12 @@ int kobject_init_and_add(struct kobject *kobj, struct kobj_type *ktype,
> Â Â Â Âkobject_init(kobj, ktype);
>
> Â Â Â Âva_start(args, fmt);
> - Â Â Â retval = kobject_add_varg(kobj, parent, fmt, args);
> + Â Â Â retval = kobject_set_name(kobj, fmt, args);
> Â Â Â Âva_end(args);
>
> + Â Â Â if (!retval)
> + Â Â Â Â Â Â Â retval = kobject_add(kobj, parent);
> +
> Â Â Â Âreturn retval;
> Â}
> ÂEXPORT_SYMBOL_GPL(kobject_init_and_add);
> @@ -653,7 +635,9 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
> Â Â Â Âif (!kobj)
> Â Â Â Â Â Â Â Âreturn NULL;
>
> - Â Â Â retval = kobject_add(kobj, parent, "%s", name);
> + Â Â Â retval = kobject_set_name(kobj, "%s", name);
> + Â Â Â if (!retval)
> + Â Â Â Â Â Â Â retval = kobject_add(kobj, parent);
> Â Â Â Âif (retval) {
> Â Â Â Â Â Â Â Âprintk(KERN_WARNING "%s: kobject_add error: %d\n",
> Â Â Â Â Â Â Â Â Â Â Â __func__, retval);
> @@ -798,7 +782,7 @@ static struct kset *kset_create(const char *name,
> Â Â Â Âkset = kzalloc(sizeof(*kset), GFP_KERNEL);
> Â Â Â Âif (!kset)
> Â Â Â Â Â Â Â Âreturn NULL;
> - Â Â Â retval = kobject_set_name(&kset->kobj, name);
> + Â Â Â retval = kobject_set_name(&kset->kobj, "%s", name);
> Â Â Â Âif (retval) {
> Â Â Â Â Â Â Â Âkfree(kset);
> Â Â Â Â Â Â Â Âreturn NULL;
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index 58ae8e0..de5f5d1 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -83,8 +83,7 @@ static inline const char *kobject_name(const struct kobject *kobj)
>
> Âextern void kobject_init(struct kobject *kobj, struct kobj_type *ktype);
> Âextern int __must_check kobject_add(struct kobject *kobj,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct kobject *parent,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â const char *fmt, ...);
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct kobject *parent);
> Âextern int __must_check kobject_init_and_add(struct kobject *kobj,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct kobj_type *ktype,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct kobject *parent,
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index b1cd040..64f7270 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -433,7 +433,11 @@ int blk_register_queue(struct gendisk *disk)
> Â Â Â Âif (ret)
> Â Â Â Â Â Â Â Âreturn ret;
>
> - Â Â Â ret = kobject_add(&q->kobj, kobject_get(&dev->kobj), "%s", "queue");
> + Â Â Â ret = kobj_set_name(&q->kobj, "%s", "queue");
> + Â Â Â if (ret < 0)
> + Â Â Â Â Â Â Â return ret;
> +
> + Â Â Â ret = kobject_add(&q->kobj, kobject_get(&dev->kobj));
> Â Â Â Âif (ret < 0)
> Â Â Â Â Â Â Â Âreturn ret;
>
> diff --git a/block/elevator.c b/block/elevator.c
> index ca86192..f26dca0 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -903,7 +903,9 @@ int elv_register_queue(struct request_queue *q)
> Â Â Â Âstruct elevator_queue *e = q->elevator;
> Â Â Â Âint error;
>
> - Â Â Â error = kobject_add(&e->kobj, &q->kobj, "%s", "iosched");
> + Â Â Â error = kobject_set_name(&e->kobj, "%s", "iosched");
> + Â Â Â if (!error)
> + Â Â Â Â Â Â Â error = kobject_add(&e->kobj, &q->kobj);
> Â Â Â Âif (!error) {
> Â Â Â Â Â Â Â Âstruct elv_fs_entry *attr = e->elevator_type->elevator_attrs;
> Â Â Â Â Â Â Â Âif (attr) {
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 7ecb193..f0cfa8a 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -625,7 +625,12 @@ static struct kobject *get_device_parent(struct device *dev,
> Â Â Â Â Â Â Â Âif (!k)
> Â Â Â Â Â Â Â Â Â Â Â Âreturn NULL;
> Â Â Â Â Â Â Â Âk->kset = &dev->class->p->class_dirs;
> - Â Â Â Â Â Â Â retval = kobject_add(k, parent_kobj, "%s", dev->class->name);
> + Â Â Â Â Â Â Â retval = kobject_set_name(k, "%s", dev->class->name);
> + Â Â Â Â Â Â Â if (retval < 0) {
> + Â Â Â Â Â Â Â Â Â Â Â kobject_put(k);
> + Â Â Â Â Â Â Â Â Â Â Â return NULL;
> + Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â retval = kobject_add(k, parent_kobj);
> Â Â Â Â Â Â Â Âif (retval < 0) {
> Â Â Â Â Â Â Â Â Â Â Â Âkobject_put(k);
> Â Â Â Â Â Â Â Â Â Â Â Âreturn NULL;
> @@ -900,8 +905,7 @@ int device_add(struct device *dev)
> Â Â Â Â Â Â Â Âset_dev_node(dev, dev_to_node(parent));
>
> Â Â Â Â/* first, register with generic layer. */
> - Â Â Â /* we require the name to be set before, and pass NULL */
> - Â Â Â error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
> + Â Â Â error = kobject_add(&dev->kobj, dev->kobj.parent);
> Â Â Â Âif (error)
> Â Â Â Â Â Â Â Âgoto Error;
>
> diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
> index d5ea8a6..161a165 100644
> --- a/drivers/firmware/memmap.c
> +++ b/drivers/firmware/memmap.c
> @@ -224,7 +224,8 @@ static int __init memmap_init(void)
>
> Â Â Â Âlist_for_each_entry(entry, &map_entries, list) {
> Â Â Â Â Â Â Â Âentry->kobj.kset = memmap_kset;
> - Â Â Â Â Â Â Â if (kobject_add(&entry->kobj, NULL, "%d", i++))
> + Â Â Â Â Â Â Â if (kobject_set_name(&entry->kobj, "%d", i++) ||
> + Â Â Â Â Â Â Â Â Â kobject_add(&entry->kobj, NULL))
> Â Â Â Â Â Â Â Â Â Â Â Âkobject_put(&entry->kobj);
> Â Â Â Â}
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 09be637..2f9fa72 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1575,7 +1575,10 @@ static int bind_rdev_to_array(mdk_rdev_t * rdev, mddev_t * mddev)
> Â Â Â Ârdev->mddev = mddev;
> Â Â Â Âprintk(KERN_INFO "md: bind<%s>\n", b);
>
> - Â Â Â if ((err = kobject_add(&rdev->kobj, &mddev->kobj, "dev-%s", b)))
> + Â Â Â if ((err = kobject_set_name(&rdev->kobj, "dev-%s", b)))
> + Â Â Â Â Â Â Â goto fail;
> +
> + Â Â Â if ((err = kobject_add(&rdev->kobj, &mddev->kobj)))
> Â Â Â Â Â Â Â Âgoto fail;
>
> Â Â Â Âko = &part_to_dev(rdev->bdev->bd_part)->kobj;
> diff --git a/drivers/net/iseries_veth.c b/drivers/net/iseries_veth.c
> index e44215c..a024c24 100644
> --- a/drivers/net/iseries_veth.c
> +++ b/drivers/net/iseries_veth.c
> @@ -1089,8 +1089,8 @@ static struct net_device *veth_probe_one(int vlan,
> Â Â Â Â Â Â Â Âreturn NULL;
> Â Â Â Â}
>
> - Â Â Â kobject_init(&port->kobject, &veth_port_ktype);
> - Â Â Â if (0 != kobject_add(&port->kobject, &dev->dev.kobj, "veth_port"))
> + Â Â Â if (kobject_init_and_add(&port->kobject, &veth_port_ktype,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&dev->dev.kobj, "%s", "veth_port")
> Â Â Â Â Â Â Â Âveth_error("Failed adding port for %s to sysfs.\n", dev->name);
>
> Â Â Â Âveth_info("%s attached to iSeries vlan %d (LPAR map = 0x%.4X)\n",
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 03efb06..7c53bb9 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -303,10 +303,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
> Â Â Â Â Â Â Â Âmap = kzalloc(sizeof(*map), GFP_KERNEL);
> Â Â Â Â Â Â Â Âif (!map)
> Â Â Â Â Â Â Â Â Â Â Â Âgoto err_map;
> - Â Â Â Â Â Â Â kobject_init(&map->kobj, &map_attr_type);
> Â Â Â Â Â Â Â Âmap->mem = mem;
> Â Â Â Â Â Â Â Âmem->map = map;
> - Â Â Â Â Â Â Â ret = kobject_add(&map->kobj, idev->map_dir, "map%d", mi);
> + Â Â Â Â Â Â Â ret = kobject_init_and_add(&map->kobj, &map_attr_type,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âidev->map_dir, "map%d", mi);
> Â Â Â Â Â Â Â Âif (ret)
> Â Â Â Â Â Â Â Â Â Â Â Âgoto err_map;
> Â Â Â Â Â Â Â Âret = kobject_uevent(&map->kobj, KOBJ_ADD);
> @@ -328,11 +328,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
> Â Â Â Â Â Â Â Âportio = kzalloc(sizeof(*portio), GFP_KERNEL);
> Â Â Â Â Â Â Â Âif (!portio)
> Â Â Â Â Â Â Â Â Â Â Â Âgoto err_portio;
> - Â Â Â Â Â Â Â kobject_init(&portio->kobj, &portio_attr_type);
> Â Â Â Â Â Â Â Âportio->port = port;
> Â Â Â Â Â Â Â Âport->portio = portio;
> - Â Â Â Â Â Â Â ret = kobject_add(&portio->kobj, idev->portio_dir,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "port%d", pi);
> + Â Â Â Â Â Â Â ret = kobject_init_and_add(&portio->kobj, &portio_attr_type,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âidev->portio_dir, "port%d", pi);
> Â Â Â Â Â Â Â Âif (ret)
> Â Â Â Â Â Â Â Â Â Â Â Âgoto err_portio;
> Â Â Â Â Â Â Â Âret = kobject_uevent(&portio->kobj, KOBJ_ADD);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
> Please read the FAQ at Âhttp://www.tux.org/lkml/
>

--
Regards
dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Avi Kivity: "Re: [PATCHv2] kvm: remove in_range and switch to rwsem for iobus"
Previous message: Johannes Berg: "Re: [2.6.31-rc1] iwlagn (4965): regression when hardware rf switchis used"
In reply to: Sergey Senozhatsky: "Re: [PATCH] kobject_set_name_vargs memory leak"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]