Re: [PATCH] ALSA: fix kernel panic in initialization of mpu401 driver

[Jaroslav Kysela]

On Wed, 4 Oct 2006, Jiri Kosina wrote:

> I am getting kernel panic (NULL pointer dereference) on boot, with kernel 
> compiled with CONFIG_SND_MPU401_UART=y, on machine which does not have 
> this piece of hardware.
> 
> I have traced the problem down to 
> sound/drivers/mpu401/mpu401.c:snd_mpu401_probe() returning EINVAL, when 
> either port or IRQ parameters are not specified.
> 
> In such case, the drivers/base/bus.c:bus_attach_device() does not perform 
> klist_add_tail() call, but rather sets dev->is_registered to 0. This flag 
> is however not checked by the driver, so later on, when 
> alsa_card_mpu401_init() is called and platform_device_register_simple() 
> fails, the following callchain happens, causing NULL pointer dereference: 
> alsa_card_mpu401_init() -> platform_device_unregister() -> 
> platform_device_del() -> device_del() -> bus_remove_device() -> 
> klist_del() -> BOOM (the entry was not added to klist in 
> bus_attach_device()).
> 
> Proper solution is returning ENODEV from the ->probe() routine, which will 
> be correctly handled then by the rest of the device-driver attaching 
> subsystem (namely the retval check in bus_attach_device()). The following 
> patch fixes the problem, please apply.

Unfortunately, I do not think that it's a proper solution. I think that
platform device layer should play more nicely and if probe() fails for 
a reason and if platform_device_register_simple() does not set 
IS_ERR(), then platform_device_unregister() must be callable to free
all resources.

Also, you've proposed to not fix all returns in snd_mpu401_probe() only 
first two.

I would reject this patch and fix drivers/base/bus.c. The problematic 
change is in commit f2eaae197f4590c4d96f31b09b0ee9067421a95c and this 
patch will probably fix it:

[PATCH] drivers/base - check if device is registered before removal

Without this fix platform_device_unregister() might oops.

Signed-off-by: Jaroslav Kysela <perex@xxxxxxx>

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 12173d1..daa2390 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -428,8 +428,10 @@ void bus_remove_device(struct device * d
 		sysfs_remove_link(&dev->kobj, "bus");
 		sysfs_remove_link(&dev->bus->devices.kobj, dev->bus_id);
 		device_remove_attrs(dev->bus, dev);
-		dev->is_registered = 0;
-		klist_del(&dev->knode_bus);
+		if (dev->is_registered) {
+			dev->is_registered = 0;
+			klist_del(&dev->knode_bus);
+		}
 		pr_debug("bus %s: remove device %s\n", dev->bus->name, dev->bus_id);
 		device_release_driver(dev);
 		put_bus(dev->bus);

						Jaroslav

-----
Jaroslav Kysela <perex@xxxxxxx>
Linux Kernel Sound Maintainer
ALSA Project, SUSE Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/