[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Pavel Herrmann]

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
per-call spi_message on stack to fix this

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/hwmon/max1111.c |   86 +++++++++++++----------------------------------
 1 files changed, 24 insertions(+), 62 deletions(-)

diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
index 12a54aa..6422baf 100644
--- a/drivers/hwmon/max1111.c
+++ b/drivers/hwmon/max1111.c
@@ -22,9 +22,6 @@
 #include <linux/spi/spi.h>
 #include <linux/slab.h>
 
-#define MAX1111_TX_BUF_SIZE	1
-#define MAX1111_RX_BUF_SIZE	2
-
 /* MAX1111 Commands */
 #define MAX1111_CTRL_PD0      (1u << 0)
 #define MAX1111_CTRL_PD1      (1u << 1)
@@ -36,35 +33,41 @@
 struct max1111_data {
 	struct spi_device	*spi;
 	struct device		*hwmon_dev;
-	struct spi_message	msg;
-	struct spi_transfer	xfer[2];
-	uint8_t *tx_buf;
-	uint8_t *rx_buf;
 };
 
 static int max1111_read(struct device *dev, int channel)
 {
-	struct max1111_data *data = dev_get_drvdata(dev);
-	uint8_t v1, v2;
 	int err;
-
-	data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) |
-		MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
-		MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR;
-
-	err = spi_sync(data->spi, &data->msg);
+	struct max1111_data *data = dev_get_drvdata(dev);
+	struct spi_message m;
+	struct spi_transfer t[2];
+	uint8_t rx_buf[2] = {0, 0};
+	uint8_t tx_buf = (channel << MAX1111_CTRL_SEL_SH) |
+			MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
+			MAX1111_CTRL_SGL | MAX1111_CTRL_UNI |
+			MAX1111_CTRL_STR;
+
+	spi_message_init(&m);
+	memset(t, 0, sizeof(t));
+
+	t[0].tx_buf = &tx_buf;
+	t[0].len = 1;
+	spi_message_add_tail(&t[0], &m);
+
+	t[1].rx_buf = rx_buf;
+	t[1].len = 2;
+	spi_message_add_tail(&t[1], &m);
+
+	err = spi_sync(data->spi, &m);
 	if (err < 0) {
 		dev_err(dev, "spi_sync failed with %d\n", err);
 		return err;
 	}
 
-	v1 = data->rx_buf[0];
-	v2 = data->rx_buf[1];
-
-	if ((v1 & 0xc0) || (v2 & 0x3f))
+	if ((rx_buf[0] & 0xc0) || (rx_buf[1] & 0x3f))
 		return -EINVAL;
 
-	return (v1 << 2) | (v2 >> 6);
+	return (rx_buf[0] << 2) | (rx_buf[1] >> 6);
 }
 
 #ifdef CONFIG_SHARPSL_PM
@@ -123,38 +126,6 @@ static const struct attribute_group max1111_attr_group = {
 	.attrs	= max1111_attributes,
 };
 
-static int setup_transfer(struct max1111_data *data)
-{
-	struct spi_message *m;
-	struct spi_transfer *x;
-
-	data->tx_buf = kmalloc(MAX1111_TX_BUF_SIZE, GFP_KERNEL);
-	if (!data->tx_buf)
-		return -ENOMEM;
-
-	data->rx_buf = kmalloc(MAX1111_RX_BUF_SIZE, GFP_KERNEL);
-	if (!data->rx_buf) {
-		kfree(data->tx_buf);
-		return -ENOMEM;
-	}
-
-	m = &data->msg;
-	x = &data->xfer[0];
-
-	spi_message_init(m);
-
-	x->tx_buf = &data->tx_buf[0];
-	x->len = 1;
-	spi_message_add_tail(x, m);
-
-	x++;
-	x->rx_buf = &data->rx_buf[0];
-	x->len = 2;
-	spi_message_add_tail(x, m);
-
-	return 0;
-}
-
 static int __devinit max1111_probe(struct spi_device *spi)
 {
 	struct max1111_data *data;
@@ -172,17 +143,13 @@ static int __devinit max1111_probe(struct spi_device *spi)
 		return -ENOMEM;
 	}
 
