linux-kernel - Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET

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]

Message-ID: <20200331202315.zialorhlxmml6ec7@treble>
Date:   Tue, 31 Mar 2020 15:23:15 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org, x86@...nel.org,
        mhiramat@...nel.org, mbenes@...e.cz,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

On Tue, Mar 31, 2020 at 01:16:52PM +0200, Peter Zijlstra wrote:
> Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> 
> This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
> to the following instructions:
> 
>  - any instruction that terminates a function, like: RETURN and sibling
>    calls. It allows the stack-frame to be off by @sp_offset, ie. it
>    allows stuffing the return stack.
> 
>  - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
>    CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
>    the instruction continue.

Looking closer, I see how my UNWIND_HINT_ADJUST idea doesn't work for
the ftrace_regs_caller() case.  The ORC data is actually correct there.
So basically we need a way to tell objtool to be quiet.

I now understand what you're trying to do with the RET_TAIL thing, and I
guess it's ok for the ftrace case.  But I'd rather an UNWIND_HINT_IGNORE
before the tail cail, which would tell objtool to just silence the tail
call warning.  It's simpler for the user to understand, it's simpler
logic in objtool, and I think an "ignore warnings for the next insn"
hint would be more generally applicable anyway.

But also... the RET_OFFSET usage for sync_core() *really* bugs me.

I know you said it's like an indirect tail call with a bigger frame, but
that's kind of stretching it because the function frame is still there.

And objtool doesn't treat it like a tail call at all.  In fact, it
handles it *completely* differently from the normal ret-tail-call case.
Instead of silencing a tail call warning, it adjusts the stack offset
and continues the code path.

This basically adds *two* new hint types, while trying to call them the
same thing.  There's no overlapping functionality between them in
objtool, other than the use of the same insn->ret_offset variable.  But
it's two distinct functionalities, depending on the context (return/tail
vs IRETQ).

I'll try to work up some patches with a different approach in a bit.

-- 
Josh