[PATCH 06/24] input/keyboard: fix dangling pointers

[PATCH 06/24] input/keyboard: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/input/keyboard/lm8323.c |    2 ++
 drivers/input/keyboard/qt2160.c |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
index 574eda2..2b8bf05 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -768,6 +768,7 @@ fail2:
 			led_classdev_unregister(&lm->pwm[i].cdev);
 fail1:
 	input_free_device(idev);
+	i2c_set_clientdata(client, NULL);
 	kfree(lm);
 	return err;
 }
@@ -789,6 +790,7 @@ static int __devexit lm8323_remove(struct i2c_client *client)
 		if (lm->pwm[i].enabled)
 			led_classdev_unregister(&lm->pwm[i].cdev);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(lm);
 
 	return 0;
diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
index 31f3008..43db5e9 100644
--- a/drivers/input/keyboard/qt2160.c
+++ b/drivers/input/keyboard/qt2160.c
@@ -356,9 +356,9 @@ static int __devexit qt2160_remove(struct i2c_client *client)
 	cancel_delayed_work_sync(&qt2160->dwork);
 
 	input_unregister_device(qt2160->input);
+	i2c_set_clientdata(client, NULL);
 	kfree(qt2160);
 
-	i2c_set_clientdata(client, NULL);
 	return 0;
 }
 
-- 
1.7.0

Re: [PATCH 06/24] input/keyboard: fix dangling pointers

[Dmitry Torokhov]

Hi Wolfram,

On Sat, Mar 20, 2010 at 03:12:47PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 

I am not sure if setting clientdata to NULL before freeing the data is
that important; we really want to be sure that we don't leave clientdata
dangling when we finish unbinding the driver. If there are another
thread the change will not really help the problem of accessing
non-existing client data.

I will apply lm8323 portion of the patch but leave qt2160 as is.

Thanks.

-- 
Dmitry

Re: [PATCH 06/24] input/keyboard: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

Hello Dmitry,

On Sat, Mar 20, 2010 at 12:20:07PM -0700, Dmitry Torokhov wrote:
> Hi Wolfram,
> 
> On Sat, Mar 20, 2010 at 03:12:47PM +0100, Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> > 
> 
> I am not sure if setting clientdata to NULL before freeing the data is
> that important; we really want to be sure that we don't leave clientdata
> dangling when we finish unbinding the driver. If there are another
> thread the change will not really help the problem of accessing
> non-existing client data.

I agree it won't matter in practice. Jean and I concluded it is better
style, though (http://comments.gmane.org/gmane.linux.drivers.i2c/5392).
Still, I will not insist and leave it up the subsystem maintainers, of
course. I also won't check for that case again in the future.

> I will apply lm8323 portion of the patch but leave qt2160 as is.

Thanks!

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 06/24] input/keyboard: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

Hi Dmitry,

> On Sat, Mar 20, 2010 at 03:12:47PM +0100, Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> > 
> 
> I am not sure if setting clientdata to NULL before freeing the data is
> that important; we really want to be sure that we don't leave clientdata
> dangling when we finish unbinding the driver. If there are another
> thread the change will not really help the problem of accessing
> non-existing client data.
> 
> I will apply lm8323 portion of the patch but leave qt2160 as is.

We reached an agreement that a) setting the clientdata-pointer to NULL should
be done in the i2c-core [1] and b) how to do it. Based on that, I will do the
modification of the i2c-core for 2.6.34 (as it fixes the dangling pointers) and
then create one single patch removing the then superflous calls to
i2c_set_clientdata for 2.6.35 (as it is a cleanup). As you already applied the
above patch to your branch: you don't have to revert, we will fix it for you in
the next round.

Sorry for the detour!

Kind regards,

   Wolfram

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/5674/focus=5729

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 07/24] input/touchscreen: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/input/touchscreen/mcs5000_ts.c |    2 +-
 drivers/input/touchscreen/tsc2007.c    |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/mcs5000_ts.c b/drivers/input/touchscreen/mcs5000_ts.c
index 4c28b89..98689a7 100644
--- a/drivers/input/touchscreen/mcs5000_ts.c
+++ b/drivers/input/touchscreen/mcs5000_ts.c
@@ -254,8 +254,8 @@ static int __devexit mcs5000_ts_remove(struct i2c_client *client)
 
 	free_irq(client->irq, data);
 	input_unregister_device(data->input_dev);
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 
 	return 0;
 }
diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index be23780..4c63fda 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -353,6 +353,7 @@ static int __devexit tsc2007_remove(struct i2c_client *client)
 		pdata->exit_platform_hw();
 
 	input_unregister_device(ts->input);
+	i2c_set_clientdata(client, NULL);
 	kfree(ts);
 
 	return 0;
-- 
1.7.0