Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Tue, Mar 31, 2020 at 03:23:15PM -0500, Josh Poimboeuf wrote:
> 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.

Right.

> 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.

I like how this is specific on how far the stack can be off, as opposed
so say 'ignore any warning on this instruction'.

Because by saying this RET should be +8, we'll still get a warning when
this is not the case (and in fact I should strengthen the patch to
implement that).

Also, you don't want to suppress any other valid warning at that
instruction.

Furthermore, I really don't think we ought to worry about ease-of-use
here, there's really not that many people writing x86 assembly.

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

Fair enough.

> 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'm not against adding a second/separate hint for this. In fact, I
almost considered teaching objtool how to interpret the whole IRET frame
so that we can do it without hints. It's just that that's too much code
for this one case.

HINT_IRET_SELF ?