linux-kernel - Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in 32-bit ____down

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:	Fri, 13 May 2016 10:03:26 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Borislav Petkov <bp@...e.de>
Cc:	Guenter Roeck <linux@...ck-us.net>, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	"linux-next@...r.kernel.org" <linux-next@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/rwsem: Save and restore all callee-clobbered regs in
 32-bit ____down_write()

On Thu, May 12, 2016 at 10:29 AM, Borislav Petkov <bp@...e.de> wrote:
> Anyway, here's an actual patch with a commit message. Guenter, can you
> give it a run please?

I think the commit message is misleading.

> ____down_write() calls a function to handle the slow path when the lock
> is contended. But in order to be able to call a C function, one has to
> stash all callee-clobbered registers. The 32-bit path saves only %ecx
> for a reason unknown to me.

Why claim that it's unknown? You know exactly what the reason was:

> the useless dependency on edx was removed and this caused the following
> splat:

The dependency on %edx was clearly exactly because the calling
convention for the slow-path was that %eax and %edx were clobbered,
and %edx was used as a temporary, so clobbering it had no downside.

So it wasn't useless, it was explicit, and commit 71c01930b42e was just broken.

I think your fix is wrong. Your fix adds the pointless push/pop that
doesn't help any, since you might as well just force the temporary
back to %edx.

The correct fix is to revert the broken commit.

If commit 71c01930b42e had actually generated better code, that would
be different. But it doesn't. So as it is, this is all just worse than
it used to be, and I don't see the point of "fixing" things by making
them worse.

Revert back to the old "use %edx as a temporary", together with a
comment so that this doesn't happen again.

                     Linus