* [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
@ 2007-04-01 7:32 Cong WANG
2007-04-02 11:01 ` Cornelia Huck
0 siblings, 1 reply; 18+ messages in thread
From: Cong WANG @ 2007-04-01 7:32 UTC (permalink / raw)
To: linux-kernel, Andrew Morton, viro
Since kobject_add, sysfs_create_link and sysfs_create_file are marked
as '__must_check', we must always check their return values.
Signed-off-by: Cong WANG <xiyou.wangcong@gmail.com>
---
--- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30
21:35:45.000000000 +0800
+++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-03-30
21:49:53.000000000 +0800
@@ -385,10 +385,16 @@ void add_partition(struct gendisk *disk,
p->kobj.parent = &disk->kobj;
p->kobj.ktype = &ktype_part;
kobject_init(&p->kobj);
- kobject_add(&p->kobj);
+ if (kobject_add(&p->kobj)) {
+ kfree(p);
+ return;
+ }
if (!disk->part_uevent_suppress)
kobject_uevent(&p->kobj, KOBJ_ADD);
- sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
+ kfree(p);
+ return;
+ }
if (flags & ADDPART_FLAG_WHOLEDISK) {
static struct attribute addpartattr = {
.name = "whole_disk",
@@ -396,7 +402,10 @@ void add_partition(struct gendisk *disk,
.owner = THIS_MODULE,
};
- sysfs_create_file(&p->kobj, &addpartattr);
+ if (sysfs_create_file(&p->kobj, &addpartattr)) {
+ kfree(p);
+ return;
+ }
}
partition_sysfs_add_subdir(p);
disk->part[part-1] = p;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-01 7:32 [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc Cong WANG
@ 2007-04-02 11:01 ` Cornelia Huck
2007-04-05 4:54 ` WANG Cong
0 siblings, 1 reply; 18+ messages in thread
From: Cornelia Huck @ 2007-04-02 11:01 UTC (permalink / raw)
To: Cong WANG; +Cc: linux-kernel, Andrew Morton, viro
On Sun, 1 Apr 2007 15:32:34 +0800,
"Cong WANG" <xiyou.wangcong@gmail.com> wrote:
> --- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30
> 21:35:45.000000000 +0800
> +++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-03-30
> 21:49:53.000000000 +0800
> @@ -385,10 +385,16 @@ void add_partition(struct gendisk *disk,
> p->kobj.parent = &disk->kobj;
> p->kobj.ktype = &ktype_part;
> kobject_init(&p->kobj);
> - kobject_add(&p->kobj);
> + if (kobject_add(&p->kobj)) {
> + kfree(p);
> + return;
> + }
> if (!disk->part_uevent_suppress)
> kobject_uevent(&p->kobj, KOBJ_ADD);
> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
> + kfree(p);
> + return;
> + }
You need to properly undo whatever you did before. You're missing a
KOBJ_DEL uevent and a kobject_del here.
> if (flags & ADDPART_FLAG_WHOLEDISK) {
> static struct attribute addpartattr = {
> .name = "whole_disk",
> @@ -396,7 +402,10 @@ void add_partition(struct gendisk *disk,
> .owner = THIS_MODULE,
> };
>
> - sysfs_create_file(&p->kobj, &addpartattr);
> + if (sysfs_create_file(&p->kobj, &addpartattr)) {
> + kfree(p);
> + return;
> + }
Also here.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-02 11:01 ` Cornelia Huck
@ 2007-04-05 4:54 ` WANG Cong
2007-04-05 9:11 ` Cornelia Huck
0 siblings, 1 reply; 18+ messages in thread
From: WANG Cong @ 2007-04-05 4:54 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-kernel, Andrew Morton, viro
On Mon, Apr 02, 2007 at 01:01:28PM +0200, Cornelia Huck wrote:
>On Sun, 1 Apr 2007 15:32:34 +0800,
>"Cong WANG" <xiyou.wangcong@gmail.com> wrote:
>
>> --- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30
>> 21:35:45.000000000 +0800
>> +++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-03-30
>> 21:49:53.000000000 +0800
>> @@ -385,10 +385,16 @@ void add_partition(struct gendisk *disk,
>> p->kobj.parent = &disk->kobj;
>> p->kobj.ktype = &ktype_part;
>> kobject_init(&p->kobj);
>> - kobject_add(&p->kobj);
>> + if (kobject_add(&p->kobj)) {
>> + kfree(p);
>> + return;
>> + }
>> if (!disk->part_uevent_suppress)
>> kobject_uevent(&p->kobj, KOBJ_ADD);
>> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
>> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
>> + kfree(p);
>> + return;
>> + }
>
>You need to properly undo whatever you did before. You're missing a
>KOBJ_DEL uevent and a kobject_del here.
>
>> if (flags & ADDPART_FLAG_WHOLEDISK) {
>> static struct attribute addpartattr = {
>> .name = "whole_disk",
>> @@ -396,7 +402,10 @@ void add_partition(struct gendisk *disk,
>> .owner = THIS_MODULE,
>> };
>>
>> - sysfs_create_file(&p->kobj, &addpartattr);
>> + if (sysfs_create_file(&p->kobj, &addpartattr)) {
>> + kfree(p);
>> + return;
>> + }
>
>Also here.
Sorry for my delay. My mutt sucked these days. ;-(
OK. Thanks for your point. I make the patch again and is it OK now?
I find -mm4 still has these warnings when compile this file. It is not fixed yet, and the following patch can also be applied to -mm4.
----
--- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30 21:35:45.000000000 +0800
+++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-04-02 21:29:02.000000000 +0800
@@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
p->kobj.parent = &disk->kobj;
p->kobj.ktype = &ktype_part;
kobject_init(&p->kobj);
- kobject_add(&p->kobj);
+ if (kobject_add(&p->kobj)) {
+ kfree(p);
+ return;
+ }
if (!disk->part_uevent_suppress)
kobject_uevent(&p->kobj, KOBJ_ADD);
- sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+ kfree(p);
+ return;
+ }
if (flags & ADDPART_FLAG_WHOLEDISK) {
static struct attribute addpartattr = {
.name = "whole_disk",
@@ -396,7 +404,13 @@ void add_partition(struct gendisk *disk,
.owner = THIS_MODULE,
};
- sysfs_create_file(&p->kobj, &addpartattr);
+ if (sysfs_create_file(&p->kobj, &addpartattr)) {
+ sysfs_remove_link(&p->kobj, "subsystem");
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+ kfree(p);
+ return;
+ }
}
partition_sysfs_add_subdir(p);
disk->part[part-1] = p;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-05 4:54 ` WANG Cong
@ 2007-04-05 9:11 ` Cornelia Huck
2007-04-05 14:44 ` WANG Cong
0 siblings, 1 reply; 18+ messages in thread
From: Cornelia Huck @ 2007-04-05 9:11 UTC (permalink / raw)
To: WANG Cong; +Cc: linux-kernel, Andrew Morton, viro
On Thu, 5 Apr 2007 12:54:11 +0800,
WANG Cong <xiyou.wangcong@gmail.com> wrote:
> --- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30 21:35:45.000000000 +0800
> +++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-04-02 21:29:02.000000000 +0800
> @@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
> p->kobj.parent = &disk->kobj;
> p->kobj.ktype = &ktype_part;
> kobject_init(&p->kobj);
> - kobject_add(&p->kobj);
> + if (kobject_add(&p->kobj)) {
> + kfree(p);
> + return;
> + }
> if (!disk->part_uevent_suppress)
> kobject_uevent(&p->kobj, KOBJ_ADD);
> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> + kobject_del(&p->kobj);
> + kfree(p);
You should use kobject_put instead of kfree, since someone could have
(theoretically) obtained a reference on the object after the
kobject_add. (Or just use kobject_unregister, if the delete uevent
isn't suppressed anyway.)
> + return;
> + }
> if (flags & ADDPART_FLAG_WHOLEDISK) {
> static struct attribute addpartattr = {
> .name = "whole_disk",
> @@ -396,7 +404,13 @@ void add_partition(struct gendisk *disk,
> .owner = THIS_MODULE,
> };
>
> - sysfs_create_file(&p->kobj, &addpartattr);
> + if (sysfs_create_file(&p->kobj, &addpartattr)) {
> + sysfs_remove_link(&p->kobj, "subsystem");
> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> + kobject_del(&p->kobj);
> + kfree(p);
Same here.
> + return;
> + }
> }
> partition_sysfs_add_subdir(p);
> disk->part[part-1] = p;
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-05 9:11 ` Cornelia Huck
@ 2007-04-05 14:44 ` WANG Cong
2007-04-05 15:05 ` Cornelia Huck
0 siblings, 1 reply; 18+ messages in thread
From: WANG Cong @ 2007-04-05 14:44 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-kernel, Andrew Morton, viro
On Thu, Apr 05, 2007 at 11:11:42AM +0200, Cornelia Huck wrote:
>On Thu, 5 Apr 2007 12:54:11 +0800,
>WANG Cong <xiyou.wangcong@gmail.com> wrote:
>
>> --- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30 21:35:45.000000000 +0800
>> +++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-04-02 21:29:02.000000000 +0800
>> @@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
>> p->kobj.parent = &disk->kobj;
>> p->kobj.ktype = &ktype_part;
>> kobject_init(&p->kobj);
>> - kobject_add(&p->kobj);
>> + if (kobject_add(&p->kobj)) {
>> + kfree(p);
>> + return;
>> + }
>> if (!disk->part_uevent_suppress)
>> kobject_uevent(&p->kobj, KOBJ_ADD);
>> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
>> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
>> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
>> + kobject_del(&p->kobj);
>> + kfree(p);
>
>You should use kobject_put instead of kfree, since someone could have
>(theoretically) obtained a reference on the object after the
>kobject_add. (Or just use kobject_unregister, if the delete uevent
>isn't suppressed anyway.)
>
I can't understand that. The memory pointed by 'p' is obtained via kmalloc(), _not_ kobject_get. How can we use kobject_put instead?
Thanks!
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-05 14:44 ` WANG Cong
@ 2007-04-05 15:05 ` Cornelia Huck
2007-04-05 15:27 ` WANG Cong
0 siblings, 1 reply; 18+ messages in thread
From: Cornelia Huck @ 2007-04-05 15:05 UTC (permalink / raw)
To: WANG Cong; +Cc: linux-kernel, Andrew Morton, viro
On Thu, 5 Apr 2007 22:44:09 +0800,
WANG Cong <xiyou.wangcong@gmail.com> wrote:
> On Thu, Apr 05, 2007 at 11:11:42AM +0200, Cornelia Huck wrote:
> >On Thu, 5 Apr 2007 12:54:11 +0800,
> >WANG Cong <xiyou.wangcong@gmail.com> wrote:
> >
> >> --- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30 21:35:45.000000000 +0800
> >> +++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-04-02 21:29:02.000000000 +0800
> >> @@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
> >> p->kobj.parent = &disk->kobj;
> >> p->kobj.ktype = &ktype_part;
> >> kobject_init(&p->kobj);
> >> - kobject_add(&p->kobj);
> >> + if (kobject_add(&p->kobj)) {
> >> + kfree(p);
> >> + return;
> >> + }
> >> if (!disk->part_uevent_suppress)
> >> kobject_uevent(&p->kobj, KOBJ_ADD);
> >> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
> >> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
> >> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> >> + kobject_del(&p->kobj);
> >> + kfree(p);
> >
> >You should use kobject_put instead of kfree, since someone could have
> >(theoretically) obtained a reference on the object after the
> >kobject_add. (Or just use kobject_unregister, if the delete uevent
> >isn't suppressed anyway.)
> >
>
> I can't understand that. The memory pointed by 'p' is obtained via kmalloc(), _not_ kobject_get. How can we use kobject_put instead?
The hd_struct *p embeds a kobject kobj (which, amongst other things,
provides reference counting for the hd_struct). The release function
for the kobject will free the embedding object (note that kobject_init
sets the reference count to 1). That means, if kobject_put(&p->kobj)
will drop the reference to 0, the release function (part_release) will
free p. (See also Documentation/kobject.txt.)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-05 15:05 ` Cornelia Huck
@ 2007-04-05 15:27 ` WANG Cong
2007-04-05 16:00 ` Cornelia Huck
0 siblings, 1 reply; 18+ messages in thread
From: WANG Cong @ 2007-04-05 15:27 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-kernel, Andrew Morton, viro
On Thu, Apr 05, 2007 at 05:05:14PM +0200, Cornelia Huck wrote:
>On Thu, 5 Apr 2007 22:44:09 +0800,
>WANG Cong <xiyou.wangcong@gmail.com> wrote:
>
>> On Thu, Apr 05, 2007 at 11:11:42AM +0200, Cornelia Huck wrote:
>> >On Thu, 5 Apr 2007 12:54:11 +0800,
>> >WANG Cong <xiyou.wangcong@gmail.com> wrote:
>> >
>> >> --- linux-2.6.21-rc5-mm3/fs/partitions/check.c.orig 2007-03-30 21:35:45.000000000 +0800
>> >> +++ linux-2.6.21-rc5-mm3/fs/partitions/check.c 2007-04-02 21:29:02.000000000 +0800
>> >> @@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
>> >> p->kobj.parent = &disk->kobj;
>> >> p->kobj.ktype = &ktype_part;
>> >> kobject_init(&p->kobj);
>> >> - kobject_add(&p->kobj);
>> >> + if (kobject_add(&p->kobj)) {
>> >> + kfree(p);
>> >> + return;
>> >> + }
>> >> if (!disk->part_uevent_suppress)
>> >> kobject_uevent(&p->kobj, KOBJ_ADD);
>> >> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
>> >> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
>> >> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
>> >> + kobject_del(&p->kobj);
>> >> + kfree(p);
>> >
>> >You should use kobject_put instead of kfree, since someone could have
>> >(theoretically) obtained a reference on the object after the
>> >kobject_add. (Or just use kobject_unregister, if the delete uevent
>> >isn't suppressed anyway.)
>> >
>>
>> I can't understand that. The memory pointed by 'p' is obtained via kmalloc(), _not_ kobject_get. How can we use kobject_put instead?
>
>The hd_struct *p embeds a kobject kobj (which, amongst other things,
>provides reference counting for the hd_struct). The release function
>for the kobject will free the embedding object (note that kobject_init
>sets the reference count to 1). That means, if kobject_put(&p->kobj)
>will drop the reference to 0, the release function (part_release) will
>free p. (See also Documentation/kobject.txt.)
Thank you very much! I know. So I should replace all kfree with kobject_put, like this one:
- sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+ kobject_put(&p->kobj);
+ return;
+ }
Is that all right?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-05 15:27 ` WANG Cong
@ 2007-04-05 16:00 ` Cornelia Huck
2007-04-06 2:53 ` WANG Cong
0 siblings, 1 reply; 18+ messages in thread
From: Cornelia Huck @ 2007-04-05 16:00 UTC (permalink / raw)
To: WANG Cong; +Cc: linux-kernel, Andrew Morton, viro
On Thu, 5 Apr 2007 23:27:32 +0800,
WANG Cong <xiyou.wangcong@gmail.com> wrote:
> Thank you very much! I know. So I should replace all kfree with kobject_put, like this one:
>
> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> + kobject_del(&p->kobj);
> + kobject_put(&p->kobj);
> + return;
> + }
>
> Is that all right?
>
Yes, or use kobject_unregister().
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-05 16:00 ` Cornelia Huck
@ 2007-04-06 2:53 ` WANG Cong
2007-04-10 12:31 ` Cornelia Huck
0 siblings, 1 reply; 18+ messages in thread
From: WANG Cong @ 2007-04-06 2:53 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-kernel, Andrew Morton, viro
On Thu, Apr 05, 2007 at 06:00:16PM +0200, Cornelia Huck wrote:
>On Thu, 5 Apr 2007 23:27:32 +0800,
>WANG Cong <xiyou.wangcong@gmail.com> wrote:
>
>> Thank you very much! I know. So I should replace all kfree with kobject_put, like this one:
>>
>> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
>> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
>> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
>> + kobject_del(&p->kobj);
>> + kobject_put(&p->kobj);
>> + return;
>> + }
>>
>> Is that all right?
>>
>
>Yes, or use kobject_unregister().
OK. Then I send it again. Hopefully it can be accepted this time. ;-p
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
---
--- linux-2.6.21-rc5-mm4/fs/partitions/check.c.orig 2007-04-05 12:48:29.000000000 +0800
+++ linux-2.6.21-rc5-mm4/fs/partitions/check.c 2007-04-05 23:15:41.000000000 +0800
@@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
p->kobj.parent = &disk->kobj;
p->kobj.ktype = &ktype_part;
kobject_init(&p->kobj);
- kobject_add(&p->kobj);
+ if (kobject_add(&p->kobj)) {
+ kobject_put(&p->kobj);
+ return;
+ }
if (!disk->part_uevent_suppress)
kobject_uevent(&p->kobj, KOBJ_ADD);
- sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+ kobject_put(&p->kobj);
+ return;
+ }
if (flags & ADDPART_FLAG_WHOLEDISK) {
static struct attribute addpartattr = {
.name = "whole_disk",
@@ -396,7 +404,13 @@ void add_partition(struct gendisk *disk,
.owner = THIS_MODULE,
};
- sysfs_create_file(&p->kobj, &addpartattr);
+ if (sysfs_create_file(&p->kobj, &addpartattr)) {
+ sysfs_remove_link(&p->kobj, "subsystem");
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+ kobject_put(&p->kobj);
+ return;
+ }
}
partition_sysfs_add_subdir(p);
disk->part[part-1] = p;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-06 2:53 ` WANG Cong
@ 2007-04-10 12:31 ` Cornelia Huck
2007-04-10 12:35 ` [Patch -mm] kobject: kobject_add() reference leak Cornelia Huck
2007-04-10 14:08 ` [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc WANG Cong
0 siblings, 2 replies; 18+ messages in thread
From: Cornelia Huck @ 2007-04-10 12:31 UTC (permalink / raw)
To: WANG Cong; +Cc: linux-kernel, Andrew Morton, viro
On Fri, 6 Apr 2007 10:53:43 +0800,
WANG Cong <xiyou.wangcong@gmail.com> wrote:
> OK. Then I send it again. Hopefully it can be accepted this time. ;-p
Looks sane. (Note that there is still a pathological case where
kobject_put() is not enough to trigger release after the failed
kobject_add(). I'm sending a fix for that.)
<Maybe you want to add one or two lines of explanation for your patch
again?>
>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>
> --- linux-2.6.21-rc5-mm4/fs/partitions/check.c.orig 2007-04-05 12:48:29.000000000 +0800
> +++ linux-2.6.21-rc5-mm4/fs/partitions/check.c 2007-04-05 23:15:41.000000000 +0800
> @@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
> p->kobj.parent = &disk->kobj;
> p->kobj.ktype = &ktype_part;
> kobject_init(&p->kobj);
> - kobject_add(&p->kobj);
> + if (kobject_add(&p->kobj)) {
> + kobject_put(&p->kobj);
> + return;
> + }
> if (!disk->part_uevent_suppress)
> kobject_uevent(&p->kobj, KOBJ_ADD);
> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> + kobject_del(&p->kobj);
> + kobject_put(&p->kobj);
> + return;
> + }
> if (flags & ADDPART_FLAG_WHOLEDISK) {
> static struct attribute addpartattr = {
> .name = "whole_disk",
> @@ -396,7 +404,13 @@ void add_partition(struct gendisk *disk,
> .owner = THIS_MODULE,
> };
>
> - sysfs_create_file(&p->kobj, &addpartattr);
> + if (sysfs_create_file(&p->kobj, &addpartattr)) {
> + sysfs_remove_link(&p->kobj, "subsystem");
> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> + kobject_del(&p->kobj);
> + kobject_put(&p->kobj);
> + return;
> + }
> }
> partition_sysfs_add_subdir(p);
> disk->part[part-1] = p;
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Patch -mm] kobject: kobject_add() reference leak
2007-04-10 12:31 ` Cornelia Huck
@ 2007-04-10 12:35 ` Cornelia Huck
2007-04-10 14:08 ` [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc WANG Cong
1 sibling, 0 replies; 18+ messages in thread
From: Cornelia Huck @ 2007-04-10 12:35 UTC (permalink / raw)
To: Greg K-H; +Cc: WANG Cong, linux-kernel, Andrew Morton, viro
<This patch is the "pathological case" in the referenced mail.>
We leak a reference if we attempt to add a kobject with no name.
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
lib/kobject.c | 1 +
1 files changed, 1 insertion(+)
--- linux-2.6.21-rc6-mm1.orig/lib/kobject.c
+++ linux-2.6.21-rc6-mm1/lib/kobject.c
@@ -218,6 +218,7 @@ int kobject_shadow_add(struct kobject *
if (!*kobj->k_name) {
pr_debug("kobject attempted to be registered with no name!\n");
WARN_ON(1);
+ kobject_put(kobj);
return -EINVAL;
}
parent = kobject_get(kobj->parent);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-10 12:31 ` Cornelia Huck
2007-04-10 12:35 ` [Patch -mm] kobject: kobject_add() reference leak Cornelia Huck
@ 2007-04-10 14:08 ` WANG Cong
2007-04-10 22:18 ` Andrew Morton
1 sibling, 1 reply; 18+ messages in thread
From: WANG Cong @ 2007-04-10 14:08 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-kernel, Andrew Morton, viro
On Tue, Apr 10, 2007 at 02:31:06PM +0200, Cornelia Huck wrote:
>On Fri, 6 Apr 2007 10:53:43 +0800,
>WANG Cong <xiyou.wangcong@gmail.com> wrote:
>
>> OK. Then I send it again. Hopefully it can be accepted this time. ;-p
>
>Looks sane. (Note that there is still a pathological case where
>kobject_put() is not enough to trigger release after the failed
>kobject_add(). I'm sending a fix for that.)
>
><Maybe you want to add one or two lines of explanation for your patch
>again?>
Thanks! My explanation of the patch is already in the first mail of this thread. Now I copy it here and resubmit the patch. Hope you don't mind. ;-p This patch can also be applied to -rc6 tree.
Since kobject_add, sysfs_create_link and sysfs_create_file are marked as '__must_check', we must always check their return values.
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
--- linux-2.6.21-rc5-mm4/fs/partitions/check.c.orig 2007-04-05 12:48:29.000000000 +0800
+++ linux-2.6.21-rc5-mm4/fs/partitions/check.c 2007-04-05 23:15:41.000000000 +0800
@@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
p->kobj.parent = &disk->kobj;
p->kobj.ktype = &ktype_part;
kobject_init(&p->kobj);
- kobject_add(&p->kobj);
+ if (kobject_add(&p->kobj)) {
+ kobject_put(&p->kobj);
+ return;
+ }
if (!disk->part_uevent_suppress)
kobject_uevent(&p->kobj, KOBJ_ADD);
- sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+ kobject_put(&p->kobj);
+ return;
+ }
if (flags & ADDPART_FLAG_WHOLEDISK) {
static struct attribute addpartattr = {
.name = "whole_disk",
@@ -396,7 +404,13 @@ void add_partition(struct gendisk *disk,
.owner = THIS_MODULE,
};
- sysfs_create_file(&p->kobj, &addpartattr);
+ if (sysfs_create_file(&p->kobj, &addpartattr)) {
+ sysfs_remove_link(&p->kobj, "subsystem");
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+ kobject_put(&p->kobj);
+ return;
+ }
}
partition_sysfs_add_subdir(p);
disk->part[part-1] = p;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-10 14:08 ` [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc WANG Cong
@ 2007-04-10 22:18 ` Andrew Morton
2007-04-11 6:45 ` WANG Cong
2007-04-11 9:24 ` Cornelia Huck
0 siblings, 2 replies; 18+ messages in thread
From: Andrew Morton @ 2007-04-10 22:18 UTC (permalink / raw)
To: WANG Cong; +Cc: Cornelia Huck, linux-kernel, viro
On Tue, 10 Apr 2007 22:08:29 +0800
WANG Cong <xiyou.wangcong@gmail.com> wrote:
> Since kobject_add, sysfs_create_link and sysfs_create_file are marked as '__must_check', we must always check their return values.
>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
> Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>
> ---
> --- linux-2.6.21-rc5-mm4/fs/partitions/check.c.orig 2007-04-05 12:48:29.000000000 +0800
> +++ linux-2.6.21-rc5-mm4/fs/partitions/check.c 2007-04-05 23:15:41.000000000 +0800
> @@ -385,10 +385,18 @@ void add_partition(struct gendisk *disk,
> p->kobj.parent = &disk->kobj;
> p->kobj.ktype = &ktype_part;
> kobject_init(&p->kobj);
> - kobject_add(&p->kobj);
> + if (kobject_add(&p->kobj)) {
> + kobject_put(&p->kobj);
> + return;
> + }
> if (!disk->part_uevent_suppress)
> kobject_uevent(&p->kobj, KOBJ_ADD);
> - sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
> + if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem")) {
> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> + kobject_del(&p->kobj);
> + kobject_put(&p->kobj);
> + return;
> + }
> if (flags & ADDPART_FLAG_WHOLEDISK) {
> static struct attribute addpartattr = {
> .name = "whole_disk",
> @@ -396,7 +404,13 @@ void add_partition(struct gendisk *disk,
> .owner = THIS_MODULE,
> };
>
> - sysfs_create_file(&p->kobj, &addpartattr);
> + if (sysfs_create_file(&p->kobj, &addpartattr)) {
> + sysfs_remove_link(&p->kobj, "subsystem");
> + kobject_uevent(&p->kobj, KOBJ_REMOVE);
> + kobject_del(&p->kobj);
> + kobject_put(&p->kobj);
> + return;
> + }
> }
> partition_sysfs_add_subdir(p);
> disk->part[part-1] = p;
Your mail client replaces tabs with spaces - please fix it.
The code duplication is unpleasing. We normally do this as below (please
review this code).
Note that I changed it to not send the KOBJ_REMOVE if we didn't send a
KOBJ_ADD.
From: WANG Cong <xiyou.wangcong@gmail.com>
Since kobject_add, sysfs_create_link and sysfs_create_file are marked as
'__must_check', we must always check their return values.
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
fs/partitions/check.c | 25 ++++++++++++++++++++-----
1 files changed, 20 insertions(+), 5 deletions(-)
diff -puN fs/partitions/check.c~partitions-check-the-return-value-of-kobject_add-etc fs/partitions/check.c
--- a/fs/partitions/check.c~partitions-check-the-return-value-of-kobject_add-etc
+++ a/fs/partitions/check.c
@@ -383,26 +383,41 @@ void add_partition(struct gendisk *disk,
p->policy = disk->policy;
if (isdigit(disk->kobj.name[strlen(disk->kobj.name)-1]))
- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%sp%d",disk->kobj.name,part);
+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%sp%d",
+ disk->kobj.name, part);
else
- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%s%d",disk->kobj.name,part);
+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%s%d",
+ disk->kobj.name, part);
p->kobj.parent = &disk->kobj;
p->kobj.ktype = &ktype_part;
kobject_init(&p->kobj);
- kobject_add(&p->kobj);
+ if (kobject_add(&p->kobj))
+ goto out_put;
if (!disk->part_uevent_suppress)
kobject_uevent(&p->kobj, KOBJ_ADD);
- sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem"))
+ goto out_uevent;
if (flags & ADDPART_FLAG_WHOLEDISK) {
static struct attribute addpartattr = {
.name = "whole_disk",
.mode = S_IRUSR | S_IRGRP | S_IROTH,
};
- sysfs_create_file(&p->kobj, &addpartattr);
+ if (sysfs_create_file(&p->kobj, &addpartattr))
+ goto out_link;
}
partition_sysfs_add_subdir(p);
disk->part[part-1] = p;
+ return;
+
+out_link:
+ sysfs_remove_link(&p->kobj, "subsystem");
+out_uevent:
+ if (!disk->part_uevent_suppress)
+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
+ kobject_del(&p->kobj);
+out_put:
+ kobject_put(&p->kobj);
}
static char *make_block_name(struct gendisk *disk)
_
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-10 22:18 ` Andrew Morton
@ 2007-04-11 6:45 ` WANG Cong
2007-04-11 7:19 ` Andrew Morton
2007-04-11 9:24 ` Cornelia Huck
1 sibling, 1 reply; 18+ messages in thread
From: WANG Cong @ 2007-04-11 6:45 UTC (permalink / raw)
To: Andrew Morton; +Cc: Cornelia Huck, linux-kernel, viro
On Tue, Apr 10, 2007 at 03:18:15PM -0700, Andrew Morton wrote:
>On Tue, 10 Apr 2007 22:08:29 +0800
>WANG Cong <xiyou.wangcong@gmail.com> wrote:
>
>> Since kobject_add, sysfs_create_link and sysfs_create_file are marked as '__must_check', we must always check their return values.
>>
<snip>
>
>Your mail client replaces tabs with spaces - please fix it.
>
Oh, I did't notice that. Thank you. I will fix it soon.
>The code duplication is unpleasing. We normally do this as below (please
>review this code).
>
Er, I see.
>Note that I changed it to not send the KOBJ_REMOVE if we didn't send a
>KOBJ_ADD.
>
I think you are right.
>
>From: WANG Cong <xiyou.wangcong@gmail.com>
>
>Since kobject_add, sysfs_create_link and sysfs_create_file are marked as
>'__must_check', we must always check their return values.
>
>Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>---
>
> fs/partitions/check.c | 25 ++++++++++++++++++++-----
> 1 files changed, 20 insertions(+), 5 deletions(-)
>
>diff -puN fs/partitions/check.c~partitions-check-the-return-value-of-kobject_add-etc fs/partitions/check.c
>--- a/fs/partitions/check.c~partitions-check-the-return-value-of-kobject_add-etc
>+++ a/fs/partitions/check.c
>@@ -383,26 +383,41 @@ void add_partition(struct gendisk *disk,
> p->policy = disk->policy;
>
> if (isdigit(disk->kobj.name[strlen(disk->kobj.name)-1]))
>- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%sp%d",disk->kobj.name,part);
>+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%sp%d",
>+ disk->kobj.name, part);
^^^
Andrew, it seems that you left an additional whitespace in the above line (marked as ^^^).
> else
>- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%s%d",disk->kobj.name,part);
>+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%s%d",
>+ disk->kobj.name, part);
^^^
Also here. ;-p
> p->kobj.parent = &disk->kobj;
> p->kobj.ktype = &ktype_part;
> kobject_init(&p->kobj);
>- kobject_add(&p->kobj);
>+ if (kobject_add(&p->kobj))
>+ goto out_put;
> if (!disk->part_uevent_suppress)
> kobject_uevent(&p->kobj, KOBJ_ADD);
>- sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem");
>+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem"))
>+ goto out_uevent;
> if (flags & ADDPART_FLAG_WHOLEDISK) {
> static struct attribute addpartattr = {
> .name = "whole_disk",
> .mode = S_IRUSR | S_IRGRP | S_IROTH,
> };
>
>- sysfs_create_file(&p->kobj, &addpartattr);
>+ if (sysfs_create_file(&p->kobj, &addpartattr))
>+ goto out_link;
> }
> partition_sysfs_add_subdir(p);
> disk->part[part-1] = p;
>+ return;
>+
>+out_link:
>+ sysfs_remove_link(&p->kobj, "subsystem");
>+out_uevent:
>+ if (!disk->part_uevent_suppress)
>+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
>+ kobject_del(&p->kobj);
>+out_put:
>+ kobject_put(&p->kobj);
> }
>
> static char *make_block_name(struct gendisk *disk)
>_
Your code is better. Thanks again!
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-11 6:45 ` WANG Cong
@ 2007-04-11 7:19 ` Andrew Morton
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Morton @ 2007-04-11 7:19 UTC (permalink / raw)
To: WANG Cong; +Cc: Cornelia Huck, linux-kernel, viro
On Wed, 11 Apr 2007 14:45:25 +0800 WANG Cong <xiyou.wangcong@gmail.com> wrote:
> >- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%sp%d",disk->kobj.name,part);
> >+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%sp%d",
> >+ disk->kobj.name, part);
> ^^^
> Andrew, it seems that you left an additional whitespace in the above line (marked as ^^^).
>
> > else
> >- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%s%d",disk->kobj.name,part);
> >+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%s%d",
> >+ disk->kobj.name, part);
> ^^^
> Also here. ;-p
Those are known as "fixes" ;)
One shouldn't mix whitespace fixes with functional changes really, but I
can't help myself.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-10 22:18 ` Andrew Morton
2007-04-11 6:45 ` WANG Cong
@ 2007-04-11 9:24 ` Cornelia Huck
1 sibling, 0 replies; 18+ messages in thread
From: Cornelia Huck @ 2007-04-11 9:24 UTC (permalink / raw)
To: Andrew Morton; +Cc: WANG Cong, linux-kernel, viro
On Tue, 10 Apr 2007 15:18:15 -0700,
Andrew Morton <akpm@linux-foundation.org> wrote:
> Note that I changed it to not send the KOBJ_REMOVE if we didn't send a
> KOBJ_ADD.
The whole part_uevent_suppress stuff is not really nice, causing the
code to use a lot of kobject_{init,add} and kobject_{del,put} instead
of kobject_register and kobject_unregister. Looking at
driver-class/block-device.patch in Greg's tree, it should be possible
to use something like the uevent_filter stuff after block has been
converted.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
2007-04-01 8:22 Parag Warudkar
@ 2007-04-01 13:46 ` Parag Warudkar
0 siblings, 0 replies; 18+ messages in thread
From: Parag Warudkar @ 2007-04-01 13:46 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: linux-kernel
On 4/1/07, Parag Warudkar <parag.warudkar@gmail.com> wrote:
> >- kobject_add(&p->kobj);
> >+ if (kobject_add(&p->kobj)) {
> >+ kfree(p);
> >+ return;
>
> Please add a printk warning before the return statement to log a
> proper warning stating what happened, which file and line etc. That
> way people can know why something did not work as expected and
> hopefully do something about it.
Never mind - kobject_add() is already verbose enough when it fails.
Parag
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
@ 2007-04-01 8:22 Parag Warudkar
2007-04-01 13:46 ` Parag Warudkar
0 siblings, 1 reply; 18+ messages in thread
From: Parag Warudkar @ 2007-04-01 8:22 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: linux-kernel
>- kobject_add(&p->kobj);
>+ if (kobject_add(&p->kobj)) {
>+ kfree(p);
>+ return;
Please add a printk warning before the return statement to log a
proper warning stating what happened, which file and line etc. That
way people can know why something did not work as expected and
hopefully do something about it.
Parag
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2007-04-11 9:22 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-04-01 7:32 [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc Cong WANG
2007-04-02 11:01 ` Cornelia Huck
2007-04-05 4:54 ` WANG Cong
2007-04-05 9:11 ` Cornelia Huck
2007-04-05 14:44 ` WANG Cong
2007-04-05 15:05 ` Cornelia Huck
2007-04-05 15:27 ` WANG Cong
2007-04-05 16:00 ` Cornelia Huck
2007-04-06 2:53 ` WANG Cong
2007-04-10 12:31 ` Cornelia Huck
2007-04-10 12:35 ` [Patch -mm] kobject: kobject_add() reference leak Cornelia Huck
2007-04-10 14:08 ` [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc WANG Cong
2007-04-10 22:18 ` Andrew Morton
2007-04-11 6:45 ` WANG Cong
2007-04-11 7:19 ` Andrew Morton
2007-04-11 9:24 ` Cornelia Huck
2007-04-01 8:22 Parag Warudkar
2007-04-01 13:46 ` Parag Warudkar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.