kobject_get_path() will oops if one of the component names is
NULL. Fix that by returning NULL instead of oopsing.
Signed-off-by: Chuck Ebbert <[email protected]>
---
Helge, this fixes your "2.6.15 OOPS while trying to mount cdrom".
Probably not the best fix, but It Works For Me (TM).
--- 2.6.15a.orig/lib/kobject.c
+++ 2.6.15a/lib/kobject.c
@@ -72,6 +72,8 @@ static int get_kobj_path_length(struct k
* Add 1 to strlen for leading '/' of each level.
*/
do {
+ if (kobject_name(parent) == NULL)
+ return 0;
length += strlen(kobject_name(parent)) + 1;
parent = parent->parent;
} while (parent);
@@ -107,6 +109,8 @@ char *kobject_get_path(struct kobject *k
int len;
len = get_kobj_path_length(kobj);
+ if (len == 0)
+ return NULL;
path = kmalloc(len, gfp_mask);
if (!path)
return NULL;
--
Chuck
Currently reading: _Olympos_ by Dan Simmons
Chuck Ebbert <[email protected]> wrote:
>
> kobject_get_path() will oops if one of the component names is
> NULL. Fix that by returning NULL instead of oopsing.
>
> Signed-off-by: Chuck Ebbert <[email protected]>
> ---
>
> Helge, this fixes your "2.6.15 OOPS while trying to mount cdrom".
>
> Probably not the best fix, but It Works For Me (TM).
>
> --- 2.6.15a.orig/lib/kobject.c
> +++ 2.6.15a/lib/kobject.c
> @@ -72,6 +72,8 @@ static int get_kobj_path_length(struct k
> * Add 1 to strlen for leading '/' of each level.
> */
> do {
> + if (kobject_name(parent) == NULL)
> + return 0;
> length += strlen(kobject_name(parent)) + 1;
> parent = parent->parent;
> } while (parent);
> @@ -107,6 +109,8 @@ char *kobject_get_path(struct kobject *k
> int len;
>
> len = get_kobj_path_length(kobj);
> + if (len == 0)
> + return NULL;
> path = kmalloc(len, gfp_mask);
> if (!path)
> return NULL;
I'd have thought that we'd want the test right at the start of
kobject_add() - fail it if ->name is zero. I don't know if that'd work for
all callers, but kobject_add() does play around with the ->name field and
will go oops if ->name==NULL and debugging is enabled.
Why did you choose kobject_get_path()?
On Fri, Jan 13, 2006 at 02:30:13PM -0800, Andrew Morton wrote:
> Chuck Ebbert <[email protected]> wrote:
> >
> > kobject_get_path() will oops if one of the component names is
> > NULL. Fix that by returning NULL instead of oopsing.
> >
> > Signed-off-by: Chuck Ebbert <[email protected]>
> > ---
> >
> > Helge, this fixes your "2.6.15 OOPS while trying to mount cdrom".
> >
> > Probably not the best fix, but It Works For Me (TM).
> >
> > --- 2.6.15a.orig/lib/kobject.c
> > +++ 2.6.15a/lib/kobject.c
> > @@ -72,6 +72,8 @@ static int get_kobj_path_length(struct k
> > * Add 1 to strlen for leading '/' of each level.
> > */
> > do {
> > + if (kobject_name(parent) == NULL)
> > + return 0;
> > length += strlen(kobject_name(parent)) + 1;
> > parent = parent->parent;
> > } while (parent);
> > @@ -107,6 +109,8 @@ char *kobject_get_path(struct kobject *k
> > int len;
> >
> > len = get_kobj_path_length(kobj);
> > + if (len == 0)
> > + return NULL;
> > path = kmalloc(len, gfp_mask);
> > if (!path)
> > return NULL;
>
> I'd have thought that we'd want the test right at the start of
> kobject_add() - fail it if ->name is zero. I don't know if that'd work for
> all callers, but kobject_add() does play around with the ->name field and
> will go oops if ->name==NULL and debugging is enabled.
Something like this instead? (warning, untested...)
I'll try it out in a reboot cycle...
thanks,
greg k-h
--- gregkh-2.6.orig/lib/kobject.c 2006-01-13 09:15:18.000000000 -0800
+++ gregkh-2.6/lib/kobject.c 2006-01-13 14:54:40.000000000 -0800
@@ -164,6 +164,11 @@ int kobject_add(struct kobject * kobj)
return -ENOENT;
if (!kobj->k_name)
kobj->k_name = kobj->name;
+ if (!kobj->k_name) {
+ pr_debug("kobject attempted to be registered with no name!\n");
+ WARN_ON(1);
+ return -EINVAL;
+ }
parent = kobject_get(kobj->parent);
pr_debug("kobject %s: registering. parent: %s, set: %s\n",
Greg KH <[email protected]> wrote:
>
> >
> > I'd have thought that we'd want the test right at the start of
> > kobject_add() - fail it if ->name is zero. I don't know if that'd work for
> > all callers, but kobject_add() does play around with the ->name field and
> > will go oops if ->name==NULL and debugging is enabled.
>
> Something like this instead?
I think so.
> (warning, untested...)
Ship it!
> I'll try it out in a reboot cycle...
>
> --- gregkh-2.6.orig/lib/kobject.c 2006-01-13 09:15:18.000000000 -0800
> +++ gregkh-2.6/lib/kobject.c 2006-01-13 14:54:40.000000000 -0800
> @@ -164,6 +164,11 @@ int kobject_add(struct kobject * kobj)
> return -ENOENT;
> if (!kobj->k_name)
> kobj->k_name = kobj->name;
> + if (!kobj->k_name) {
> + pr_debug("kobject attempted to be registered with no name!\n");
> + WARN_ON(1);
> + return -EINVAL;
> + }
> parent = kobject_get(kobj->parent);
>
> pr_debug("kobject %s: registering. parent: %s, set: %s\n",
It might be worth emitting the warning and then proceeding rather than
failing - minimise potential disruption. I guess we'll see...
On Fri, Jan 13, 2006 at 03:12:13PM -0800, Andrew Morton wrote:
> Greg KH <[email protected]> wrote:
> >
> > >
> > > I'd have thought that we'd want the test right at the start of
> > > kobject_add() - fail it if ->name is zero. I don't know if that'd work for
> > > all callers, but kobject_add() does play around with the ->name field and
> > > will go oops if ->name==NULL and debugging is enabled.
> >
> > Something like this instead?
>
> I think so.
>
> > (warning, untested...)
>
> Ship it!
Heh, it works for me, I'm running with it right now :)
>
> > I'll try it out in a reboot cycle...
> >
> > --- gregkh-2.6.orig/lib/kobject.c 2006-01-13 09:15:18.000000000 -0800
> > +++ gregkh-2.6/lib/kobject.c 2006-01-13 14:54:40.000000000 -0800
> > @@ -164,6 +164,11 @@ int kobject_add(struct kobject * kobj)
> > return -ENOENT;
> > if (!kobj->k_name)
> > kobj->k_name = kobj->name;
> > + if (!kobj->k_name) {
> > + pr_debug("kobject attempted to be registered with no name!\n");
> > + WARN_ON(1);
> > + return -EINVAL;
> > + }
> > parent = kobject_get(kobj->parent);
> >
> > pr_debug("kobject %s: registering. parent: %s, set: %s\n",
>
> It might be worth emitting the warning and then proceeding rather than
> failing - minimise potential disruption. I guess we'll see...
Hm, I looked at the only user of kobjects in the kernel that I know of
that doesn't use sysfs (the cdev code) and even it sets the kobject name
to something sane, so I think we should be safe with this.
I'll add it to my tree and let's see what the next -mm causes to pop up
:)
thanks,
greg k-h
In-Reply-To: <[email protected]>
On Fri, 13 Jan 2006, Greg KH wrote:
> Hm, I looked at the only user of kobjects in the kernel that I know of
> that doesn't use sysfs (the cdev code) and even it sets the kobject name
> to something sane, so I think we should be safe with this.
Well, something is still wrong.
I applied your patch to prevent registration of objects with null names on
top of my patch, then applied this to see if my test still triggered, and
the message was printed:
--- 2.6.15a.orig/lib/kobject.c
+++ 2.6.15a/lib/kobject.c
@@ -72,8 +72,10 @@ static int get_kobj_path_length(struct k
* Add 1 to strlen for leading '/' of each level.
*/
do {
- if (kobject_name(parent) == NULL)
+ if (kobject_name(parent) == NULL) {
+ printk("get_kobj_path_length: encountered NULL name\n");
return 0;
+ }
length += strlen(kobject_name(parent)) + 1;
parent = parent->parent;
} while (parent);
To reproduce:
Start with vanilla 2.6.15 and apply the three patches, which I called:
kobject_dont_register_null_name.patch <- my original
kobject_handle_null_object_name.patch <- Greg's
kobject_debug_null_path.patch <- included above
On a machine with an ATAPI CD-ROM, boot with "hdx=ide-scsi" where
hdx is the CD-ROM's drivename. Then try to mount a CD:
mount -t iso9660 /dev/hdx /mnt/cdrom
Note that hdx is being controlled by ide-scsi so this should fail. You
will see the message from my new patch print in the kernel log.
NOTE: This won't happen on 2.6.15-current because
fs/super.c::kill_block_super() no longer calls kobject_uevent().
--
Chuck
Currently reading: _Olympos_ by Dan Simmons
In-Reply-To: <[email protected]>
References: <[email protected]>
On Fri, 13 Jan 2006, Andrew Morton wrote:
> Why did you choose kobject_get_path()?
This is the piece of code that was oopsing in Helge Hafting's
bug report: "2.6.15 OOPS while trying to mount cdrom". This
patch solves that by fixing the symptoms.
Details are in that thread.
--
Chuck
Currently reading: _Olympos_ by Dan Simmons
On Fri, Jan 13, 2006 at 10:07:33PM -0500, Chuck Ebbert wrote:
> In-Reply-To: <[email protected]>
>
> On Fri, 13 Jan 2006, Greg KH wrote:
>
> > Hm, I looked at the only user of kobjects in the kernel that I know of
> > that doesn't use sysfs (the cdev code) and even it sets the kobject name
> > to something sane, so I think we should be safe with this.
>
> Well, something is still wrong.
>
> I applied your patch to prevent registration of objects with null names on
> top of my patch, then applied this to see if my test still triggered, and
> the message was printed:
What was the message? What traceback?
So, I think the point is that it isn't a kobject_add() issue, right?
>
>
> --- 2.6.15a.orig/lib/kobject.c
> +++ 2.6.15a/lib/kobject.c
> @@ -72,8 +72,10 @@ static int get_kobj_path_length(struct k
> * Add 1 to strlen for leading '/' of each level.
> */
> do {
> - if (kobject_name(parent) == NULL)
> + if (kobject_name(parent) == NULL) {
> + printk("get_kobj_path_length: encountered NULL name\n");
> return 0;
> + }
> length += strlen(kobject_name(parent)) + 1;
> parent = parent->parent;
> } while (parent);
>
>
> To reproduce:
>
> Start with vanilla 2.6.15 and apply the three patches, which I called:
>
> kobject_dont_register_null_name.patch <- my original
> kobject_handle_null_object_name.patch <- Greg's
> kobject_debug_null_path.patch <- included above
>
> On a machine with an ATAPI CD-ROM, boot with "hdx=ide-scsi" where
> hdx is the CD-ROM's drivename. Then try to mount a CD:
>
> mount -t iso9660 /dev/hdx /mnt/cdrom
>
> Note that hdx is being controlled by ide-scsi so this should fail. You
> will see the message from my new patch print in the kernel log.
>
> NOTE: This won't happen on 2.6.15-current because
> fs/super.c::kill_block_super() no longer calls kobject_uevent().
So everything's fixed and we don't have to worry about it anymore :)
Seriously, I agree, we still need to fix it for -stable.
thanks,
greg k-h
In-Reply-To: <[email protected]>
On Fri, 13 Jan 2006, Greg KH wrote:
> > I applied your patch to prevent registration of objects with null names on
> > top of my patch, then applied this to see if my test still triggered, and
> > the message was printed:
>
> What was the message? What traceback?
>
> So, I think the point is that it isn't a kobject_add() issue, right?
My message was printed:
get_kobj_path_length: encountered NULL name
So an uninitialized kobject was passed to kobject_get_path().
This is likely a problem somewhere in IDE when "hdx=ide-scsi' is used.
--
Chuck
Currently reading: _Olympos_ by Dan Simmons