[PATCH 1/3] w1_therm, don't let the slave go away while in w1_slave_show

From: David Fries
Date: Sat Mar 07 2015 - 23:25:37 EST


A temperature conversion can take 750 ms to complete, if the sensor is
externally powered it releases the bus_mutex while it waits, but if
the slave device is removed sl becomes a dangling pointer.
The race condition window can be 10 * 750 ms = 7.5 seconds if the crc
check fails.

Reported-By: Thorsten Bschorr <thorsten@xxxxxxxxxx>
Signed-off-by: David Fries <David@xxxxxxxxx>
---
drivers/w1/slaves/w1_therm.c | 52 ++++++++++++++++++++++++++++++------------
1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 1f11a20..6b3ef93 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -59,9 +59,20 @@ MODULE_ALIAS("w1-family-" __stringify(W1_THERM_DS28EA00));
static int w1_strong_pullup = 1;
module_param_named(strong_pullup, w1_strong_pullup, int, 0);

+struct w1_therm_family_data {
+ uint8_t rom[9];
+ struct mutex lock;
+};
+
+/* return the address of the lock in the family data */
+#define THERM_LOCK(sl) \
+ (&((struct w1_therm_family_data*)sl->family_data)->lock)
+
static int w1_therm_add_slave(struct w1_slave *sl)
{
- sl->family_data = kzalloc(9, GFP_KERNEL);
+ sl->family_data = kzalloc(sizeof(struct w1_therm_family_data),
+ GFP_KERNEL);
+ mutex_init(THERM_LOCK(sl));
if (!sl->family_data)
return -ENOMEM;
return 0;
@@ -69,6 +80,11 @@ static int w1_therm_add_slave(struct w1_slave *sl)

static void w1_therm_remove_slave(struct w1_slave *sl)
{
+ /* Getting the lock means w1_slave_show isn't sleeping and the
+ * family_data can be freed.
+ */
+ mutex_lock(THERM_LOCK(sl));
+ mutex_unlock(THERM_LOCK(sl));
kfree(sl->family_data);
sl->family_data = NULL;
}
@@ -194,13 +210,15 @@ static ssize_t w1_slave_show(struct device *device,
struct w1_slave *sl = dev_to_w1_slave(device);
struct w1_master *dev = sl->master;
u8 rom[9], crc, verdict, external_power;
- int i, max_trying = 10;
+ int i, ret, max_trying = 10;
ssize_t c = PAGE_SIZE;

- i = mutex_lock_interruptible(&dev->bus_mutex);
- if (i != 0)
- return i;
+ ret = mutex_lock_interruptible(&dev->bus_mutex);
+ if (ret != 0)
+ goto post_unlock;

+ /* prevent the slave from going away in sleep */
+ mutex_lock(THERM_LOCK(sl));
memset(rom, 0, sizeof(rom));

while (max_trying--) {
@@ -229,18 +247,19 @@ static ssize_t w1_slave_show(struct device *device,
if (external_power) {
mutex_unlock(&dev->bus_mutex);

- sleep_rem = msleep_interruptible(tm);
- if (sleep_rem != 0)
- return -EINTR;
+ if (sleep_rem != 0) {
+ ret = -EINTR;
+ goto post_unlock;
+ }

- i = mutex_lock_interruptible(&dev->bus_mutex);
- if (i != 0)
- return i;
+ ret = mutex_lock_interruptible(&dev->bus_mutex);
+ if (ret != 0)
+ goto post_unlock;
} else if (!w1_strong_pullup) {
sleep_rem = msleep_interruptible(tm);
if (sleep_rem != 0) {
- mutex_unlock(&dev->bus_mutex);
- return -EINTR;
+ ret = -EINTR;
+ goto pre_unlock;
}
}

@@ -279,9 +298,14 @@ static ssize_t w1_slave_show(struct device *device,

c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n",
w1_convert_temp(rom, sl->family->fid));
+ ret = PAGE_SIZE - c;
+
+pre_unlock:
mutex_unlock(&dev->bus_mutex);

- return PAGE_SIZE - c;
+post_unlock:
+ mutex_unlock(THERM_LOCK(sl));
+ return ret;
}

static int __init w1_therm_init(void)
--
1.7.10.4