[llvm-bugs] [Bug 36028] New: Incorrect use of pushf/popf enables/disables interrupts on amd64 kernels
via llvm-bugs
llvm-bugs at lists.llvm.org
Sat Jan 20 11:41:58 PST 2018
https://bugs.llvm.org/show_bug.cgi?id=36028
Bug ID: 36028
Summary: Incorrect use of pushf/popf enables/disables
interrupts on amd64 kernels
Product: libraries
Version: 6.0
Hardware: PC
OS: FreeBSD
Status: NEW
Severity: normal
Priority: P
Component: Backend: X86
Assignee: unassignedbugs at nondot.org
Reporter: jonlooney at gmail.com
CC: llvm-bugs at lists.llvm.org
Created attachment 19716
--> https://bugs.llvm.org/attachment.cgi?id=19716&action=edit
Patch to make LLVM do a slightly better job at restoring the EFLAGS register
I am a FreeBSD kernel developer. While making a seemingly-simple change to the
FreeBSD kernel, I found that interrupt handling was suddenly incorrect. After
troubleshooting for some time, I found that this was due to the way LLVM's x86
backend is using pushf/popf to save/restore EFLAGS across function calls.
Simple reproduction (which requires no headers):
---Start File---
void spinlock_acquire(void);
void spinlock_release(void);
void fx(void);
void otherfx(void);
static int count = 0;
void
fx(void)
{
_Bool needtocall;
spinlock_acquire();
needtocall = (++count == 0);
spinlock_release();
if (needtocall)
otherfx();
}
---End File---
In the above file, assume that spinlock_acquire() may disable interrupts and
spinlock_release() my enable interrupts.
If you compile this code using clang (5.0.0, 5.0.1, or 6.0.0 all behave
similarly in the critical respect) with the -O2 and -S flags for a generic
x86-64 cpu, you will see that LLVM generates this assembly:
---Start Assembly---
.text
.file "popfqtest.c"
.globl fx # -- Begin function fx
.p2align 4, 0x90
.type fx, at function
fx: # @fx
.cfi_startproc
# BB#0: # %entry
pushq %rbp
.Lcfi0:
.cfi_def_cfa_offset 16
.Lcfi1:
.cfi_offset %rbp, -16
movq %rsp, %rbp
.Lcfi2:
.cfi_def_cfa_register %rbp
pushq %rbx
pushq %rax
.Lcfi3:
.cfi_offset %rbx, -24
callq spinlock_acquire
incl count(%rip)
pushfq
popq %rbx
callq spinlock_release
addq $8, %rsp
pushq %rbx
popfq
je .LBB0_2
# BB#1: # %if.end
popq %rbx
popq %rbp
retq
.LBB0_2: # %if.then
popq %rbx
popq %rbp
jmp otherfx # TAILCALL
.Lfunc_end0:
.size fx, .Lfunc_end0-fx
.cfi_endproc
# -- End function
.type count, at object # @count
.local count
.comm count,4,4
.ident "FreeBSD clang version 5.0.1 (tags/RELEASE_501/final 320880)
(based on LLVM 5.0.1)"
.section ".note.GNU-stack","", at progbits
---End Assembly---
The critical part is the way that LLVM handles the conditional:
1. Increment the value. This will set ZF if count reaches 0:
incl count(%rip)
2. Now, save the status flags (particularly, ZF) using pushfq. Critically,
pushfq saves the entire EFLAGS value, which includes the interrupt flag.
Because we are holding a spin mutex, interrupts are disabled at this point:
pushfq
popq %rbx
3. Now, call the function to unlock the spin mutex, which will potentially
enable interrupts.
callq spinlock_release
4. Now, restore the status flags. Critically, popfq restores the entire EFLAGS
value, which includes the interrupt flag. Because interrupts were disabled when
the flags were stored, these instructions will disable them again:
addq $8, %rsp
pushq %rbx
popfq
5. Now, use the status flags to determine whether to call the function:
je .LBB0_2
After running this code, interrupts may remain erroneously disabled.
(This is a contrived minimal test case, but I did run into this bug while
testing a code change in the kernel. Interrupts remained erroneously disabled
after dropping a spin lock.)
By now, the bug should be obvious: LLVM shouldn't save and restore the entire
EFLAGS register. It doesn't know what changes these other function calls make
to the values of the EFLAGS register about which clang doesn't care (e.g. IF,
IOPL, AC, etc.). By saving and restoring EFLAGS across function calls, it can
load incorrect values for these other flags.
It looks like the correct solutions are to either not save EFLAGS across
function calls, or to just save the individual flag(s) that need to be
evaluated across the function call. (For example, a simple setz/test would
probably be sufficient -- and more efficient -- in this particular case.)
However, these appear to be very significant changes that are complicated.
Two other things to note:
1. If LLVM knows that the target hardware supports lahf/sahf, it will use those
instructions instead. That solves the problem, as it doesn't reset any flags
other than status flags. (It still may not be as efficient as setz, but at
least it is safe for kernel code. And, it is probably efficient enough for as
often as it is used.) lahf/sahf are supported on all i386 and *most* (but not
all) x86-64 CPUs. By default, we build the FreeBSD kernel for a "generic"
processor, which means that LLVM assumes the processor doesn't support the
lahf/sahf instructions. This means that our kernel builds default to using the
pushf/popf method for saving EFLAGS.
2. In practice, this seems to very seldom show up in our kernel binaries. Even
a trivial change to the above code (for example, "++count == 5" instead of
"++count == 0") would make the compiler no longer save the EFLAGS register
across the function call. I quickly audited the kernel and modules I compile
and use at work, and found only one place that compiles with the pushf/popf
instructions to save/restore the EFLAGS register across a function call. (Of
course, this fragility can work in the reverse: a trivial change, such as the
one I tried when I found this bug, can cause the compiler to use the pushf/popf
instructions where it had not previously and, therefore, introduce a bug in
interrupt handling.)
Finally, I have attached a patch that "fixes" (or, probably more accurately,
"works around") the bug. It changes the pushf/popf so that the program will
retrieve the latest EFLAGS register at restore time and only modify the status
bits.
In C Pseudo-code, it does this:
save_eflags(register dest) {
dest = load_eflags();
dest &= (OF|SF|ZF|AF|PF|CF);
}
restore_eflags(register src) {
register RAX;
RAX = load_eflags();
RAX &= ~(OF|SF|ZF|AF|PF|CF);
RAX |= src;
set_eflags(RAX);
}
The new assembly sequence looks like this:
Save EFLAGS:
28: 9c pushfq
29: 5b pop %rbx
2a: 48 81 e3 d5 08 00 00 and $0x8d5,%rbx
Restore EFLAGS:
4f: 9c pushfq
50: 48 8b 04 24 mov (%rsp),%rax
54: 48 25 2a f7 ff ff and $0xfffffffffffff72a,%rax
5a: 48 09 d8 or %rbx,%rax
5d: 48 89 04 24 mov %rax,(%rsp)
61: 9d popfq
This is far from ideal, but seems better than what is currently there. And, the
good news is that the compiler seems to actually save/restore EFLAGS very few
places in our kernel code (hopefully, none of which are in hot code paths).
And, people who really care about performance can compile with a specific CPU
architecture that supports LAHF/SAHF. So, perhaps, this is good enough for now.
--
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20180120/1d9f8bbb/attachment-0001.html>
More information about the llvm-bugs
mailing list