[-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Cong WANG]

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;

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Cornelia Huck]

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.

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[WANG Cong]

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;

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Cornelia Huck]

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;
> 
> 
> 

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[WANG Cong]

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! 

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Cornelia Huck]

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.)

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[WANG Cong]

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?

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Cornelia Huck]

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().

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[WANG Cong]

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;

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Cornelia Huck]

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;
> 
> 

[Patch -mm] kobject: kobject_add() reference leak

[Cornelia Huck]

<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);

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[WANG Cong]

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;

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Andrew Morton]

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)
_

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[WANG Cong]

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!

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Andrew Morton]

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.

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Cornelia Huck]

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.

RE: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Parag Warudkar]

>-	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

Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[Parag Warudkar]

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