linux-kernel - Re: [PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53695734.9040302@ispras.ru>
Date:	Wed, 07 May 2014 01:42:12 +0400
From:	Alexey Khoroshilov <khoroshilov@...ras.ru>
To:	David Fries <David@...es.net>
CC:	Evgeniy Polyakov <zbr@...emap.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, ldv-project@...uxtesting.org
Subject: Re: [PATCH] w1: do not unlock unheld list_mutex in __w1_remove_master_device()

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/