Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 14 Jun 2023 11:17:51 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Omar Sandoval <osandov@osandov.com>
Cc:     Josh Poimboeuf <jpoimboe@kernel.org>, linux-kernel@vger.kernel.org,
        linux-debuggers@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH v2] x86/unwind/orc: add ELF section with ORC version
 identifier
Message-ID: <20230614091751.GE1639749@hirez.programming.kicks-ass.net>
References: <aef9c8dc43915b886a8c48509a12ec1b006ca1ca.1686690801.git.osandov@osandov.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aef9c8dc43915b886a8c48509a12ec1b006ca1ca.1686690801.git.osandov@osandov.com>
Precedence: bulk

On Tue, Jun 13, 2023 at 02:14:56PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Commits ffb1b4a41016 ("x86/unwind/orc: Add 'signal' field to ORC
> metadata") and fb799447ae29 ("x86,objtool: Split UNWIND_HINT_EMPTY in
> two") changed the ORC format. Although ORC is internal to the kernel,
> it's the only way for external tools to get reliable kernel stack traces
> on x86-64. In particular, the drgn debugger [1] uses ORC for stack
> unwinding, and these format changes broke it [2]. As the drgn
> maintainer, I don't care how often or how much the kernel changes the
> ORC format as long as I have a way to detect the change.
> 
> It suffices to store a version identifier in the vmlinux and kernel
> module ELF files (to use when parsing ORC sections from ELF), and in
> kernel memory (to use when parsing ORC from a core dump+symbol table).
> Rather than hard-coding a version number that needs to be manually
> bumped, Peterz suggested hashing the definitions from orc_types.h. If
> there is a format change that isn't caught by this, the hashing script
> can be updated.
> 
> This patch adds an .orc_header allocated ELF section containing the
> 20-byte hash to vmlinux and kernel modules, along with the corresponding
> __start_orc_header and __stop_orc_header symbols in vmlinux.
> 
> 1: https://github.com/osandov/drgn
> 2: https://github.com/osandov/drgn/issues/303
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Patch looks good to me; as a follow up I suppose we could verify the orc
hash on module load, to ensure the module and main kernel agree on the
ORC version used -- but we can do that later.

> ---
> Hi,
> 
> This is v2 of my patch to make it possible for external tools like drgn
> to identify versions of the ORC format. As stated in v1 [1], I don't
> want ORC to be stable ABI; I just need a way to identify the format
> being used.
> 
> This version incorporates Peter's suggestion to hash the ORC definitions
> instead of requiring a manual version number; this is easier to maintain
> and more resilient to backports.
> 
> I would love to get this in before 6.4 is released, and then hopefully
> backport it to 6.3-stable.

So we're fairly late in the cycle and it would need justification to go
into objtool/urgent -- preferably only fixes at this point.

But given we 'broke' the ORC layout this cycle, we can mark this with
Fixes: for the two mentioned commits.

Josh?