linux-kernel - Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning

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]

Date:	Mon, 7 Mar 2016 18:18:09 +0100
From:	Sedat Dilek <sedat.dilek@...il.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Jiri Kosina <jikos@...nel.org>, Tejun Heo <tj@...nel.org>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Benjamin Tissoires <benjamin.tissoires@...hat.com>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Andy Lutomirski <luto@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	USB list <linux-usb@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning

On Mon, Mar 7, 2016 at 5:41 PM, Alan Stern <stern@...land.harvard.edu> wrote:
> On Mon, 7 Mar 2016, Sedat Dilek wrote:
>
>> Hmm, we are there where I was looking at...
>>
>> Please, read the reply of Jiri [1], we did some tweaking.
>> With CONFIG_FTRACE=n and CONFIG_PROVE_LOCKING=n !
>
> Yes, Jiri was looking more or less at the right place but his
> conclusions were wrong.
>
>> *** Part one: ObjectDump of hid-core.o ***
>>
>> $ objdump -D drivers/hid/usbhid/hid-core.o | awk '/<[^>]*>:$/ { p=0; }
>> /<usbhid_close>:/ { p=1; } { if (p) print $0; }' >
>> ../objdump-D_hid-core_o_usbhid_close_$(uname -r).txt
>
> This reveals the problem, at last...
>
>> $ cat ../objdump-D_hid-core_o_usbhid_close_4.4.4-1-iniza-small.txt
>> 00000000000002e0 <usbhid_close>:
>>      2e0:       55                      push   %rbp
>>      2e1:       48 89 e5                mov    %rsp,%rbp
>>      2e4:       41 57                   push   %r15
>>      2e6:       41 56                   push   %r14
>>      2e8:       41 54                   push   %r12
>>      2ea:       53                      push   %rbx
>>      2eb:       49 89 ff                mov    %rdi,%r15
>>      2ee:       4d 8b b7 e8 1e 00 00    mov    0x1ee8(%r15),%r14
>>      2f5:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi
>>      2fc:       31 f6                   xor    %esi,%esi
>>      2fe:       e8 00 00 00 00          callq  303 <usbhid_close+0x23>
>
> mutex_lock(&hid_open_mut);
>
>>      303:       49 8d 9e 88 28 00 00    lea    0x2888(%r14),%rbx
>>      30a:       48 89 df                mov    %rbx,%rdi
>>      30d:       e8 00 00 00 00          callq  312 <usbhid_close+0x32>
>
> spin_lock_irq(&usbhid->lock);
>
>>      312:       41 ff 8f e4 1d 00 00    decl   0x1de4(%r15)
>
> --hid->open
>
>>      319:       9c                      pushfq
>>      31a:       41 5c                   pop    %r12
>>      31c:       48 89 df                mov    %rbx,%rdi
>>      31f:       e8 00 00 00 00          callq  324 <usbhid_close+0x44>
>>      324:       41 54                   push   %r12
>>      326:       9d                      popfq
>
> spin_unlock_irq(&usbhid->lock); while attempting to preserve the Z
> flag.  The problem is that this code sequence will also preserve the
> Interrupt Flag!
>
>>      327:       75 23                   jne    34c <usbhid_close+0x6c>
>
> if (!--hid->open), testing the Z flag from the decl.
>
>>      329:       4c 89 f7                mov    %r14,%rdi
>>      32c:       e8 3f 00 00 00          callq  370 <hid_cancel_delayed_stuff>
>
> But now hid_cancel_delayed_stuff(usbhid) gets called with interrupts
> disabled.
>
> It's hard to call this a compiler bug, but perhaps it is -- I don't
> know how programmers are supposed to tell CLANG that a subroutine
> modifies the Interrupt Flag in a way that the compiler shouldn't mess
> up.
>

So, if Clang is producing wrong X86 code here, is it possible to turn
interrupts on/off manually?
But, hmm that affects other places as well in the Linux sources, so.

- Sedat -