[patch] kobject: don't oops on null kobject.name

[patch] kobject: don't oops on null kobject.name

[Chuck Ebbert]

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 <76306.1226@compuserve.com>
---

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

Re: [patch] kobject: don't oops on null kobject.name

[Andrew Morton]

Chuck Ebbert <76306.1226@compuserve.com> 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 <76306.1226@compuserve.com>
> ---
> 
> 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()?

Re: [patch] kobject: don't oops on null kobject.name

[Greg KH]

On Fri, Jan 13, 2006 at 02:30:13PM -0800, Andrew Morton wrote:
> Chuck Ebbert <76306.1226@compuserve.com> 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 <76306.1226@compuserve.com>
> > ---
> > 
> > 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",

Re: [patch] kobject: don't oops on null kobject.name

[Andrew Morton]

Greg KH <greg@kroah.com> 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...

Re: [patch] kobject: don't oops on null kobject.name

[Greg KH]

On Fri, Jan 13, 2006 at 03:12:13PM -0800, Andrew Morton wrote:
> Greg KH <greg@kroah.com> 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

Re: [patch] kobject: don't oops on null kobject.name

[Chuck Ebbert]

In-Reply-To: <20060113143013.0ed0f9c0.akpm@osdl.org>
References: <20060110184624.GA6721@aitel.hist.no>

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

Re: [patch] kobject: don't oops on null kobject.name

[Chuck Ebbert]

In-Reply-To: <20060114000246.GA7549@kroah.com>

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

Re: [patch] kobject: don't oops on null kobject.name

[Greg KH]

On Fri, Jan 13, 2006 at 10:07:33PM -0500, Chuck Ebbert wrote:
> In-Reply-To: <20060114000246.GA7549@kroah.com>
> 
> 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

Re: [patch] kobject: don't oops on null kobject.name

[Chuck Ebbert]

In-Reply-To: <20060114034404.GA23061@kroah.com>

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

[PATCH] SPI: spi_butterfly, restore lost deltas

[Greg KH]

[PATCH] SPI: spi_butterfly, restore lost deltas

This resolves some minor version skew glitches that accumulated for the AVR
Butterfly adapter driver, which caused among other things the existence of
a duplicate Kconfig entry.  Most of it boils down to comment updates, but in
one case it removes some now-superfluous code that would be better if not
copied into other controller-level drivers.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

---
commit 9c1da3cb46316e40bac766ce45556dc4fd8df3ca
tree d2ab578f2601383f39d316dfca0f00d12da21dba
parent 022f7b07bf2b384ece7fbd7edb90e54cd78db252
author David Brownell <david-b@pacbell.net> Sat, 21 Jan 2006 13:21:43 -0800
committer Greg Kroah-Hartman <gregkh@suse.de> Mon, 06 Feb 2006 12:17:17 -0800

 Documentation/spi/butterfly |   23 +++++++++++++++++------
 drivers/spi/Kconfig         |   10 ----------
 drivers/spi/spi_butterfly.c |   36 +++++++++++++++++-------------------
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/Documentation/spi/butterfly b/Documentation/spi/butterfly
index a2e8c8d..9927af7 100644
--- a/Documentation/spi/butterfly
+++ b/Documentation/spi/butterfly
@@ -12,13 +12,20 @@ You can make this adapter from an old pr
 directly to the Butterfly.  Or (if you have the parts and skills) you
 can come up with something fancier, providing ciruit protection to the
 Butterfly and the printer port, or with a better power supply than two
-signal pins from the printer port.
+signal pins from the printer port.  Or for that matter, you can use
+similar cables to talk to many AVR boards, even a breadboard.
+
+This is more powerful than "ISP programming" cables since it lets kernel
+SPI protocol drivers interact with the AVR, and could even let the AVR
+issue interrupts to them.  Later, your protocol driver should work
+easily with a "real SPI controller", instead of this bitbanger.
 
 
 The first cable connections will hook Linux up to one SPI bus, with the
 AVR and a DataFlash chip; and to the AVR reset line.  This is all you
 need to reflash the firmware, and the pins are the standard Atmel "ISP"
-connector pins (used also on non-Butterfly AVR boards).
+connector pins (used also on non-Butterfly AVR boards).  On the parport
+side this is like "sp12" programming cables.
 
 	Signal	  Butterfly	  Parport (DB-25)
 	------	  ---------	  ---------------
@@ -40,10 +47,14 @@ by clearing PORTB.[0-3]); (b) configure 
 	SELECT	= J400.PB0/nSS	= pin 17/C3,nSELECT
 	GND	= J400.GND	= pin 24/GND
 
-The "USI" controller, using J405, can be used for a second SPI bus.  That
-would let you talk to the AVR over SPI, running firmware that makes it act
-as an SPI slave, while letting either Linux or the AVR use the DataFlash.
-There are plenty of spare parport pins to wire this one up, such as:
+Or you could flash firmware making the AVR into an SPI slave (keeping the
+DataFlash in reset) and tweak the spi_butterfly driver to make it bind to
+the driver for your custom SPI-based protocol.
+
+The "USI" controller, using J405, can also be used for a second SPI bus.
+That would let you talk to the AVR using custom SPI-with-USI firmware,
+while letting either Linux or the AVR use the DataFlash.  There are plenty
+of spare parport pins to wire this one up, such as:
 
 	Signal	  Butterfly	  Parport (DB-25)
 	------	  ---------	  ---------------
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index b77dbd6..7a75fae 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -75,16 +75,6 @@ config SPI_BUTTERFLY
 	  inexpensive battery powered microcontroller evaluation board.
 	  This same cable can be used to flash new firmware.
 
