[PATCH] w1: keep balance of mutex locks and refcnts

[PATCH] w1: keep balance of mutex locks and refcnts

[Alexey Khoroshilov]

w1_therm_eeprom() and w1_DS18B20_precision() decrement THERM_REFCNT
on error paths, while they did not increment it yet.

read_therm() unlocks bus mutex on some error paths,
while it is not acquired.

The patch makes sure all the functions keep the balance in usage of
the mutex and the THERM_REFCNT.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/w1/slaves/w1_therm.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 259525c3382a..fcd4d52e56e3 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -270,11 +270,11 @@ static inline int w1_therm_eeprom(struct device *device)
 
 	ret = mutex_lock_interruptible(&dev->bus_mutex);
 	if (ret != 0)
-		goto post_unlock;
+		return ret;
 
 	if (!sl->family_data) {
-		ret = -ENODEV;
-		goto pre_unlock;
+		mutex_unlock(&dev->bus_mutex);
+		return -ENODEV;
 	}
 
 	/* prevent the slave from going away in sleep */
@@ -326,7 +326,6 @@ static inline int w1_therm_eeprom(struct device *device)
 
 pre_unlock:
 	mutex_unlock(&dev->bus_mutex);
-
 post_unlock:
 	atomic_dec(THERM_REFCNT(family_data));
 	return ret;
@@ -350,16 +349,16 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
 
 	if (val > 12 || val < 9) {
 		pr_warn("Unsupported precision\n");
-		return -1;
+		return -EINVAL;
 	}
 
 	ret = mutex_lock_interruptible(&dev->bus_mutex);
 	if (ret != 0)
-		goto post_unlock;
+		return ret;
 
 	if (!sl->family_data) {
-		ret = -ENODEV;
-		goto pre_unlock;
+		mutex_unlock(&dev->bus_mutex);
+		return -ENODEV;
 	}
 
 	/* prevent the slave from going away in sleep */
@@ -411,10 +410,7 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
 		}
 	}
 
-pre_unlock:
 	mutex_unlock(&dev->bus_mutex);
-
-post_unlock:
 	atomic_dec(THERM_REFCNT(family_data));
 	return ret;
 }
@@ -492,11 +488,11 @@ static ssize_t read_therm(struct device *device,
 
 	ret = mutex_lock_interruptible(&dev->bus_mutex);
 	if (ret != 0)
-		goto error;
+		return ret;
 
 	if (!family_data) {
-		ret = -ENODEV;
-		goto mt_unlock;
+		mutex_unlock(&dev->bus_mutex);
+		return -ENODEV;
 	}
 
 	/* prevent the slave from going away in sleep */
@@ -532,17 +528,17 @@ static ssize_t read_therm(struct device *device,
 				sleep_rem = msleep_interruptible(tm);
 				if (sleep_rem != 0) {
 					ret = -EINTR;
-					goto dec_refcnt;
+					goto post_unlock;
 				}
 
 				ret = mutex_lock_interruptible(&dev->bus_mutex);
 				if (ret != 0)
-					goto dec_refcnt;
+					goto post_unlock;
 			} else if (!w1_strong_pullup) {
 				sleep_rem = msleep_interruptible(tm);
 				if (sleep_rem != 0) {
 					ret = -EINTR;
-					goto dec_refcnt;
+					goto pre_unlock;
 				}
 			}
 
@@ -567,11 +563,10 @@ static ssize_t read_therm(struct device *device,
 			break;
 	}
 
-dec_refcnt:
-	atomic_dec(THERM_REFCNT(family_data));
-mt_unlock:
+pre_unlock:
 	mutex_unlock(&dev->bus_mutex);
-error:
+post_unlock:
+	atomic_dec(THERM_REFCNT(family_data));
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH] w1: keep balance of mutex locks and refcnts

[Evgeniy Polyakov]

Hi Alex

29.09.2017, 23:23, "Alexey Khoroshilov" <khoroshilov@ispras.ru>:
> w1_therm_eeprom() and w1_DS18B20_precision() decrement THERM_REFCNT
> on error paths, while they did not increment it yet.
>
> read_therm() unlocks bus mutex on some error paths,
> while it is not acquired.
>
> The patch makes sure all the functions keep the balance in usage of
> the mutex and the THERM_REFCNT.
>
> Found by Linux Driver Verification project (linuxtesting.org).

Yes, this looks like a bug, thanks for finding it!

Please update your patch to use single exit point and not a mix of returns in the body of the function.

