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

[Greg KH]

On Thu, Oct 05, 2006 at 04:16:01PM +0200, Jaroslav Kysela wrote:
> On Thu, 5 Oct 2006, Alan Stern wrote:
> 
> > On Thu, 5 Oct 2006, Jaroslav Kysela wrote:
> > 
> > > 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:
> > 
> > Your patch is right as far as it goes, but it misses part of the problem.  
> > device_add() shouldn't ignore the return value from bus_attach_device().  
> > That's why we ended up trying to unregister a device which never was 
> > properly registered in the first place.
> > 
> > What do you think of this addition to your patch (untested)?
> 
> Your addition looks good. Acked from me.

Great, Alan, care to resend with a better description and the proper
signed-off-by lines?

thanks,

greg k-h
-
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/