From: khali@linux-fr.org (Jean Delvare) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Date: Mon, 11 Jul 2011 22:11:48 +0200 [thread overview] Message-ID: <20110711221148.3a76d55b@endymion.delvare> (raw) In-Reply-To: <1310410051-13127-1-git-send-email-morpheus.ibis@gmail.com> Hi Pavel, On Mon, 11 Jul 2011 20:47:31 +0200, Pavel Herrmann wrote: > spi_sync call uses its spi_message parameter to keep completion information, > having this structure static is not thread-safe, potentially causing one > thread having pointers to memory on or above other threads stack. use mutex > to prevent multiple access This has nothing to do with static, as a matter of fact the structure is dynamically allocated. The bottom line is that the driver structure is such that calls to max1111_read() must be serialized. > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk> > Acked-by: Pavel Machek <pavel@ucw.cz> > Acked-by: Marek Vasut <marek.vasut@gmail.com> > Acked-by: Cyril Hrubis <metan@ucw.cz> > --- > drivers/hwmon/max1111.c | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c > index 12a54aa..d872f57 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; "lock" and "mutex" being kind of synonyms, this is a rather bad name. And you should add a comment explaining what is protected. As I understand it, it's more than just msg, it's also protecting xfer, tx_buf and rx_buf. > }; > > static int max1111_read(struct device *dev, int channel) > @@ -48,6 +49,11 @@ static int max1111_read(struct device *dev, int channel) > uint8_t v1, v2; > int err; > > + /* spi_sync requires data not to be freed before function returns > + * for static data, any access is dangerous, use locks > + */ This has nothing to do with "freeing data". max1111_read() doesn't free anything. It is making use of a data structure, the access to which must be serialized. Easy as that. And no, access isn't dangerous ;) > + mutex_lock(&data->msg_lock_mutex); > + > data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) | > MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 | > MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR; > @@ -55,12 +61,15 @@ static int max1111_read(struct device *dev, int channel) > err = spi_sync(data->spi, &data->msg); > if (err < 0) { > dev_err(dev, "spi_sync failed with %d\n", err); > + mutex_unlock(&data->msg_lock_mutex); > return err; > } > > v1 = data->rx_buf[0]; > v2 = data->rx_buf[1]; > > + mutex_unlock(&data->msg_lock_mutex); > + > if ((v1 & 0xc0) || (v2 & 0x3f)) > return -EINVAL; > > @@ -176,6 +185,8 @@ static int __devinit max1111_probe(struct spi_device *spi) > if (err) > goto err_free_data; > > + mutex_init(&data->msg_lock_mutex); > + > data->spi = spi; > spi_set_drvdata(spi, data); > > @@ -213,6 +224,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); I didn't know about this function, interesting. No other hwmon driver calls it, not sure if it makes sense when the underlying memory is going to be freed immediately afterwards anyway. But of course it can't hurt. > kfree(data->rx_buf); > kfree(data->tx_buf); > kfree(data); Unrelated to your patch, but these 3 separate allocs are really insane, memory usage in this driver could be much improved IMHO. Please respin your patch with a better struct member name and improved description and comments, and I'll be happy to apply it. Thanks, -- Jean Delvare
WARNING: multiple messages have this Message-ID (diff)
From: Jean Delvare <khali@linux-fr.org> To: linux-arm-kernel@lists.infradead.org Subject: Re: [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Date: Mon, 11 Jul 2011 20:11:48 +0000 [thread overview] Message-ID: <20110711221148.3a76d55b@endymion.delvare> (raw) In-Reply-To: <1310410051-13127-1-git-send-email-morpheus.ibis@gmail.com> Hi Pavel, On Mon, 11 Jul 2011 20:47:31 +0200, Pavel Herrmann wrote: > spi_sync call uses its spi_message parameter to keep completion information, > having this structure static is not thread-safe, potentially causing one > thread having pointers to memory on or above other threads stack. use mutex > to prevent multiple access This has nothing to do with static, as a matter of fact the structure is dynamically allocated. The bottom line is that the driver structure is such that calls to max1111_read() must be serialized. > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk> > Acked-by: Pavel Machek <pavel@ucw.cz> > Acked-by: Marek Vasut <marek.vasut@gmail.com> > Acked-by: Cyril Hrubis <metan@ucw.cz> > --- > drivers/hwmon/max1111.c | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c > index 12a54aa..d872f57 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; "lock" and "mutex" being kind of synonyms, this is a rather bad name. And you should add a comment explaining what is protected. As I understand it, it's more than just msg, it's also protecting xfer, tx_buf and rx_buf. > }; > > static int max1111_read(struct device *dev, int channel) > @@ -48,6 +49,11 @@ static int max1111_read(struct device *dev, int channel) > uint8_t v1, v2; > int err; > > + /* spi_sync requires data not to be freed before function returns > + * for static data, any access is dangerous, use locks > + */ This has nothing to do with "freeing data". max1111_read() doesn't free anything. It is making use of a data structure, the access to which must be serialized. Easy as that. And no, access isn't dangerous ;) > + mutex_lock(&data->msg_lock_mutex); > + > data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) | > MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 | > MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR; > @@ -55,12 +61,15 @@ static int max1111_read(struct device *dev, int channel) > err = spi_sync(data->spi, &data->msg); > if (err < 0) { > dev_err(dev, "spi_sync failed with %d\n", err); > + mutex_unlock(&data->msg_lock_mutex); > return err; > } > > v1 = data->rx_buf[0]; > v2 = data->rx_buf[1]; > > + mutex_unlock(&data->msg_lock_mutex); > + > if ((v1 & 0xc0) || (v2 & 0x3f)) > return -EINVAL; > > @@ -176,6 +185,8 @@ static int __devinit max1111_probe(struct spi_device *spi) > if (err) > goto err_free_data; > > + mutex_init(&data->msg_lock_mutex); > + > data->spi = spi; > spi_set_drvdata(spi, data); > > @@ -213,6 +224,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); I didn't know about this function, interesting. No other hwmon driver calls it, not sure if it makes sense when the underlying memory is going to be freed immediately afterwards anyway. But of course it can't hurt. > kfree(data->rx_buf); > kfree(data->tx_buf); > kfree(data); Unrelated to your patch, but these 3 separate allocs are really insane, memory usage in this driver could be much improved IMHO. Please respin your patch with a better struct member name and improved description and comments, and I'll be happy to apply it. Thanks, -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
next prev parent reply other threads:[~2011-07-11 20:11 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-07-11 18:47 [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-07-11 18:47 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing NULL Pavel Herrmann 2011-07-11 20:11 ` Jean Delvare [this message] 2011-07-11 20:11 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Jean Delvare 2011-07-11 20:36 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-07-11 20:36 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Pavel Herrmann 2011-07-11 21:03 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Guenter Roeck 2011-07-11 21:03 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Guenter Roeck 2011-07-11 21:49 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-07-11 21:49 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Pavel Herrmann 2011-07-12 6:48 ` [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Jean Delvare 2011-07-12 6:48 ` [lm-sensors] [PATCH v2] MAX1111: Fix Race condition causing Jean Delvare 2011-07-11 20:56 ` [Zaurus-devel] [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Stanislav Brabec 2011-07-11 20:56 ` [lm-sensors] [Zaurus-devel] [PATCH v2] MAX1111: Fix Race Stanislav Brabec -- strict thread matches above, loose matches on Subject: below -- 2011-06-03 20:00 [PATCH v2] MAX1111: Fix Race condition causing NULL pointer exception Pavel Herrmann 2011-06-29 20:11 ` Pavel Herrmann
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110711221148.3a76d55b@endymion.delvare \ --to=khali@linux-fr.org \ --cc=linux-arm-kernel@lists.infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.