linux-kernel - Re: [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in xsave_user/xrestore

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: <20150317120739.GH18917@pd.tnic>
Date:	Tue, 17 Mar 2015 13:07:39 +0100
From:	Borislav Petkov <bp@...e.de>
To:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	Dave Hansen <dave.hansen@...el.com>,
	Ingo Molnar <mingo@...nel.org>,
	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>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in
 xsave_user/xrestore_user

On Tue, Mar 17, 2015 at 12:36:58PM +0100, Quentin Casasnovas wrote:
> Right, FWIW I think your approach is valid, but not very generic.  Re-using
> the check_insn() and making it more generic so we can widen its use felt
> like a better approach to me.
> 
> AIUI, you didn't like my earlier draft because it wasn't very readable, but
> I think this was just due to the (bad) example I took and by reworking it a
> bit more, we could end up with the code you previously envisionned:
> 
>   if (static_cpu_has_safe(X86_FEATURE_XSAVEOPT))
>           return check_insn(XSAVEOPT, xsave_buf, ...);
>   else if (static_cpu_has_safe(X86_FEATURE_XSAVES)
>           return check_insn(XSAVES, xsave_buf, ...);
>   else
> 	  return check_insn(XSAVE, xsave_buf, ...)
> 
> Or maybe you were saying the actual macros weren't readable?

Well, TBH, I don't like check_insn() either:

* naming is generic but it is not really used in a generic way - only in
FPU code.

* having variable arguments makes it really really unreadable to me when
you start looking at how it is called:

	...
        if (config_enabled(CONFIG_X86_32))
                return check_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
	...

The only thing that lets me differentiate what is input and what is
output is the "=" in there and you have to know inline asm to know that.

* The arguments have the same syntax as inline asm() arguments but you
don't see "asm volatile" there so it looks like something half-arsed in
between.

* the first argument is the instruction string with the operands which
gets stringified, yuck!

Do I need to say more? :-)

So what I would like is if we killed those half-arsed macros and
use either generic, clean macros like the alternatives or define
FPU-specific ones which do what FPU code needs done. If the second,
they should be self-contained, all in one place so that you don't have
to grep like crazy to rhyme together what the macro does - nothing like
xsave_fault. Yuck.

Or even extend the generic macros to fit the FPU use case, if possible
and if it makes sense.

Oh, and we shouldn't leave readability somewhere on the road.

I hope you catch my drift here.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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/