-	err = setup_transfer(data);
-	if (err)
-		goto err_free_data;
-
 	data->spi = spi;
 	spi_set_drvdata(spi, data);
 
 	err = sysfs_create_group(&spi->dev.kobj, &max1111_attr_group);
 	if (err) {
 		dev_err(&spi->dev, "failed to create attribute group\n");
-		goto err_free_all;
+		goto err_free_data;
 	}
 
 	data->hwmon_dev = hwmon_device_register(&spi->dev);
@@ -199,9 +166,6 @@ static int __devinit max1111_probe(struct spi_device *spi)
 
 err_remove:
 	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
-err_free_all:
-	kfree(data->rx_buf);
-	kfree(data->tx_buf);
 err_free_data:
 	kfree(data);
 	return err;
@@ -213,8 +177,6 @@ static int __devexit max1111_remove(struct spi_device *spi)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
-	kfree(data->rx_buf);
-	kfree(data->tx_buf);
 	kfree(data);
 	return 0;
 }
-- 
1.7.5.rc3

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Eric Miao]

On Wed, May 18, 2011 at 11:18 PM, Pavel Herrmann
<morpheus.ibis@gmail.com> 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
> per-call spi_message on stack to fix this
>
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>

OK

> ---
> ?drivers/hwmon/max1111.c | ? 86 +++++++++++++----------------------------------
> ?1 files changed, 24 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
> index 12a54aa..6422baf 100644
> --- a/drivers/hwmon/max1111.c
> +++ b/drivers/hwmon/max1111.c
> @@ -22,9 +22,6 @@
> ?#include <linux/spi/spi.h>
> ?#include <linux/slab.h>
>
> -#define MAX1111_TX_BUF_SIZE ? ?1
> -#define MAX1111_RX_BUF_SIZE ? ?2
> -
> ?/* MAX1111 Commands */
> ?#define MAX1111_CTRL_PD0 ? ? ?(1u << 0)
> ?#define MAX1111_CTRL_PD1 ? ? ?(1u << 1)
> @@ -36,35 +33,41 @@
> ?struct max1111_data {
> ? ? ? ?struct spi_device ? ? ? *spi;
> ? ? ? ?struct device ? ? ? ? ? *hwmon_dev;
> - ? ? ? struct spi_message ? ? ?msg;
> - ? ? ? struct spi_transfer ? ? xfer[2];
> - ? ? ? uint8_t *tx_buf;
> - ? ? ? uint8_t *rx_buf;
> ?};
>
> ?static int max1111_read(struct device *dev, int channel)
> ?{
> - ? ? ? struct max1111_data *data = dev_get_drvdata(dev);
> - ? ? ? uint8_t v1, v2;
> ? ? ? ?int err;
> -
> - ? ? ? data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) |
> - ? ? ? ? ? ? ? MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
> - ? ? ? ? ? ? ? MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR;
> -
> - ? ? ? err = spi_sync(data->spi, &data->msg);
> + ? ? ? struct max1111_data *data = dev_get_drvdata(dev);
> + ? ? ? struct spi_message m;
> + ? ? ? struct spi_transfer t[2];
> + ? ? ? uint8_t rx_buf[2] = {0, 0};
> + ? ? ? uint8_t tx_buf = (channel << MAX1111_CTRL_SEL_SH) |
> + ? ? ? ? ? ? ? ? ? ? ? MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
> + ? ? ? ? ? ? ? ? ? ? ? MAX1111_CTRL_SGL | MAX1111_CTRL_UNI |
> + ? ? ? ? ? ? ? ? ? ? ? MAX1111_CTRL_STR;
> +
> + ? ? ? spi_message_init(&m);
> + ? ? ? memset(t, 0, sizeof(t));
> +
> + ? ? ? t[0].tx_buf = &tx_buf;
> + ? ? ? t[0].len = 1;
> + ? ? ? spi_message_add_tail(&t[0], &m);
> +
> + ? ? ? t[1].rx_buf = rx_buf;
> + ? ? ? t[1].len = 2;
> + ? ? ? spi_message_add_tail(&t[1], &m);
> +
> + ? ? ? err = spi_sync(data->spi, &m);
> ? ? ? ?if (err < 0) {
> ? ? ? ? ? ? ? ?dev_err(dev, "spi_sync failed with %d\n", err);
> ? ? ? ? ? ? ? ?return err;
> ? ? ? ?}
>
> - ? ? ? v1 = data->rx_buf[0];
> - ? ? ? v2 = data->rx_buf[1];
> -
> - ? ? ? if ((v1 & 0xc0) || (v2 & 0x3f))
> + ? ? ? if ((rx_buf[0] & 0xc0) || (rx_buf[1] & 0x3f))
> ? ? ? ? ? ? ? ?return -EINVAL;
>
> - ? ? ? return (v1 << 2) | (v2 >> 6);
> + ? ? ? return (rx_buf[0] << 2) | (rx_buf[1] >> 6);
> ?}
>
> ?#ifdef CONFIG_SHARPSL_PM
> @@ -123,38 +126,6 @@ static const struct attribute_group max1111_attr_group = {
> ? ? ? ?.attrs ?= max1111_attributes,
> ?};
>
> -static int setup_transfer(struct max1111_data *data)
> -{
> - ? ? ? struct spi_message *m;
> - ? ? ? struct spi_transfer *x;
> -
> - ? ? ? data->tx_buf = kmalloc(MAX1111_TX_BUF_SIZE, GFP_KERNEL);
> - ? ? ? if (!data->tx_buf)
> - ? ? ? ? ? ? ? return -ENOMEM;
> -
> - ? ? ? data->rx_buf = kmalloc(MAX1111_RX_BUF_SIZE, GFP_KERNEL);
> - ? ? ? if (!data->rx_buf) {
> - ? ? ? ? ? ? ? kfree(data->tx_buf);
> - ? ? ? ? ? ? ? return -ENOMEM;
> - ? ? ? }
> -
> - ? ? ? m = &data->msg;
> - ? ? ? x = &data->xfer[0];
> -
> - ? ? ? spi_message_init(m);
> -
> - ? ? ? x->tx_buf = &data->tx_buf[0];
> - ? ? ? x->len = 1;
> - ? ? ? spi_message_add_tail(x, m);
> -
> - ? ? ? x++;
> - ? ? ? x->rx_buf = &data->rx_buf[0];
> - ? ? ? x->len = 2;
> - ? ? ? spi_message_add_tail(x, m);
> -
> - ? ? ? return 0;
> -}
> -
> ?static int __devinit max1111_probe(struct spi_device *spi)
> ?{
> ? ? ? ?struct max1111_data *data;
> @@ -172,17 +143,13 @@ static int __devinit max1111_probe(struct spi_device *spi)
> ? ? ? ? ? ? ? ?return -ENOMEM;
> ? ? ? ?}
>
> - ? ? ? err = setup_transfer(data);
> - ? ? ? if (err)
> - ? ? ? ? ? ? ? goto err_free_data;
> -
> ? ? ? ?data->spi = spi;
> ? ? ? ?spi_set_drvdata(spi, data);
>
> ? ? ? ?err = sysfs_create_group(&spi->dev.kobj, &max1111_attr_group);
> ? ? ? ?if (err) {
> ? ? ? ? ? ? ? ?dev_err(&spi->dev, "failed to create attribute group\n");
> - ? ? ? ? ? ? ? goto err_free_all;
> + ? ? ? ? ? ? ? goto err_free_data;
> ? ? ? ?}
>
> ? ? ? ?data->hwmon_dev = hwmon_device_register(&spi->dev);
> @@ -199,9 +166,6 @@ static int __devinit max1111_probe(struct spi_device *spi)
>
> ?err_remove:
> ? ? ? ?sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
> -err_free_all:
> - ? ? ? kfree(data->rx_buf);
> - ? ? ? kfree(data->tx_buf);
> ?err_free_data:
> ? ? ? ?kfree(data);
> ? ? ? ?return err;
> @@ -213,8 +177,6 @@ static int __devexit max1111_remove(struct spi_device *spi)
>
> ? ? ? ?hwmon_device_unregister(data->hwmon_dev);
> ? ? ? ?sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
> - ? ? ? kfree(data->rx_buf);
> - ? ? ? kfree(data->tx_buf);
> ? ? ? ?kfree(data);
> ? ? ? ?return 0;
> ?}
> --
> 1.7.5.rc3
>
>

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Russell King - ARM Linux]

