On Wed, Nov 12, 2014 at 2:00 PM, Oleg Nesterov <o...@redhat.com> wrote: > Andy, > > As I said many times I do not understand asm ;) so most probably I missed > something but let me ask anyway.
You must be the most competent non-asm-speaking asm reviewer in the world :) > > On 11/11, Andy Lutomirski wrote: >> >> --- a/arch/x86/kernel/entry_64.S >> +++ b/arch/x86/kernel/entry_64.S >> @@ -1064,6 +1064,9 @@ ENTRY(\sym) >> CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15 >> >> .if \paranoid >> + CFI_REMEMBER_STATE >> + testl $3, CS(%rsp) /* If coming from userspace, switch */ >> + jnz 1f /* stacks. */ >> call save_paranoid >> .else >> call error_entry >> @@ -1104,6 +1107,36 @@ ENTRY(\sym) >> jmp error_exit /* %ebx: no swapgs flag */ >> .endif >> >> + .if \paranoid >> + CFI_RESTORE_STATE >> + /* >> + * Paranoid entry from userspace. Switch stacks and treat it >> + * as a normal entry. This means that paranoid handlers >> + * run in real process context if user_mode(regs). >> + */ >> +1: >> + call error_entry >> + >> + DEFAULT_FRAME 0 >> + >> + movq %rsp,%rdi /* pt_regs pointer */ >> + call sync_regs > > Can't we simplify sync_regs() then? Yes. Will do. > >> @@ -1324,8 +1357,6 @@ ENTRY(paranoid_exit) >> TRACE_IRQS_OFF_DEBUG >> testl %ebx,%ebx /* swapgs needed? */ >> jnz paranoid_restore >> - testl $3,CS(%rsp) >> - jnz paranoid_userspace >> paranoid_swapgs: > > Looks like this label can die. > Yes. >> -paranoid_userspace: >> - GET_THREAD_INFO(%rcx) >> - movl TI_flags(%rcx),%ebx >> - andl $_TIF_WORK_MASK,%ebx >> - jz paranoid_swapgs >> - movq %rsp,%rdi /* &pt_regs */ >> - call sync_regs >> - movq %rax,%rsp /* switch stack for scheduling */ >> - testl $_TIF_NEED_RESCHED,%ebx >> - jnz paranoid_schedule >> - movl %ebx,%edx /* arg3: thread flags */ >> - TRACE_IRQS_ON >> - ENABLE_INTERRUPTS(CLBR_NONE) >> - xorl %esi,%esi /* arg2: oldset */ >> - movq %rsp,%rdi /* arg1: &pt_regs */ >> - call do_notify_resume > > So, before this patch we use _TIF_WORK_MASK to decide if we need to call > do_notify_resume(). > > After this patch we jump to error_exit and it checks the same _TIF_WORK_MASK. > But note that retint_careful->retint_careful checks another mask, > _TIF_DO_NOTIFY_MASK. > > So it seems to me we can miss (say) TIF_UPROBE after int3 handler, no? Only if we didn't call uprobe_deny_signal. Presumably the TIF_NOTIFY_RESUME got added there because it didn't work on x86 due to exactly this issue. > > Yes, even _if_ I am right we should blame these masks, _TIF_DO_NOTIFY_MASK > should probably include _TIF_UPROBE (and afaics in this case we can remove > set_thread_flag(TIF_NOTIFY_RESUME) in uprobe_deny_signal()). Ugh, yes. I'll fix that with a separate patch. Fortunately, the other uprobe architectures seem to be okay. > > And in any case, can't we cleanup _TIF_WORK_MASK and _TIF_ALLWORK_MASK? > IMHO, they should clearly define which bits we want to check. Almost certainly. I'll leave that for another day, though. v2 coming soon with these changes and some additional comment cleanups. --Andy -- 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/
