| Message ID | 1419790325-4004-1-git-send-email-aniroop.mathur@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(+)