Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 29 Apr 2020 17:51:09 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Miroslav Benes <mbenes@suse.cz>
Cc:     jpoimboe@redhat.com, alexandre.chartre@oracle.com,
        linux-kernel@vger.kernel.org, jthierry@redhat.com,
        tglx@linutronix.de, x86@kernel.org, Jann Horn <jannh@google.com>
Subject: Re: [PATCH v2 02/14] objtool: Fix ORC vs alternatives
Message-ID: <20200429155109.GN13592@hirez.programming.kicks-ass.net>
References: <20200428191101.886208539@infradead.org>
 <20200428191659.499074346@infradead.org>
 <alpine.LSU.2.21.2004291622160.28992@pobox.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LSU.2.21.2004291622160.28992@pobox.suse.cz>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Apr 29, 2020 at 04:33:31PM +0200, Miroslav Benes wrote:
> On Tue, 28 Apr 2020, Peter Zijlstra wrote:
> >  /*
> > + * Alternatives should not contain any ORC entries, this in turn means they
> > + * should not contain any CFI ops, which implies all instructions should have
> > + * the same same CFI state.
> > + *
> > + * It is possible to constuct alternatives that have unreachable holes that go
> > + * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
> > + * states which then results in ORC entries, which we just said we didn't want.
> > + *
> > + * Avoid them by copying the CFI entry of the first instruction into the whole
> > + * alternative.
> > + */
> > +static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
> > +{
> > +	struct instruction *first_insn = insn;
> > +	int alt_group = insn->alt_group;
> > +
> > +	sec_for_each_insn_continue(file, insn) {
> > +		if (insn->alt_group != alt_group)
> > +			break;
> > +		insn->cfi = first_insn->cfi;
> > +	}
> > +}
> 
> If I am reading this and previous patch correctly...
> 
> The function would copy cfi only to "orig" alternative (its insn->alts is 
> non-empty, orig_insn->alt_group differs from new_insn->alt_group), right? 

Yes.

> Would it make sense to do the same for "new" alternative, because of the 
> invariant? It seems to me it is not processed anywhere that way.

No.

> Am I missing something? Whenever I try to read all this alternatives 
> handling in objtool, I get lost pretty soon.

We only care about the ORC covering the original range, because that is
the range we execute code from. The memory where we store the
alternative instructions (.altinstruction section) is never executed,
that is, RIP should never point there, so we don't need ORC data covering
it.