linux-kernel - Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <54F9D2A9.8030401@citrix.com>
Date:	Fri, 6 Mar 2015 16:15:37 +0000
From:	David Vrabel <david.vrabel@...rix.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	Ingo Molnar <mingo@...nel.org>,
	Dave Hansen <dave.hansen@...el.com>,
	Borislav Petkov <bp@...e.de>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Pekka Riikonen <priikone@....fi>,
	Rik van Riel <riel@...hat.com>,
	Suresh Siddha <sbsiddha@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"Yu, Fenghua" <fenghua.yu@...el.com>,
	Quentin Casasnovas <quentin.casasnovas@...cle.com>
Subject: Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly
 disable irqs

On 06/03/15 15:36, Oleg Nesterov wrote:
> On 03/06, David Vrabel wrote:
>>
>> On 06/03/15 14:01, Oleg Nesterov wrote:
>>
>>> Not sure I understand it correctly after the first quick look, but
>>>
>>> 1. It conflicts with the recent changes in tip/x86/fpu
>>>
>>> 2. fpu_ini() initializes current->thread.fpu.state. This looks unneeded,
>>>    the kernel threads no longer have FPU context and do not abuse CPU.
>>>
>>> 3. I can be easily wrong, but it looks buggy... Note that
>>>    arch_dup_task_struct() doesn't allocate child->fpu.state if
>>>    !tsk_used_math(parent).
>>
>> ...yes. It's bit-rotted a bit.
>>
>>> No, I do not think this patch is a good idea. Perhaps I am wrong, but I
>>> think we need other changes. And they should start from init_fpu().
>>
>> But the general principle of avoiding the allocation in the #NM handler
>> and hence avoiding the need to re-enable IRQs is still a good idea, yes?
> 
> This needs more discussion, but in short so far I think that fpu_alloc()
> from #NM exception is fine if user_mode(regs) == T.

I think a memory allocation here, where the only behaviour on a failure
is to kill the task, is (and has always been) a crazy idea.

Additionally, in a Xen PV guest the #NM handler is called with TS
already cleared by the hypervisor so the handler must not enable
interrupts (and thus potentially schedule another task) until after the
current task's fpu state has been restored.  If a task was scheduled
before restoring the FPU state, TS would be clear and that task will use
fpu state from a previous task.

> Just do_device_not_available() should simply do conditional_sti(), I think.
> Perhaps it can even enable irqs unconditionally, but we need to verify that
> this is 100% correct.
> 
> And I agree that "if (!tsk_used_math(tsk))" code in math_state_restore()
> should be removed. But not to avoid the allocation, and this needs other
> changes.
> 
> Oleg.
> 

--
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/