[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z_K83hCo-IgEHvM_@gmail.com>
Date: Sun, 6 Apr 2025 19:41:50 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
Peter Zijlstra <peterz@...radead.org>,
Juergen Gross <jgross@...e.com>,
Andrew Cooper <andrew.cooper3@...rix.com>
Subject: Re: [PATCH v3] objtool: Fix SYSCALL instruction handling and
INSN_CONTEXT_SWITCH
* Josh Poimboeuf <jpoimboe@...nel.org> wrote:
> The !CONFIG_IA32_EMULATION version of xen_entry_SYSCALL_compat() ends
> with a SYSCALL instruction. In Xen, SYSCALL is a hypercall. Usually
> that would return, but in this case it's a hypercall to IRET, which
> doesn't return.
>
> Objtool doesn't know that, so it falls through to the next function,
> triggering a false positive:
>
> vmlinux.o: warning: objtool: xen_reschedule_interrupt+0x2a: RET before UNTRAIN
>
> Fix that by adding UD2 after the SYSCALL to avoid the undefined behavior
> and prevent the objtool fallthrough, and teach validate_unret() to stop
> control flow on the UD2 like validate_branch() already does.
>
> Unfortunately that's not the whole story. While that works for
> validate_unret(), it breaks validate_branch() which terminates control
> flow after the SYSCALL, triggering an unreachable instruction warning on
> the UD2.
>
> The real problem here is that INSN_CONTEXT_SWITCH is ambiguous. It can
> represent both call semantics (SYSCALL, SYSENTER) and return semantics
> (SYSRET, IRET, RETS, RETU). Those differ significantly: calls preserve
> control flow whereas returns terminate it.
>
> validate_branch() uses an arbitrary rule for INSN_CONTEXT_SWITCH that
> almost works by accident: if in a function, keep going; otherwise stop.
> It should instead be based on the semantics of the underlying
> instruction.
>
> INSN_CONTEXT_SWITCH's original purpose was to enable the "unsupported
> instruction in callable function" warning. But that warning really has
> no reason to exist. It has never found any bugs, and those instructions
> are only in entry code anyway. So just get rid of it.
>
> That in turn allows objtool to stop caring about SYSCALL or SYSENTER.
> Their call semantic means they usually don't affect control flow in the
> containing function/code, and can just be INSN_OTHER. The far
> returns/jumps can also be ignored as those aren't used anywhere.
>
> With SYSCALL and SYSENTER removed, INSN_CONTEXT_SWITCH now has a sane
> well-defined return semantic. Rename it to INSN_EXCEPTION_RETURN to
> better reflect its meaning.
I think this should be split up into a series of ~3 patches. :-/
Thanks,
Ingo
Powered by blists - more mailing lists