[PATCH] Input: Avoid kernel panic during device unregistration

[PATCH] Input: Avoid kernel panic during device unregistration

[Aniroop Mathur]

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.

Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
 drivers/input/input.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 01fe49e..575219d 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2170,6 +2170,9 @@ EXPORT_SYMBOL(input_register_device);
  */
 void input_unregister_device(struct input_dev *dev)
 {
+	if (!dev)
+		return;
+
 	if (dev->devres_managed) {
 		WARN_ON(devres_destroy(dev->dev.parent,
 					devm_input_device_unregister,
-- 
1.9.1

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

[Dmitry Torokhov]

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.

> 
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> ---
>  drivers/input/input.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 01fe49e..575219d 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -2170,6 +2170,9 @@ EXPORT_SYMBOL(input_register_device);
>   */
>  void input_unregister_device(struct input_dev *dev)
>  {
> +	if (!dev)
> +		return;
> +
>  	if (dev->devres_managed) {
>  		WARN_ON(devres_destroy(dev->dev.parent,
>  					devm_input_device_unregister,
> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry

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

[Aniroop Mathur]

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.
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 ?


Regards,
Aniroop

>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>> ---
>>  drivers/input/input.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index 01fe49e..575219d 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -2170,6 +2170,9 @@ EXPORT_SYMBOL(input_register_device);
>>   */
>>  void input_unregister_device(struct input_dev *dev)
>>  {
>> +     if (!dev)
>> +             return;
>> +
>>       if (dev->devres_managed) {
>>               WARN_ON(devres_destroy(dev->dev.parent,
>>                                       devm_input_device_unregister,
>> --
>> 1.9.1
>>
>
> Thanks.
>
> --
> Dmitry

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

[Dmitry Torokhov]

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

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

[Aniroop Mathur]

Hello Mr. Torokhov,

On Mon, Dec 29, 2014 at 10:53 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> 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?
>

They never fail as such.
But still handling error chances for very rare cases like memory not
avaliable, etc.

>> 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.
>

Seems better approach. I'll try to follow the same.

Thanks,
Aniroop


> Thanks.
>
> --
> Dmitry