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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/