On Wed, May 18, 2011 at 05:18:38PM +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
> per-call spi_message on stack to fix this

I assume this has not been tested with DMA debugging enabled.

The DMA API does not like mapping memory from the stack, which is what
you're potentially doing with this:

> +	uint8_t rx_buf[2] = {0, 0};
> +	uint8_t tx_buf = (channel << MAX1111_CTRL_SEL_SH) |
> +			MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
> +			MAX1111_CTRL_SGL | MAX1111_CTRL_UNI |
> +			MAX1111_CTRL_STR;
> +
> +	spi_message_init(&m);
> +	memset(t, 0, sizeof(t));
> +
> +	t[0].tx_buf = &tx_buf;
> +	t[0].len = 1;
> +	spi_message_add_tail(&t[0], &m);
> +
> +	t[1].rx_buf = rx_buf;
> +	t[1].len = 2;
> +	spi_message_add_tail(&t[1], &m);

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Marek Vasut]

> On Wed, May 18, 2011 at 05:18:38PM +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 per-call spi_message on stack to fix this
> 
> I assume this has not been tested with DMA debugging enabled.
> 
> The DMA API does not like mapping memory from the stack, which is what
> you're potentially doing with this:

Yikes, good catch, but kmallocing this and kfreeing it again is not something I'd like to see either.

