On Mon, Mar 23, 2015 at 12:21 PM, Denys Vlasenko <dvlas...@redhat.com> wrote: > On 03/23/2015 08:10 PM, Andy Lutomirski wrote: >> We currently have a race: if we're preempted during syscall exit, we >> can fail to process syscall return work that is queued up while >> we're preempted in ret_from_sys_call after checking ti.flags. >> >> Fix it by disabling interrupts before checking ti.flags. >> >> Fixes: 96b6352c1271 x86_64, entry: Remove the syscall exit audit and >> schedule optimizations >> Reported-by: Stefan Seyfried <stefan.seyfr...@googlemail.com> >> Reported-by: Takashi Iwai <ti...@suse.de> >> Signed-off-by: Andy Lutomirski <l...@kernel.org> >> --- >> >> Ingo, I don't understand the LOCKDEP_SYS_EXIT stuff. Can you take a quick >> look to confirm that it's okay to call it more than once? >> >> arch/x86/kernel/entry_64.S | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S >> index 1d74d161687c..2babb393915e 100644 >> --- a/arch/x86/kernel/entry_64.S >> +++ b/arch/x86/kernel/entry_64.S >> @@ -364,12 +364,21 @@ system_call_fastpath: >> * Has incomplete stack frame and undefined top of stack. >> */ >> ret_from_sys_call: >> - testl $_TIF_ALLWORK_MASK,TI_flags+THREAD_INFO(%rsp,RIP-ARGOFFSET) >> - jnz int_ret_from_sys_call_fixup /* Go the the slow path */ >> - >> LOCKDEP_SYS_EXIT >> DISABLE_INTERRUPTS(CLBR_NONE) >> TRACE_IRQS_OFF >> + >> + /* >> + * We must check ti flags with interrupts (or at least preemption) >> + * off because we must *never* return to userspace without >> + * processing exit work that is enqueued if we're preempted here. >> + * In particular, returning to userspace with any of the one-shot >> + * flags (TIF_NOTIFY_RESUME, TIF_USER_RETURN_NOTIFY, etc) set is >> + * very bad. >> + */ >> + testl $_TIF_ALLWORK_MASK,TI_flags+THREAD_INFO(%rsp,RIP-ARGOFFSET) >> + jnz int_ret_from_sys_call_fixup /* Go the the slow path */ > ^^^^^^^^^^^^^^^^^^^^ > > typo here; s/the the/to the/
Whoops. > > >> + >> CFI_REMEMBER_STATE >> /* >> * sysretq will re-enable interrupts: >> @@ -386,7 +395,7 @@ ret_from_sys_call: >> >> int_ret_from_sys_call_fixup: >> FIXUP_TOP_OF_STACK %r11, -ARGOFFSET >> - jmp int_ret_from_sys_call >> + jmp int_ret_from_sys_call_irqs_off >> >> /* Do syscall tracing */ >> tracesys: >> @@ -432,6 +441,7 @@ tracesys_phase2: >> GLOBAL(int_ret_from_sys_call) >> DISABLE_INTERRUPTS(CLBR_NONE) >> TRACE_IRQS_OFF >> +int_ret_from_sys_call_irqs_off: >> movl $_TIF_ALLWORK_MASK,%edi >> /* edi: mask to check */ >> GLOBAL(int_with_check) > > > You can avoid having to know LOCKDEP_SYS_EXIT :) > Just set %edi = $_TIF_ALLWORK_MASK, and jump a bit farther: > > > movl $_TIF_ALLWORK_MASK,%edi > testl %edi,TI_flags+THREAD_INFO(%rsp,RIP-ARGOFFSET) > jnz int_ret_from_sys_call_fixup /* Go to the slow path */ > ... > ... > GLOBAL(int_ret_from_sys_call) > DISABLE_INTERRUPTS(CLBR_NONE) > TRACE_IRQS_OFF > movl $_TIF_ALLWORK_MASK,%edi > /* edi: mask to check */ > GLOBAL(int_with_check) > LOCKDEP_SYS_EXIT_IRQ > int_ret_from_sys_call_irqs_off: <========== HERE > I didn't want to do that, because I really want to rewrite int_ret_from_sys_call in C. --Andy -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/