Re: [PATCH] Input: Avoid kernel panic during device unregistration

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Aniroop,

On Mon, Dec 29, 2014 at 10:11:54PM +0530, Aniroop Mathur wrote:
> Hello Mr. Torokhov,
> 
> On Mon, Dec 29, 2014 at 1:51 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Aniroop,
> >
> > On Sun, Dec 28, 2014 at 11:42:05PM +0530, Aniroop Mathur wrote:
> >> This patch adds null check before actually unregistering the input device
> >> to avoid null pointer exception which leads to kernel panic.
> >>
> >> So now, input device drivers won't have to worry about or add null case
> >> condition before calling input_unregister_device() in shutdown and
> >> remove functions.
> >
> > input_unregister_device() should be called only if
> > input_register_device() succeeded, which it would not with input device
> > being NULL.
> >
> > Unlike input_free_device() which does handle NULL argument, similar to
> > many other "free" APIs I do not believe that input_unregister_device
> > should be handling such cases.
> >
> 
> Right !!
> Actually, quite recently I worked on one input device hub driver in which many
> devices are registered in probe and in shutdown and remove functions,
> they are unregistered.
> 
> probe() {
> ...
> ...
> accel_dev = input_register_device();
> gyro_dev = input_register_device();
> mag_dev = input_register_device();
> prox_dev = input_register_device();
> light_dev = input_register_device();
> baro_dev = input_register_device();
> more ...
> ...
> }
> 
> shutdown() {
> ...
> ...
> if (accel_dev)
> input_unregister_device(accel_dev);
> if (gyro_dev)
> input_unregister_device(gyro_dev);
> if (mag_dev)
> input_unregister_device(mag_dev);
> if (prox_dev )
> input_unregister_device(prox_dev);
> if (light_dev)
> input_unregister_device(light_dev);
> if (baro_dev)
> input_unregister_device(baro_dev);
> more ...
> ...
> }
> 
> In probe, few registrations may fail and so it is freed in probe itself.

Why would they fail? Is it because the hardware is not there or other
errors?

> And in driver shutdown function, we need to unregister or free devices
> registered in probe.
> So adding null check before every input_device_unregister() looks not
> quite good.
> Similar thing for remove function in driver.
> The best solution I thought is to add null check in input subsystem
> unregister function itself.
> Umm... Is there any better way possible ?

I would look into using devm_* infrastructure instead and simply not
allocate input devices for any sub-devices of your "hub" that are not
present and simply aborting the probe() for other errors. Then you
would not need pretty much any code in your remove() method.

If not devm_ then you can consider creating array of struct input_dev *
and iterating it in error paths and remove() instead of long open-coded
sequence of unregistering. Then a single NULL check won't be seen as
such an issue.

Thanks.

-- 
Dmitry