diff mbox

MAX1111: Fix race condition causing NULL pointer exception

Message ID 201105191451.40416.morpheus.ibis@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Herrmann May 19, 2011, 12:51 p.m. UTC 
Hi

On Thursday, May 19, 2011 02:35:08 PM Pavel Machek wrote:
> > you're potentially doing with this:
> In some other mail, you said "just add the locking". Pavel H.
> actually produced patch doing so...

yes, that was the original version of the patch. while I agree with Marek on 
locks not being the right way, I was going send a cleaner version of the 
original locking patch today (locking in probe is not really necessary), you 
just beat me to it.

Pavel Herrmann

Comments

Marek Vasut May 19, 2011, 1:55 p.m. UTC | #1 
On Thursday, May 19, 2011 02:51:40 PM Pavel Herrmann wrote:
> Hi
> 
> On Thursday, May 19, 2011 02:35:08 PM Pavel Machek wrote:
> > > you're potentially doing with this:
> > In some other mail, you said "just add the locking". Pavel H.
> > actually produced patch doing so...
> 
> yes, that was the original version of the patch. while I agree with Marek
> on locks not being the right way, I was going send a cleaner version of
> the original locking patch today (locking in probe is not really
> necessary), you just beat me to it.

Please send the patch inline next time ;-)

> 
> Pavel Herrmann
Russell King - ARM Linux May 19, 2011, 7:31 p.m. UTC | #2 
On Thu, May 19, 2011 at 02:51:40PM +0200, Pavel Herrmann wrote:
> @@ -52,7 +53,14 @@ static int max1111_read(struct device *dev, int channel)
>  		MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
>  		MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR;
>  
> +	/* spi_sync requires data not to be freed before function returns
> +	 * for static data, any access is dangerous, use locks
> +	 */
> +	mutex_lock(&data->msg_lock_mutex);
> +
>  	err = spi_sync(data->spi, &data->msg);
> +
> +	mutex_unlock(&data->msg_lock_mutex);

I'm not sure that this is right.  Taking the lock around spi_sync() ensures
that two concurrent spi_sync()s can't happen in parallel, but with this you
could end up with another happening as soon as this lock is released -
before you've accessed the data which was transferred.

I think you want to hold the mutex at the point you setup the data to be
transferred, do the transfer, and then release the mutex once you've read
the results of the transfer.
diff mbox

Patch

From 5782df40e6c343b6dc9ff5ca008c1ee8b50cad51 Mon Sep 17 00:00:00 2001
From: Pavel Herrmann <morpheus.ibis@gmail.com>
Date: Mon, 16 May 2011 14:18:18 +0200
Subject: [PATCH 1/2] Fix NULL pointer exception in max1111

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
 drivers/hwmon/max1111.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
index 12a54aa..c748fa3 100644
--- a/drivers/hwmon/max1111.c
+++ b/drivers/hwmon/max1111.c
@@ -40,6 +40,7 @@  struct max1111_data {
 	struct spi_transfer	xfer[2];
 	uint8_t *tx_buf;
 	uint8_t *rx_buf;
+	struct mutex		msg_lock_mutex;
 };
 
 static int max1111_read(struct device *dev, int channel)
@@ -52,7 +53,14 @@  static int max1111_read(struct device *dev, int channel)
 		MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
 		MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR;
 
+	/* spi_sync requires data not to be freed before function returns
+	 * for static data, any access is dangerous, use locks
+	 */
+	mutex_lock(&data->msg_lock_mutex);
+
 	err = spi_sync(data->spi, &data->msg);
+
+	mutex_unlock(&data->msg_lock_mutex);
 	if (err < 0) {
 		dev_err(dev, "spi_sync failed with %d\n", err);
 		return err;
@@ -138,6 +146,8 @@  static int setup_transfer(struct max1111_data *data)
 		return -ENOMEM;
 	}
 
+	mutex_lock(&data->msg_lock_mutex);
+
 	m = &data->msg;
 	x = &data->xfer[0];
 
@@ -152,6 +162,8 @@  static int setup_transfer(struct max1111_data *data)
 	x->len = 2;
 	spi_message_add_tail(x, m);
 
+	mutex_unlock(&data->msg_lock_mutex);
+
 	return 0;
 }
 
@@ -172,6 +184,8 @@  static int __devinit max1111_probe(struct spi_device *spi)
 		return -ENOMEM;
 	}
 
+	mutex_init(&data->msg_lock_mutex);
+
 	err = setup_transfer(data);
 	if (err)
 		goto err_free_data;
@@ -213,6 +227,7 @@  static int __devexit max1111_remove(struct spi_device *spi)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
+	mutex_destroy(data->msg_lock_mutex);
 	kfree(data->rx_buf);
 	kfree(data->tx_buf);
 	kfree(data);
-- 
1.7.5.rc1