-config SPI_BUTTERFLY
-	tristate "Parallel port adapter for AVR Butterfly (DEVELOPMENT)"
-	depends on SPI_MASTER && PARPORT && EXPERIMENTAL
-	select SPI_BITBANG
-	help
-	  This uses a custom parallel port cable to connect to an AVR
-	  Butterfly <http://www.atmel.com/products/avr/butterfly>, an
-	  inexpensive battery powered microcontroller evaluation board.
-	  This same cable can be used to flash new firmware.
-
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
diff --git a/drivers/spi/spi_butterfly.c b/drivers/spi/spi_butterfly.c
index 79a3c59..ff9e5fa 100644
--- a/drivers/spi/spi_butterfly.c
+++ b/drivers/spi/spi_butterfly.c
@@ -163,21 +163,20 @@ static void butterfly_chipselect(struct 
 	struct butterfly	*pp = spidev_to_pp(spi);
 
 	/* set default clock polarity */
-	if (value)
+	if (value != BITBANG_CS_INACTIVE)
 		setsck(spi, spi->mode & SPI_CPOL);
 
 	/* no chipselect on this USI link config */
 	if (is_usidev(spi))
 		return;
 
-	/* here, value == "activate or not" */
-
-	/* most PARPORT_CONTROL_* bits are negated */
+	/* here, value == "activate or not";
+	 * most PARPORT_CONTROL_* bits are negated, so we must
+	 * morph it to value == "bit value to write in control register"
+	 */
 	if (spi_cs_bit == PARPORT_CONTROL_INIT)
 		value = !value;
 
-	/* here, value == "bit value to write in control register"  */
-
 	parport_frob_control(pp->port, spi_cs_bit, value ? spi_cs_bit : 0);
 }
 
@@ -202,7 +201,9 @@ butterfly_txrx_word_mode0(struct spi_dev
 
 /* override default partitioning with cmdlinepart */
 static struct mtd_partition partitions[] = { {
-	/* JFFS2 wants partitions of 4*N blocks for this device ... */
+	/* JFFS2 wants partitions of 4*N blocks for this device,
+	 * so sectors 0 and 1 can't be partitions by themselves.
+	 */
 
 	/* sector 0 = 8 pages * 264 bytes/page (1 block)
 	 * sector 1 = 248 pages * 264 bytes/page
@@ -316,8 +317,9 @@ static void butterfly_attach(struct parp
 	if (status < 0)
 		goto clean2;
 
-	/* Bus 1 lets us talk to at45db041b (firmware disables AVR)
-	 * or AVR (firmware resets at45, acts as spi slave)
+	/* Bus 1 lets us talk to at45db041b (firmware disables AVR SPI), AVR
+	 * (firmware resets at45, acts as spi slave) or neither (we ignore
+	 * both, AVR uses AT45).  Here we expect firmware for the first option.
 	 */
 	pp->info[0].max_speed_hz = 15 * 1000 * 1000;
 	strcpy(pp->info[0].modalias, "mtd_dataflash");
@@ -330,7 +332,9 @@ static void butterfly_attach(struct parp
 				pp->dataflash->dev.bus_id);
 
 #ifdef	HAVE_USI
-	/* even more custom AVR firmware */
+	/* Bus 2 is only for talking to the AVR, and it can work no
+	 * matter who masters bus 1; needs appropriate AVR firmware.
+	 */
 	pp->info[1].max_speed_hz = 10 /* ?? */ * 1000 * 1000;
 	strcpy(pp->info[1].modalias, "butterfly");
 	// pp->info[1].platform_data = ... TBD ... ;
@@ -378,13 +382,8 @@ static void butterfly_detach(struct parp
 	pp = butterfly;
 	butterfly = NULL;
 
-#ifdef	HAVE_USI
-	spi_unregister_device(pp->butterfly);
-	pp->butterfly = NULL;
-#endif
-	spi_unregister_device(pp->dataflash);
-	pp->dataflash = NULL;
-
+	/* stop() unregisters child devices too */
+	pdev = to_platform_device(pp->bitbang.master->cdev.dev);
 	status = spi_bitbang_stop(&pp->bitbang);
 
 	/* turn off VCC */
@@ -394,8 +393,6 @@ static void butterfly_detach(struct parp
 	parport_release(pp->pd);
 	parport_unregister_device(pp->pd);
 
-	pdev = to_platform_device(pp->bitbang.master->cdev.dev);
-
 	(void) spi_master_put(pp->bitbang.master);
 
 	platform_device_unregister(pdev);
@@ -420,4 +417,5 @@ static void __exit butterfly_exit(void)
 }
 module_exit(butterfly_exit);
 
+MODULE_DESCRIPTION("Parport Adapter driver for AVR Butterfly");
 MODULE_LICENSE("GPL");

[PATCH] kobject: don't oops on null kobject.name

[Greg KH]

[PATCH] kobject: don't oops on null kobject.name

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 <76306.1226@compuserve.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
commit b365b3daf2a9e2a8b002ea9fef877af1c71513fd
tree dcd673d830b61ee37ab433af60c0f81ffaa86779
parent c171fef5c8566cf5f57877e7832fa696ecdf5228
author Chuck Ebbert <76306.1226@compuserve.com> Thu, 12 Jan 2006 20:02:00 -0500
committer Greg Kroah-Hartman <gregkh@suse.de> Mon, 06 Feb 2006 12:17:17 -0800

 lib/kobject.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index fe4ae36..efe67fa 100644
--- a/lib/kobject.c
+++ b/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;