What other options do you suggest?

Btw note, this isn't the only driver doing this, maybe we have a horde of patches on the way?

> 
> > +??? uint8_t rx_buf[2] = {0, 0};
> > +??? uint8_t tx_buf = (channel << MAX1111_CTRL_SEL_SH) |
> > +??? ??? ??? MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
> > +??? ??? ??? MAX1111_CTRL_SGL | MAX1111_CTRL_UNI |
> > +??? ??? ??? MAX1111_CTRL_STR;
> > +
> > +??? spi_message_init(&m);
> > +??? memset(t, 0, sizeof(t));
> > +
> > +??? t[0].tx_buf = &tx_buf;
> > +??? t[0].len = 1;
> > +??? spi_message_add_tail(&t[0], &m);
> > +
> > +??? t[1].rx_buf = rx_buf;
> > +??? t[1].len = 2;
> > +??? spi_message_add_tail(&t[1], &m);
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Cyril Hrubis]

Hi!
I've applied this patch over 2.6.39-rc3 and did couple of suspends. After about
ten of them I've got attached trace (instead of the usuall NULL pointer
dereference in complete()).

And yes, the MMC is still broken after this change it seems that there are more
bugs in zaurus SPI drivers.

-- 
metan
-------------- next part --------------
PM: Syncing filesystems ... done.
mmc0: card 0002 removed
Freezing user space processes ... (elapsed 0.03 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.07 seconds) done.
Suspending console(s) (use no_console_suspend to debug)
------------[ cut here ]------------
WARNING: at lib/kref.c:34 kref_get+0x20/0x44()
Modules linked in:
---[ end trace 435fc8b0048da5e9 ]---
sd 0:0:0:0: [sda] Stopping disk
------------[ cut here ]------------
WARNING: at lib/kref.c:34 kref_get+0x20/0x44()
Modules linked in:
---[ end trace 435fc8b0048da5ea ]---
PM: suspend of devices complete after 81.489 msecs
------------[ cut here ]------------
WARNING: at lib/kref.c:34 kref_get+0x20/0x44()
Modules linked in:
---[ end trace 435fc8b0048da5eb ]---
PM: late suspend of devices complete after 1.993 msecs
------------[ cut here ]------------
WARNING: at lib/kref.c:34 kref_get+0x20/0x44()
Modules linked in:
---[ end trace 435fc8b0048da5ec ]---
PM: early resume of devices complete after 728.029 msecs
------------[ cut here ]------------
WARNING: at lib/kref.c:34 kref_get+0x20/0x44()
Modules linked in:
---[ end trace 435fc8b0048da5ed ]---
sd 0:0:0:0: [sda] Starting disk
PM: resume of devices complete after 232.192 msecs
Restarting tasks ... done.
mmc0: error -95 whilst initialising SD card
PM: Syncing filesystems ... done.