>          ret = mutex_lock_interruptible(&dev->bus_mutex);
>          if (ret != 0)
> - goto post_unlock;
> + return ret;
>
>          if (!sl->family_data) {
> - ret = -ENODEV;
> - goto pre_unlock;
> + mutex_unlock(&dev->bus_mutex);
> + return -ENODEV;
>          }

Re: [PATCH] w1: keep balance of mutex locks and refcnts

[Alexey Khoroshilov]

Hi Evgeniy,

mutex_lock() and atomic_inc() are not nested currently:

  ret = mutex_lock_interruptible(&dev->bus_mutex);
  ...
  atomic_inc(THERM_REFCNT(family_data));

  ...

  mutex_unlock(&dev->bus_mutex);
  ...
  atomic_dec(THERM_REFCNT(family_data));

As a result, error handling without returns will be still quite messy.

Is it possible to switch to a nested variant:
mutex_lock-atomic_inc-atomic_dec-mutex_unlock
or
atomic_inc-mutex_lock-mutex_unlock-atomic_dec
?

--
Alexey



On 01.10.2017 08:55, Evgeniy Polyakov wrote:
> Hi Alex
> 
> 29.09.2017, 23:23, "Alexey Khoroshilov" <khoroshilov@ispras.ru>:
>> w1_therm_eeprom() and w1_DS18B20_precision() decrement THERM_REFCNT
>> on error paths, while they did not increment it yet.
>>
>> read_therm() unlocks bus mutex on some error paths,
>> while it is not acquired.
>>
>> The patch makes sure all the functions keep the balance in usage of
>> the mutex and the THERM_REFCNT.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Yes, this looks like a bug, thanks for finding it!
> 
> Please update your patch to use single exit point and not a mix of returns in the body of the function.
> 
>>          ret = mutex_lock_interruptible(&dev->bus_mutex);
>>          if (ret != 0)
>> - goto post_unlock;
>> + return ret;
>>
>>          if (!sl->family_data) {
>> - ret = -ENODEV;
>> - goto pre_unlock;
>> + mutex_unlock(&dev->bus_mutex);
>> + return -ENODEV;
>>          }
> 

Re: [PATCH] w1: keep balance of mutex locks and refcnts

[Evgeniy Polyakov]

Hi Alexey

07.10.2017, 20:59, "Alexey Khoroshilov" <khoroshilov@ispras.ru>:
> Is it possible to switch to a nested variant:
> mutex_lock-atomic_inc-atomic_dec-mutex_unlock
> or
> atomic_inc-mutex_lock-mutex_unlock-atomic_dec
> ?

Yeah, you are right, it is a bit messy - we drop the lock while sleeping waiting for the bus master to complete operation,
and during this period family driver has to be referenced.

But we can easily grab the reference earlier and then try to lock the bus, so the second variant will work.

> --
> Alexey
>
> On 01.10.2017 08:55, Evgeniy Polyakov wrote:
>>  Hi Alex
>>
>>  29.09.2017, 23:23, "Alexey Khoroshilov" <khoroshilov@ispras.ru>:
>>>  w1_therm_eeprom() and w1_DS18B20_precision() decrement THERM_REFCNT
>>>  on error paths, while they did not increment it yet.
>>>
>>>  read_therm() unlocks bus mutex on some error paths,
>>>  while it is not acquired.
>>>
>>>  The patch makes sure all the functions keep the balance in usage of
>>>  the mutex and the THERM_REFCNT.
>>>
>>>  Found by Linux Driver Verification project (linuxtesting.org).
>>
>>  Yes, this looks like a bug, thanks for finding it!
>>
>>  Please update your patch to use single exit point and not a mix of returns in the body of the function.
>>
>>>           ret = mutex_lock_interruptible(&dev->bus_mutex);
>>>           if (ret != 0)
>>>  - goto post_unlock;
>>>  + return ret;
>>>
>>>           if (!sl->family_data) {
>>>  - ret = -ENODEV;
>>>  - goto pre_unlock;
>>>  + mutex_unlock(&dev->bus_mutex);
>>>  + return -ENODEV;
>>>           }

[PATCH v2] w1: keep balance of mutex locks and refcnts

[Alexey Khoroshilov]

w1_therm_eeprom() and w1_DS18B20_precision() decrement THERM_REFCNT
on error paths, while they did not increment it yet.

read_therm() unlocks bus mutex on some error paths,
while it is not acquired.

The patch makes sure all the functions keep the balance in usage of
the mutex and the THERM_REFCNT.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
v2: Implement suggestions of Evgeniy Polyakov.
Make use single exit point and not a mix of returns in the body of the function.
Switch to nested locking: grab the reference earlier and then try to lock the bus.

 drivers/w1/slaves/w1_therm.c | 59 +++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 259525c3382a..3c350dfbcd0b 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -268,17 +268,18 @@ static inline int w1_therm_eeprom(struct device *device)
 	int ret, max_trying = 10;
 	u8 *family_data = sl->family_data;
 
