[PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[Alexey Khoroshilov]

w1_process_callbacks() expects to be called with dev->list_mutex held,
but it is the fact only in w1_process(). __w1_remove_master_device()
calls w1_process_callbacks() after it releases list_mutex.

The patch fixes __w1_remove_master_device() to acquire list_mutex
for w1_process_callbacks(). It is implemented by moving
w1_process_callbacks() functionality to __w1_process_callbacks()
and adding a wrapper with the old name.

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

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/w1/w1.c |  8 +++++---
 drivers/w1/w1.h | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index ff52618cafbe..e507c51512aa 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -1075,12 +1075,14 @@ static void w1_search_process(struct w1_master *dev, u8 search_type)
 }
 
 /**
- * w1_process_callbacks() - execute each dev->async_list callback entry
+ * __w1_process_callbacks() - execute each dev->async_list callback entry
  * @dev: w1_master device
  *
+ * The w1 master list_mutex must be held.
+ *
  * Return: 1 if there were commands to executed 0 otherwise
  */
-int w1_process_callbacks(struct w1_master *dev)
+int __w1_process_callbacks(struct w1_master *dev)
 {
 	int ret = 0;
 	struct w1_async_cmd *async_cmd, *async_n;
@@ -1122,7 +1124,7 @@ int w1_process(void *data)
 		/* Note, w1_process_callback drops the lock while processing,
 		 * but locks it again before returning.
 		 */
-		if (!w1_process_callbacks(dev) && jremain) {
+		if (!__w1_process_callbacks(dev) && jremain) {
 			/* a wake up is either to stop the thread, process
 			 * callbacks, or search, it isn't process callbacks, so
 			 * schedule a search.
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 734dab7fc687..e357321a5e13 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -341,9 +341,20 @@ extern int w1_max_slave_ttl;
 extern struct list_head w1_masters;
 extern struct mutex w1_mlock;
 
-extern int w1_process_callbacks(struct w1_master *dev);
+extern int __w1_process_callbacks(struct w1_master *dev);
 extern int w1_process(void *);
 
+static inline int w1_process_callbacks(struct w1_master *dev)
+{
+	int ret;
+
+	mutex_lock(&dev->list_mutex);
+	ret = __w1_process_callbacks(dev);
+	mutex_unlock(&dev->list_mutex);
+	return ret;
+}
+
+
 #endif /* __KERNEL__ */
 
 #endif /* __W1_H */
-- 
1.8.3.2

Re: [PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[Alexey Khoroshilov]

As far as it is 3.15-rc1 regression introduced by 9fcbbac5ded48,
it makes sense to fix it in 3.15 cycle.

--
Alexey

On 01.05.2014 00:37, Alexey Khoroshilov wrote:
> w1_process_callbacks() expects to be called with dev->list_mutex held,
> but it is the fact only in w1_process(). __w1_remove_master_device()
> calls w1_process_callbacks() after it releases list_mutex.
>
> The patch fixes __w1_remove_master_device() to acquire list_mutex
> for w1_process_callbacks(). It is implemented by moving
> w1_process_callbacks() functionality to __w1_process_callbacks()
> and adding a wrapper with the old name.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/w1/w1.c |  8 +++++---
>  drivers/w1/w1.h | 13 ++++++++++++-
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
> index ff52618cafbe..e507c51512aa 100644
> --- a/drivers/w1/w1.c
> +++ b/drivers/w1/w1.c
> @@ -1075,12 +1075,14 @@ static void w1_search_process(struct w1_master *dev, u8 search_type)
>  }
>  
>  /**
> - * w1_process_callbacks() - execute each dev->async_list callback entry
> + * __w1_process_callbacks() - execute each dev->async_list callback entry
>   * @dev: w1_master device
>   *
> + * The w1 master list_mutex must be held.
> + *
>   * Return: 1 if there were commands to executed 0 otherwise
>   */
> -int w1_process_callbacks(struct w1_master *dev)
> +int __w1_process_callbacks(struct w1_master *dev)
>  {
>  	int ret = 0;
>  	struct w1_async_cmd *async_cmd, *async_n;
> @@ -1122,7 +1124,7 @@ int w1_process(void *data)
>  		/* Note, w1_process_callback drops the lock while processing,
>  		 * but locks it again before returning.
>  		 */
> -		if (!w1_process_callbacks(dev) && jremain) {
> +		if (!__w1_process_callbacks(dev) && jremain) {
>  			/* a wake up is either to stop the thread, process
>  			 * callbacks, or search, it isn't process callbacks, so
>  			 * schedule a search.
> diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
> index 734dab7fc687..e357321a5e13 100644
> --- a/drivers/w1/w1.h
> +++ b/drivers/w1/w1.h
> @@ -341,9 +341,20 @@ extern int w1_max_slave_ttl;
>  extern struct list_head w1_masters;
>  extern struct mutex w1_mlock;
>  
> -extern int w1_process_callbacks(struct w1_master *dev);
> +extern int __w1_process_callbacks(struct w1_master *dev);
>  extern int w1_process(void *);
>  
> +static inline int w1_process_callbacks(struct w1_master *dev)
> +{
> +	int ret;
> +
> +	mutex_lock(&dev->list_mutex);
> +	ret = __w1_process_callbacks(dev);
> +	mutex_unlock(&dev->list_mutex);
> +	return ret;
> +}
> +
> +
>  #endif /* __KERNEL__ */
>  
>  #endif /* __W1_H */

Re: [PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[David Fries]

On Thu, May 01, 2014 at 12:37:58AM +0400, Alexey Khoroshilov wrote:
> w1_process_callbacks() expects to be called with dev->list_mutex held,
> but it is the fact only in w1_process(). __w1_remove_master_device()
> calls w1_process_callbacks() after it releases list_mutex.
> 
> The patch fixes __w1_remove_master_device() to acquire list_mutex
> for w1_process_callbacks(). It is implemented by moving
> w1_process_callbacks() functionality to __w1_process_callbacks()
> and adding a wrapper with the old name.

Good catch.  I guess as it was in the shutdown path it failed the
unlock silently.

I would prefer a different fix.  If w1_process_callbacks was more of a
public facing API called from multiple locations where the caller
doesn't have access to the locks, something like this would probably
be the way to go.  This is called from two areas of the code, and not
likely to be called from any new areas in the future.

Adding a documentation comment is good.  I would be tempted in
__w1_remove_master_device to move the dev->list_mutex down after the
while loop, but I can't be sure that whatever has a refcnt wouldn't
need list_mutex.  The last w1_process_callbacks after the while loop
shouldn't be needed I wouldn't think, I was just being defensive.  By
the time it completes the while loop the reference count is 0 so there
shouldn't be anything left for it to process and if there is, it's a
race condition and game over anyway.  So just sandwich
w1_process_callbacks with the lock/unlock in
__w1_remove_master_device please.

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

Was this found with code inspection or hardware testing with lock
debugging enabled?

> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/w1/w1.c |  8 +++++---
>  drivers/w1/w1.h | 13 ++++++++++++-
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
> index ff52618cafbe..e507c51512aa 100644
> --- a/drivers/w1/w1.c
> +++ b/drivers/w1/w1.c
> @@ -1075,12 +1075,14 @@ static void w1_search_process(struct w1_master *dev, u8 search_type)
>  }
>  
>  /**
> - * w1_process_callbacks() - execute each dev->async_list callback entry
> + * __w1_process_callbacks() - execute each dev->async_list callback entry
>   * @dev: w1_master device
>   *
> + * The w1 master list_mutex must be held.
> + *
>   * Return: 1 if there were commands to executed 0 otherwise
>   */
> -int w1_process_callbacks(struct w1_master *dev)
> +int __w1_process_callbacks(struct w1_master *dev)
>  {
>  	int ret = 0;
>  	struct w1_async_cmd *async_cmd, *async_n;
> @@ -1122,7 +1124,7 @@ int w1_process(void *data)
>  		/* Note, w1_process_callback drops the lock while processing,
>  		 * but locks it again before returning.
>  		 */
> -		if (!w1_process_callbacks(dev) && jremain) {
> +		if (!__w1_process_callbacks(dev) && jremain) {
>  			/* a wake up is either to stop the thread, process
>  			 * callbacks, or search, it isn't process callbacks, so
>  			 * schedule a search.
> diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
> index 734dab7fc687..e357321a5e13 100644
> --- a/drivers/w1/w1.h
> +++ b/drivers/w1/w1.h
> @@ -341,9 +341,20 @@ extern int w1_max_slave_ttl;
>  extern struct list_head w1_masters;
>  extern struct mutex w1_mlock;
>  
> -extern int w1_process_callbacks(struct w1_master *dev);
> +extern int __w1_process_callbacks(struct w1_master *dev);
>  extern int w1_process(void *);
>  
> +static inline int w1_process_callbacks(struct w1_master *dev)
> +{
> +	int ret;
> +
> +	mutex_lock(&dev->list_mutex);
> +	ret = __w1_process_callbacks(dev);
> +	mutex_unlock(&dev->list_mutex);
> +	return ret;
> +}
> +
> +
>  #endif /* __KERNEL__ */
>  
>  #endif /* __W1_H */
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/

[PATCH v2] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[Alexey Khoroshilov]

w1_process_callbacks() expects to be called with dev->list_mutex held,
but it is the fact only in w1_process(). __w1_remove_master_device()
calls w1_process_callbacks() after it releases list_mutex.

The patch fixes __w1_remove_master_device() to acquire list_mutex
for w1_process_callbacks().

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

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/w1/w1.c     | 2 ++
 drivers/w1/w1_int.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index ff52618cafbe..5d7341520544 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -1078,6 +1078,8 @@ static void w1_search_process(struct w1_master *dev, u8 search_type)
  * w1_process_callbacks() - execute each dev->async_list callback entry
  * @dev: w1_master device
  *
+ * The w1 master list_mutex must be held.
+ *
  * Return: 1 if there were commands to executed 0 otherwise
  */
 int w1_process_callbacks(struct w1_master *dev)
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 9b084db739c7..728039d2efe1 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -219,9 +219,13 @@ void __w1_remove_master_device(struct w1_master *dev)
 
 		if (msleep_interruptible(1000))
 			flush_signals(current);
+		mutex_lock(&dev->list_mutex);
 		w1_process_callbacks(dev);
+		mutex_unlock(&dev->list_mutex);
 	}
+	mutex_lock(&dev->list_mutex);
 	w1_process_callbacks(dev);
+	mutex_unlock(&dev->list_mutex);
 
 	memset(&msg, 0, sizeof(msg));
 	msg.id.mst.id = dev->id;
-- 
1.8.3.2

Re: [PATCH v2] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[David Fries]

Acked-by: David Fries <david@fries.net>

On Wed, May 07, 2014 at 01:26:04AM +0400, Alexey Khoroshilov wrote:
> w1_process_callbacks() expects to be called with dev->list_mutex held,
> but it is the fact only in w1_process(). __w1_remove_master_device()
> calls w1_process_callbacks() after it releases list_mutex.
> 
> The patch fixes __w1_remove_master_device() to acquire list_mutex
> for w1_process_callbacks().
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/w1/w1.c     | 2 ++
>  drivers/w1/w1_int.c | 4 ++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
> index ff52618cafbe..5d7341520544 100644
> --- a/drivers/w1/w1.c
> +++ b/drivers/w1/w1.c
> @@ -1078,6 +1078,8 @@ static void w1_search_process(struct w1_master *dev, u8 search_type)
>   * w1_process_callbacks() - execute each dev->async_list callback entry
>   * @dev: w1_master device
>   *
> + * The w1 master list_mutex must be held.
> + *
>   * Return: 1 if there were commands to executed 0 otherwise
>   */
>  int w1_process_callbacks(struct w1_master *dev)
> diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
> index 9b084db739c7..728039d2efe1 100644
> --- a/drivers/w1/w1_int.c
> +++ b/drivers/w1/w1_int.c
> @@ -219,9 +219,13 @@ void __w1_remove_master_device(struct w1_master *dev)
>  
>  		if (msleep_interruptible(1000))
>  			flush_signals(current);
> +		mutex_lock(&dev->list_mutex);
>  		w1_process_callbacks(dev);
> +		mutex_unlock(&dev->list_mutex);
>  	}
> +	mutex_lock(&dev->list_mutex);
>  	w1_process_callbacks(dev);
> +	mutex_unlock(&dev->list_mutex);
>  
>  	memset(&msg, 0, sizeof(msg));
>  	msg.id.mst.id = dev->id;
> -- 
> 1.8.3.2

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/

Re: [PATCH v2] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[zbr]

Hi everyone

07.05.2014, 03:49, "David Fries" <David@Fries.net>:
> Acked-by: David Fries <david@fries.net>
>
> On Wed, May 07, 2014 at 01:26:04AM +0400, Alexey Khoroshilov wrote:
>
>>  w1_process_callbacks() expects to be called with dev->list_mutex held,
>>  but it is the fact only in w1_process(). __w1_remove_master_device()
>>  calls w1_process_callbacks() after it releases list_mutex.
>>
>>  The patch fixes __w1_remove_master_device() to acquire list_mutex
>>  for w1_process_callbacks().
>>
>>  Found by Linux Driver Verification project (linuxtesting.org).
>>
>>  Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>

Thanks for this fix
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

Re: [PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[Alexey Khoroshilov]

On 01.05.2014 07:48, David Fries wrote:
> On Thu, May 01, 2014 at 12:37:58AM +0400, Alexey Khoroshilov wrote:
>> w1_process_callbacks() expects to be called with dev->list_mutex held,
>> but it is the fact only in w1_process(). __w1_remove_master_device()
>> calls w1_process_callbacks() after it releases list_mutex.
>>
>> The patch fixes __w1_remove_master_device() to acquire list_mutex
>> for w1_process_callbacks(). It is implemented by moving
>> w1_process_callbacks() functionality to __w1_process_callbacks()
>> and adding a wrapper with the old name.
> Good catch.  I guess as it was in the shutdown path it failed the
> unlock silently.
>
> I would prefer a different fix.  If w1_process_callbacks was more of a
> public facing API called from multiple locations where the caller
> doesn't have access to the locks, something like this would probably
> be the way to go.  This is called from two areas of the code, and not
> likely to be called from any new areas in the future.
>
> Adding a documentation comment is good.  I would be tempted in
> __w1_remove_master_device to move the dev->list_mutex down after the
> while loop, but I can't be sure that whatever has a refcnt wouldn't
> need list_mutex.  The last w1_process_callbacks after the while loop
> shouldn't be needed I wouldn't think, I was just being defensive.
I do not understand all the details, but I am not sure.
refcnt can became 0, but that does not mean all callbacks have been
processed because they can be added before refcnt decrement but after
previous w1_process_callbacks invocation.
>   By
> the time it completes the while loop the reference count is 0 so there
> shouldn't be anything left for it to process and if there is, it's a
> race condition and game over anyway.  So just sandwich
> w1_process_callbacks with the lock/unlock in
> __w1_remove_master_device please.
Done.


Another suspicious question for me is how safe is it to call
  wake_up_process(dev->thread);
after
  kthread_stop(dev->thread);
?

>> Found by Linux Driver Verification project (linuxtesting.org).
> Was this found with code inspection or hardware testing with lock
> debugging enabled?
It was found by static verification tools developed within Linux Driver
Verification project.


Best regards,
Alexey

Re: [PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

[David Fries]

On Wed, May 07, 2014 at 01:42:12AM +0400, Alexey Khoroshilov wrote:
> On 01.05.2014 07:48, David Fries wrote:
> > On Thu, May 01, 2014 at 12:37:58AM +0400, Alexey Khoroshilov wrote:
> >> w1_process_callbacks() expects to be called with dev->list_mutex held,
> >> but it is the fact only in w1_process(). __w1_remove_master_device()
> >> calls w1_process_callbacks() after it releases list_mutex.
> >>
> >> The patch fixes __w1_remove_master_device() to acquire list_mutex
> >> for w1_process_callbacks(). It is implemented by moving
> >> w1_process_callbacks() functionality to __w1_process_callbacks()
> >> and adding a wrapper with the old name.
> > Good catch.  I guess as it was in the shutdown path it failed the
> > unlock silently.
> >
> > I would prefer a different fix.  If w1_process_callbacks was more of a
> > public facing API called from multiple locations where the caller
> > doesn't have access to the locks, something like this would probably
> > be the way to go.  This is called from two areas of the code, and not
> > likely to be called from any new areas in the future.
> >
> > Adding a documentation comment is good.  I would be tempted in
> > __w1_remove_master_device to move the dev->list_mutex down after the
> > while loop, but I can't be sure that whatever has a refcnt wouldn't
> > need list_mutex.  The last w1_process_callbacks after the while loop
> > shouldn't be needed I wouldn't think, I was just being defensive.
> I do not understand all the details, but I am not sure.
> refcnt can became 0, but that does not mean all callbacks have been
> processed because they can be added before refcnt decrement but after
> previous w1_process_callbacks invocation.
> >   By
> > the time it completes the while loop the reference count is 0 so there
> > shouldn't be anything left for it to process and if there is, it's a
> > race condition and game over anyway.  So just sandwich
> > w1_process_callbacks with the lock/unlock in
> > __w1_remove_master_device please.
> Done.

Thanks for finding and fixing this bug.

> Another suspicious question for me is how safe is it to call
>   wake_up_process(dev->thread);
> after
>   kthread_stop(dev->thread);
> ?

Looks bad, but I would need to convince myself it could actually be a
problem.
int wake_up_process(struct task_struct *p)
{       
        WARN_ON(task_is_stopped_or_traced(p));
        return try_to_wake_up(p, TASK_NORMAL, 0);
}

But I think there are lower hanging fruit, like revisiting all the
locks in all the code and testing with the lock debugging code on and
get it all to pass or document that it really is correct.  Currently
it is detecting a locking order problem, but both code paths can only
be called from the w1_process function, which can only be called by
one thread, and one thread can't dead lock itself with a locking order
problem.

> >> Found by Linux Driver Verification project (linuxtesting.org).
> > Was this found with code inspection or hardware testing with lock
> > debugging enabled?
> It was found by static verification tools developed within Linux Driver
> Verification project.
> 
> 
> Best regards,
> Alexey

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/