haruka-chan login: root
kernel BUG at kernel/cred.c:171!
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = c399c000
[00000000] *pgd=a39ac831, *pte=00000000, *ppte=00000000
Internal error: Oops: 817 [#1] PREEMPT
last sysfs file: /sys/devices/platform/pxa2xx-spi.2/spi2.1/backlight/corgi_bl/brightness
Modules linked in:
CPU: 0    Tainted: G        W    (2.6.39-rc7-00259-g09c1ce4 #14)
PC is at __bug+0x18/0x24
LR is at __bug+0x14/0x24
pc : [<c00258bc>]    lr : [<c00258b8>]    psr: 40000013
sp : c3bc5f78  ip : 00000000  fp : bef2ea2c
r10: 00020000  r9 : c3bc4000  r8 : fffffffe
r7 : ffffff9c  r6 : c38d2d80  r5 : 00000000  r4 : c38d2d80
r3 : 00000000  r2 : c3bc5f6c  r1 : c02d6e9b  r0 : 00000027
Flags: nZcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0000397f  Table: a399c000  DAC: 00000015
Process login (pid: 1105, stack limit = 0xc3bc4278)
Stack: (0xc3bc5f78 to 0xc3bc6000)
5f60:                                                       c38d2d80 c0054c2c
5f80: c38d2d80 c009c0e0 ffffff9c 400e6c14 00000000 00000001 00000000 00000021
5fa0: c0023184 c0023000 00000000 00000001 400e6c14 00000000 400f0f18 00000000
5fc0: 00000000 00000001 00000000 00000021 400f1000 400f1000 fffffdc8 bef2ea2c
5fe0: 00000000 bef2e9b8 400e0ed8 400e247c 60000010 400e6c14 00000000 00000000
Code: e1a01000 e59f000c eb08ff53 e3a03000 (e5833000)
Unable to handle kernel NULL pointer dereference at virtual address 00000002
pgd = c397c000
[00000002] *pgd=a3b71831, *pte=00000000, *ppte=00000000
Internal error: Oops: f3 [#2] PREEMPT
last sysfs file: /sys/devices/platform/pxa2xx-spi.2/spi2.1/backlight/corgi_bl/brightness
Modules linked in:
CPU: 0    Tainted: G      D W    (2.6.39-rc7-00259-g09c1ce4 #14)
PC is at kmem_cache_alloc+0x2c/0x78
LR is at scsi_pool_alloc_command+0x38/0x60
pc : [<c0099904>]    lr : [<c0172ed8>]    psr: 20000093
sp : c388bd88  ip : 00000000  fp : ec300000
r10: c38f0000  r9 : c38bd0b8  r8 : 00001000
r7 : 00000002  r6 : 00000020  r5 : a0000093  r4 : c3802100
r3 : c03c8018  r2 : 00000000  r1 : 00000020  r0 : c3802100
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
Control: 0000397f  Table: a397c000  DAC: 00000017
Process sync_supers (pid: 139, stack limit = 0xc388a278)
Stack: (0xc388bd88 to 0xc388c000)
bd80:                   c0329330 c380fdc0 00000020 00000020 00001000 c0172ed8
bda0: c38bd000 00000020 c38bc800 c0172f38 c38bd000 c38bc800 c38bd0b8 c0173010
bdc0: c38bd000 c38bd0b8 c38bd0b8 c01730e4 00000000 c38f4a90 c38bd000 0000003f
bde0: 00001000 c0178834 c38f4a90 c38bd000 c393c340 c0181918 c380fd20 c38f80a4
be00: 00001000 c38f9638 c393c340 00000001 c38f4a90 c38f4ad8 00000007 c0117d64
be20: c38f4a90 c38f4a90 c38f4a90 c393c340 00000000 c393c340 c38bd0b8 00000008
be40: ec300000 c01197f8 c38bd000 c38bc800 c38bd024 c388a000 c393c340 c0177e44
be60: c393c340 c393c498 00012a00 c393c340 c3b1fc00 00000003 00000091 c393c340
be80: 00000001 00000008 ec300000 c0118060 c393c340 c011ad38 c393c340 c38f4a90
bea0: c3b1fc00 c388a000 c388beb8 00000000 c393c340 c0119150 c3896b00 c388a000
bec0: c388beec c0265b20 c388a000 c3854aa0 00011210 c388bef4 00000000 00000000
bee0: c3896b00 c3b1fc00 00000008 00000001 00000001 c0322fdc 00000000 00000000
bf00: 00000000 c01192d8 c388bf40 c3408000 c3854a60 00000091 00000010 00000001
bf20: 00000000 c00c85fc c3b1fc00 c00c86b4 00000010 0000000f c3408000 c3408000
bf40: c3b1fc00 00000091 00000001 c0322fdc 00000000 c00c3540 c3408000 00000091
bf60: 00000002 c00c6300 c38e6c00 c388a000 c3ffc400 c00f2750 00000001 c3ffc400
bf80: c38e6c00 c38e6c40 c0322fdc c00f27d8 c38e6c00 c388a000 00000000 c009efec
bfa0: c388a000 00000001 c388bfd4 c008103c 00000000 c0081070 c3831f78 00000000
bfc0: c388bfd4 c004f380 c0023890 00000000 00000000 00000000 c388bfd8 c388bfd8
bfe0: 00000000 c3831f78 c004f2fc c0023890 00000013 c0023890 0000074e 00000000
Code: e5903000 e5937000 e3570000 15902014 (17972002)
---[ end trace 435fc8b0048da5ee ]---
note: sync_supers[139] exited with preempt_count 1
Unable to handle kernel NULL pointer dereference at virtual address 00000002
pgd = c3b80000
[00000002] *pgd=a3b05831, *pte=00000000, *ppte=00000000
Internal error: Oops: f3 [#3] PREEMPT
last sysfs file: /sys/devices/platform/pxa2xx-spi.2/spi2.1/backlight/corgi_bl/brightness
Modules linked in:
CPU: 0    Tainted: G      D W    (2.6.39-rc7-00259-g09c1ce4 #14)
PC is at kmem_cache_alloc+0x2c/0x78
LR is at prepare_creds+0x24/0x8c
pc : [<c0099904>]    lr : [<c0054af4>]    psr: 20000093
sp : c3b39f60  ip : 0000397f  fp : bed7dfc4
r10: 00020000  r9 : c3b38000  r8 : 401ea280
r7 : 00000002  r6 : 000000d0  r5 : 40000013  r4 : c3802100
r3 : c03c8018  r2 : 00000000  r1 : 000000d0  r0 : c3802100
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0000397f  Table: a3b80000  DAC: 00000015
Process syslogd (pid: 940, stack limit = 0xc3b38278)
Stack: (0xc3b39f60 to 0xc3b3a000)
9f60: 401fa000 c389fb80 000004b0 ffffff9c 401ea280 c0054af4 401fa000 00000000
9f80: 000004b0 c009bf90 ffffff9c 401ea280 401fa000 000002c8 000004b0 00000021
9fa0: c0023184 c0023000 401fa000 000002c8 401ea280 00000000 00000000 0000000d
9fc0: 401fa000 000002c8 000004b0 00000021 ffff0270 bed7df08 401fa4b0 bed7dfc4
9fe0: 401ea280 bed7d6c8 401cc30c 40185c7c 60000010 401ea280 00000000 00000000
Code: e5903000 e5937000 e3570000 15902014 (17972002)
---[ end trace 435fc8b0048da5ef ]---
Unable to handle kernel NULL pointer dereference at virtual address 00000002
pgd = c3918000
[00000002] *pgd=a391d831, *pte=00000000, *ppte=00000000
Internal error: Oops: f3 [#4] PREEMPT
last sysfs file: /sys/devices/platform/pxa2xx-spi.2/spi2.1/backlight/corgi_bl/brightness
Modules linked in:
CPU: 0    Tainted: G      D W    (2.6.39-rc7-00259-g09c1ce4 #14)
PC is at kmem_cache_alloc+0x2c/0x78
LR is at prepare_creds+0x24/0x8c
pc : [<c0099904>]    lr : [<c0054af4>]    psr: 20000093
sp : c3831f60  ip : 0000397f  fp : 0000e704
r10: 00020000  r9 : c3830000  r8 : 0000e708
r7 : 00000002  r6 : 000000d0  r5 : 40000013  r4 : c3802100
r3 : c03c8018  r2 : 00000000  r1 : 000000d0  r0 : c3802100
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0000397f  Table: a3918000  DAC: 00000015
Process init (pid: 1, stack limit = 0xc3830278)
Stack: (0xc3831f60 to 0xc3832000)
1f60: 00000008 c382e000 00000451 ffffff9c 0000e708 c0054af4 00000008 00000002
1f80: 00000451 c009bf90 ffffff9c 0000e708 00000008 0000e5c4 00000451 00000021
1fa0: c0023184 c0023000 00000008 0000e5c4 0000e708 00000002 00000451 00000008
1fc0: 00000008 0000e5c4 00000451 00000021 00000000 beff18c0 00000008 0000e704
1fe0: 00019724 beff1588 0000dc30 402fec7c 20000010 0000e708 00000000 00000000
Code: e5903000 e5937000 e3570000 15902014 (17972002)
---[ end trace 435fc8b0048da5f0 ]---
Kernel panic - not syncing: Attempted to kill init!

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Russell King - ARM Linux]

On Wed, May 18, 2011 at 07:36:54PM +0200, Marek Vasut wrote:
> > On Wed, May 18, 2011 at 05:18:38PM +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 per-call spi_message on stack to fix this
> > 
> > I assume this has not been tested with DMA debugging enabled.
> > 
> > The DMA API does not like mapping memory from the stack, which is what
> > you're potentially doing with this:
> 
> Yikes, good catch, but kmallocing this and kfreeing it again is not
> something I'd like to see either.

You could use a semaphore to protect against other threads.

However, this driver just gives us yet more problems, as it overlaps
the DMA'd data with the DMA metadata (spi message/spi transfer
structures.)  And yes we do get bug reports on that too...

I think its about time driver and subsystem authors got a clue about
DMA incoherent architectures, and these things called 'cache lines'
which have a direct impact on whether code is buggy or not.  Sharing
cache lines between DMA buffers and other data is Really Bad News for
data integrity - even sharing a cache line between two DMA buffers
can be a problem.

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Pavel Machek]

Hi!

On Wed 2011-05-18 16:29:35, Russell King - ARM Linux wrote:
> On Wed, May 18, 2011 at 05:18:38PM +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
> > per-call spi_message on stack to fix this
> 
> I assume this has not been tested with DMA debugging enabled.
> 
> The DMA API does not like mapping memory from the stack, which is what
> you're potentially doing with this:

In some other mail, you said "just add the locking". Pavel H.
actually produced patch doing so...


From: Pavel Herrmann <morpheus.ibis@gmail.com>
To: Marek Vasut <marek.vasut@gmail.com>

>From 14063b017123233a8b56d6706a9ff046a791eaf4 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] Fix NULL pointer exception in max1111

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

diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
index 12a54aa..7be50e5 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)
@@ -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
+	 */
+	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;
 
@@ -138,6 +147,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 +163,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 +185,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 +228,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



----- End forwarded message -----


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Pavel Herrmann]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-NULL-pointer-exception-in-max1111.patch
Type: text/x-patch
Size: 2110 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110519/db197747/attachment.bin>

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Marek Vasut]

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

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Russell King - ARM Linux]

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.

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Pavel Herrmann]