-	ret = mutex_lock_interruptible(&dev->bus_mutex);
-	if (ret != 0)
-		goto post_unlock;
-
 	if (!sl->family_data) {
 		ret = -ENODEV;
-		goto pre_unlock;
+		goto error;
 	}
 
 	/* prevent the slave from going away in sleep */
 	atomic_inc(THERM_REFCNT(family_data));
+
+	ret = mutex_lock_interruptible(&dev->bus_mutex);
+	if (ret != 0)
+		goto dec_refcnt;
+
 	memset(rom, 0, sizeof(rom));
 
 	while (max_trying--) {
@@ -306,17 +307,17 @@ static inline int w1_therm_eeprom(struct device *device)
 				sleep_rem = msleep_interruptible(tm);
 				if (sleep_rem != 0) {
 					ret = -EINTR;
-					goto post_unlock;
+					goto dec_refcnt;
 				}
 
 				ret = mutex_lock_interruptible(&dev->bus_mutex);
 				if (ret != 0)
-					goto post_unlock;
+					goto dec_refcnt;
 			} else if (!w1_strong_pullup) {
 				sleep_rem = msleep_interruptible(tm);
 				if (sleep_rem != 0) {
 					ret = -EINTR;
-					goto pre_unlock;
+					goto mt_unlock;
 				}
 			}
 
@@ -324,11 +325,11 @@ static inline int w1_therm_eeprom(struct device *device)
 		}
 	}
 
-pre_unlock:
+mt_unlock:
 	mutex_unlock(&dev->bus_mutex);
-
-post_unlock:
+dec_refcnt:
 	atomic_dec(THERM_REFCNT(family_data));
+error:
 	return ret;
 }
 
@@ -350,20 +351,22 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
 
 	if (val > 12 || val < 9) {
 		pr_warn("Unsupported precision\n");
-		return -1;
+		ret = -EINVAL;
+		goto error;
 	}
 
-	ret = mutex_lock_interruptible(&dev->bus_mutex);
-	if (ret != 0)
-		goto post_unlock;
-
 	if (!sl->family_data) {
 		ret = -ENODEV;
-		goto pre_unlock;
+		goto error;
 	}
 
 	/* prevent the slave from going away in sleep */
 	atomic_inc(THERM_REFCNT(family_data));
+
+	ret = mutex_lock_interruptible(&dev->bus_mutex);
+	if (ret != 0)
+		goto dec_refcnt;
+
 	memset(rom, 0, sizeof(rom));
 
 	/* translate precision to bitmask (see datasheet page 9) */
@@ -411,11 +414,10 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
 		}
 	}
 
-pre_unlock:
 	mutex_unlock(&dev->bus_mutex);
-
-post_unlock:
+dec_refcnt:
 	atomic_dec(THERM_REFCNT(family_data));
+error:
 	return ret;
 }
 
@@ -490,17 +492,18 @@ static ssize_t read_therm(struct device *device,
 	int ret, max_trying = 10;
 	u8 *family_data = sl->family_data;
 
-	ret = mutex_lock_interruptible(&dev->bus_mutex);
-	if (ret != 0)
-		goto error;
-
 	if (!family_data) {
 		ret = -ENODEV;
-		goto mt_unlock;
+		goto error;
 	}
 
 	/* prevent the slave from going away in sleep */
 	atomic_inc(THERM_REFCNT(family_data));
+
+	ret = mutex_lock_interruptible(&dev->bus_mutex);
+	if (ret != 0)
+		goto dec_refcnt;
+
 	memset(info->rom, 0, sizeof(info->rom));
 
 	while (max_trying--) {
@@ -542,7 +545,7 @@ static ssize_t read_therm(struct device *device,
 				sleep_rem = msleep_interruptible(tm);
 				if (sleep_rem != 0) {
 					ret = -EINTR;
-					goto dec_refcnt;
+					goto mt_unlock;
 				}
 			}
 
@@ -567,10 +570,10 @@ static ssize_t read_therm(struct device *device,
 			break;
 	}
 
-dec_refcnt:
-	atomic_dec(THERM_REFCNT(family_data));
 mt_unlock:
 	mutex_unlock(&dev->bus_mutex);
+dec_refcnt:
+	atomic_dec(THERM_REFCNT(family_data));
 error:
 	return ret;
 }
-- 
2.7.4