On Thursday, May 19, 2011 09:31:21 PM Russell King - ARM Linux wrote:
> 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.

oh no, not again...
this was the earliest version, not the cleaned-up one (notice the lock in 
setup-transfer, which I said was unnecessary)
here is the cleaner (newest) version:


>From bd55d6b18fa4fcb884980825b43b43df01767149 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] MAX1111: Fix Race condition causing NULL pointer exception

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

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
 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;
 };
 
 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
+	 */
+	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);
 	kfree(data->rx_buf);
 	kfree(data->tx_buf);
 	kfree(data);
-- 
1.7.5.rc3

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Russell King - ARM Linux]

On Fri, May 20, 2011 at 12:13:20AM +0200, Pavel Herrmann wrote:
> From bd55d6b18fa4fcb884980825b43b43df01767149 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] MAX1111: Fix Race condition causing NULL pointer exception
> 
> 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
> 
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>

Looks good, thanks.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

> ---
>  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;
>  };
>  
>  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
> +	 */
> +	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);
>  	kfree(data->rx_buf);
>  	kfree(data->tx_buf);
>  	kfree(data);
> -- 
> 1.7.5.rc3
> 

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Pavel Machek]

Hi!

> > 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.
> 
> oh no, not again...
> this was the earliest version, not the cleaned-up one (notice the lock in 
> setup-transfer, which I said was unnecessary)
> here is the cleaner (newest) version:

Unfortunately that one does not apply, due to 

> @@ -213,6 +224,7 @@ static int __devexit max1111_remove(struct spi_device 
> *spi)

Whitespace damage. (but if you just delete this extra newline, its
fine).

Other than that, it seems to work. I was playing with max1111 a lot
today, and no oops.

ACK.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Pavel Herrmann]

On Saturday, May 21, 2011 10:28:07 PM Pavel Machek wrote:
> Unfortunately that one does not apply, due to
> 
> > @@ -213,6 +224,7 @@ static int __devexit max1111_remove(struct spi_device
> > *spi)
> 
> Whitespace damage. (but if you just delete this extra newline, its
> fine).

sorry for that one, I followed Mareks advice on sending the pach inline, and my mail 
client does automatic text reflow (defaults to 78 columns). I still have to find the 
correct formating settings to send these patches.

thanks for the ACK and testing

Pavel

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Marek Vasut]

On Saturday, May 21, 2011 10:45:02 PM Pavel Herrmann wrote:
> On Saturday, May 21, 2011 10:28:07 PM Pavel Machek wrote:
> > Unfortunately that one does not apply, due to
> > 
> > > @@ -213,6 +224,7 @@ static int __devexit max1111_remove(struct
> > > spi_device *spi)
> > 
> > Whitespace damage. (but if you just delete this extra newline, its
> > fine).
> 
> sorry for that one, I followed Mareks advice on sending the pach inline,
> and my mail client does automatic text reflow (defaults to 78 columns). I
> still have to find the correct formating settings to send these patches.
> 
> thanks for the ACK and testing
> 
> Pavel

Use git send-email, I recall telling you this ;-)

[PATCH] MAX1111: Fix race condition causing NULL pointer exception

[Marek Vasut]

On Wednesday, May 18, 2011 11:47:44 PM Cyril Hrubis wrote:
> Hi!
> I've applied this patch over 2.6.39-rc3 and did couple of suspends. After
> about ten of them I've got attached trace (instead of the usuall NULL
> pointer dereference in complete()).
> 
> And yes, the MMC is still broken after this change it seems that there are
> more bugs in zaurus SPI drivers.

Looks like corgi-bl, I think there was a patch for this I had, dunno if it was 
applied.

Eric, was that gpio_set_value_cansleep() for corgi-bl merged ?