| Message ID | 20221018122708.823792-1-jolsa@kernel.org |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp1932814wrs;
Tue, 18 Oct 2022 05:29:39 -0700 (PDT)
X-Google-Smtp-Source:
AMsMyM7T1egqqfUPmiDsjuM5Ff6d2peGMBNRCDPhNZIz8of0sE+b22OfY9KkLThbn8yzwYmtB/Kq
X-Received: by 2002:a17:906:2681:b0:783:6a92:4c38 with SMTP id
t1-20020a170906268100b007836a924c38mr2220274ejc.75.1666096179634;
Tue, 18 Oct 2022 05:29:39 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1666096179; cv=none;
d=google.com; s=arc-20160816;
b=fsP0qCAKsWwOn1FT9ywlJfBujUauLJ51IHYyn9VKGW35tdNgR/E/zReodz329tgigf
bNOpCzKa2CQtC19es45Ns8Ugi4gbUIIOizf+E4aQo21dWS+Dh6mrUNIUfbWnh4S4yQ2d
d34CnnHLll3TbpAfL3WrK+hCgsqzdQoJK3mjveHlJWgahndHkXd8j8TnJrhfOPI89es2
TUoJFhtW/DeH+l2EaV9L7FyQK3rEaYdBw7KYt5SfghUDUu4jiJs8kKZCLA1y8GDIjk7o
+MUlwNqvQi9hP/g2kks9m+afPGTYGUVPgpCWYhiW/gXFzHB1NLY/D2U2Ic0A0YAHsDlK
ud5A==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:content-transfer-encoding:mime-version
:message-id:date:subject:cc:to:from:dkim-signature;
bh=3S5tAXJNU/hSPSvRVRKydKWw46cogc0DhgOH2eIG+pw=;
b=F1DF8qL15I6UfD59Cf5G7W0I5WSKKDq/3e6Ol57HM+VJhPaMcPHJB4eil4S42mnE9k
G+TasBptBQ8ddkBpyRSTFVtrv+IJmbgZEs+s1N2XY3Qtj6Nq028/04HAZMYTkHe4P8Q5
tnPneyG4K6xeCjhxPmSjiedge1riYkWQfIM6JJaS+b+RfBXe4WCAz4S0cLqP9NAMSz0u
pz1xOfxm6mjU9lsf06TyB1RS3aZUATI5D4NPFw5HVWBV7PFq0RoziLvxFWbexAvAfpA+
d5lBEa2oiZe4uBfywJvL6tT9qlYFoY8LIZWpnwZMmzUWr360iAtYxFGs8TLrx3m649z/
UEwQ==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@kernel.org header.s=k20201202 header.b=SnamShzR;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
c8-20020a05640227c800b0045dbd131cd7si3351812ede.65.2022.10.18.05.29.14;
Tue, 18 Oct 2022 05:29:39 -0700 (PDT)
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;
Authentication-Results: mx.google.com;
dkim=pass header.i=@kernel.org header.s=k20201202 header.b=SnamShzR;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S230057AbiJRM1T (ORCPT <rfc822;carlos.wei.hk@gmail.com>
+ 99 others); Tue, 18 Oct 2022 08:27:19 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50132 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S229491AbiJRM1S (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Tue, 18 Oct 2022 08:27:18 -0400
Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3C2D9B1DF5;
Tue, 18 Oct 2022 05:27:17 -0700 (PDT)
Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by ams.source.kernel.org (Postfix) with ESMTPS id A82E0B81E9F;
Tue, 18 Oct 2022 12:27:15 +0000 (UTC)
Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5DA08C433D6;
Tue, 18 Oct 2022 12:27:10 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;
s=k20201202; t=1666096034;
bh=VQPk5DjCowXwXW+B/j4PJh6cSP/7mrS4QgVwxTpPUrE=;
h=From:To:Cc:Subject:Date:From;
b=SnamShzRyVDajya2DifAvhooiLO2rs+X1yv+zEsKTGIIEWf+rnyEBaElBV2jhOeUV
HO3XiGQn/GxhVD18TF6M1npJGJOuDg5BCi9VRB0VOjicJxBudNTjXhFedimSQvKIlx
h2yjtY87OQ1CBhIfj6LkvD/18NqpzDx15OaHRKD12pkUwILEaj0xUjtz8S+LO8/9ug
rJF/6pd4qXU8gqYcwSoxrBfkbqNsfW+rquNXAX4N9e6bm58F4tRx4Q30qsXMG0npf5
PDd2MkqAlPFFIv+nX781y3zeLosEdoyyw8kbvpd2LJOQjPptljtOGDYXvFKuxFH0ER
vgbIwyaq1KVJw==
From: Jiri Olsa <jolsa@kernel.org>
To: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
Andy Lutomirski <luto@kernel.org>
Cc: Akihiro HARAI <jharai0815@gmail.com>, bpf@vger.kernel.org,
linux-kernel@vger.kernel.org, Martin KaFai Lau <kafai@fb.com>,
Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@chromium.org>,
Stanislav Fomichev <sdf@google.com>,
Hao Luo <haoluo@google.com>
Subject: [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header
Date: Tue, 18 Oct 2022 14:27:08 +0200
Message-Id: <20221018122708.823792-1-jolsa@kernel.org>
X-Mailer: git-send-email 2.37.3
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1747028467432817589?=
X-GMAIL-MSGID: =?utf-8?q?1747028467432817589?=
|
| Series |
x86: Include asm/ptrace.h in syscall_wrapper header
|
|
Commit Message
Jiri Olsa
Oct. 18, 2022, 12:27 p.m. UTC
From: Jiri Olsa <olsajiri@gmail.com> With just the forward declaration of the 'struct pt_regs' in syscall_wrapper.h, the syscall stub functions: __[x64|ia32]_sys_*(struct pt_regs *regs) will have different definition of 'regs' argument in BTF data based on which object file they are defined in. If the syscall's object includes 'struct pt_regs' definition, the BTF argument data will point to 'struct pt_regs' record, like: [226] STRUCT 'pt_regs' size=168 vlen=21 'r15' type_id=1 bits_offset=0 'r14' type_id=1 bits_offset=64 'r13' type_id=1 bits_offset=128 ... If not, it will point to fwd declaration record: [15439] FWD 'pt_regs' fwd_kind=struct and make bpf tracing program hooking on those functions unable to access fields from 'struct pt_regs'. Including asm/ptrace.h directly in syscall_wrapper.h to make sure all syscalls see 'struct pt_regs' definition and resulted BTF for '__*_sys_*(struct pt_regs *regs)' functions point to actual struct, not just forward declaration. Reported-by: Akihiro HARAI <jharai0815@gmail.com> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- arch/x86/include/asm/syscall_wrapper.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 10/18, Jiri Olsa wrote: > From: Jiri Olsa <olsajiri@gmail.com> > With just the forward declaration of the 'struct pt_regs' in > syscall_wrapper.h, the syscall stub functions: > __[x64|ia32]_sys_*(struct pt_regs *regs) > will have different definition of 'regs' argument in BTF data > based on which object file they are defined in. > If the syscall's object includes 'struct pt_regs' definition, > the BTF argument data will point to 'struct pt_regs' record, > like: > [226] STRUCT 'pt_regs' size=168 vlen=21 > 'r15' type_id=1 bits_offset=0 > 'r14' type_id=1 bits_offset=64 > 'r13' type_id=1 bits_offset=128 > ... > If not, it will point to fwd declaration record: > [15439] FWD 'pt_regs' fwd_kind=struct > and make bpf tracing program hooking on those functions unable > to access fields from 'struct pt_regs'. Is the core issue here is that we can't / don't resolve FWD declarations at load time (or dedup time)? I'm assuming 'struct pt_regs' is still exposed somewhere in BTF via some other obj file, it's just those syscall definitions that have FWD decl? Applying this patch seems fine for now, but also seems fragile. Should we instead/also teach verifier/dedup/whatever to resolve those FWD declarations? > Including asm/ptrace.h directly in syscall_wrapper.h to make > sure all syscalls see 'struct pt_regs' definition and resulted > BTF for '__*_sys_*(struct pt_regs *regs)' functions point to > actual struct, not just forward declaration. > Reported-by: Akihiro HARAI <jharai0815@gmail.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > arch/x86/include/asm/syscall_wrapper.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > diff --git a/arch/x86/include/asm/syscall_wrapper.h > b/arch/x86/include/asm/syscall_wrapper.h > index 59358d1bf880..fd2669b1cb2d 100644 > --- a/arch/x86/include/asm/syscall_wrapper.h > +++ b/arch/x86/include/asm/syscall_wrapper.h > @@ -6,7 +6,7 @@ > #ifndef _ASM_X86_SYSCALL_WRAPPER_H > #define _ASM_X86_SYSCALL_WRAPPER_H > -struct pt_regs; > +#include <asm/ptrace.h> > extern long __x64_sys_ni_syscall(const struct pt_regs *regs); > extern long __ia32_sys_ni_syscall(const struct pt_regs *regs); > -- > 2.37.3
On Tue, Oct 18, 2022 at 11:23:21AM -0700, sdf@google.com wrote: > On 10/18, Jiri Olsa wrote: > > From: Jiri Olsa <olsajiri@gmail.com> > > > With just the forward declaration of the 'struct pt_regs' in > > syscall_wrapper.h, the syscall stub functions: > > > __[x64|ia32]_sys_*(struct pt_regs *regs) > > > will have different definition of 'regs' argument in BTF data > > based on which object file they are defined in. > > > If the syscall's object includes 'struct pt_regs' definition, > > the BTF argument data will point to 'struct pt_regs' record, > > like: > > > [226] STRUCT 'pt_regs' size=168 vlen=21 > > 'r15' type_id=1 bits_offset=0 > > 'r14' type_id=1 bits_offset=64 > > 'r13' type_id=1 bits_offset=128 > > ... > > > If not, it will point to fwd declaration record: > > > [15439] FWD 'pt_regs' fwd_kind=struct > > > and make bpf tracing program hooking on those functions unable > > to access fields from 'struct pt_regs'. > > Is the core issue here is that we can't / don't resolve FWD declarations > at load time (or dedup time)? I'm assuming 'struct pt_regs' is still > exposed somewhere in BTF via some other obj file, it's just those > syscall definitions that have FWD decl? yes, BTF is generated from dwarf debug info, so it's object based, and in some objects the regs argument will point to full struct definition and in some just to forward declaration > > Applying this patch seems fine for now, but also seems fragile. Should > we instead/also teach verifier/dedup/whatever to resolve those FWD > declarations? I'm not sure how hard it'd be to connect forward declarations to definitions.. it'd need to be probably in dedup or verifier as you suggest and I think it'd need to be done just based on struct name search, so I expect all sort of unexpected problems ;-) other than to be sure you connect to proper struct Andrii will have probably better idea if and where this is possible jirka > > > Including asm/ptrace.h directly in syscall_wrapper.h to make > > sure all syscalls see 'struct pt_regs' definition and resulted > > BTF for '__*_sys_*(struct pt_regs *regs)' functions point to > > actual struct, not just forward declaration. > > > Reported-by: Akihiro HARAI <jharai0815@gmail.com> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > arch/x86/include/asm/syscall_wrapper.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > diff --git a/arch/x86/include/asm/syscall_wrapper.h > > b/arch/x86/include/asm/syscall_wrapper.h > > index 59358d1bf880..fd2669b1cb2d 100644 > > --- a/arch/x86/include/asm/syscall_wrapper.h > > +++ b/arch/x86/include/asm/syscall_wrapper.h > > @@ -6,7 +6,7 @@ > > #ifndef _ASM_X86_SYSCALL_WRAPPER_H > > #define _ASM_X86_SYSCALL_WRAPPER_H > > > -struct pt_regs; > > +#include <asm/ptrace.h> > > > extern long __x64_sys_ni_syscall(const struct pt_regs *regs); > > extern long __ia32_sys_ni_syscall(const struct pt_regs *regs); > > -- > > 2.37.3 >
On Wed, Oct 19, 2022 at 2:30 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Tue, Oct 18, 2022 at 11:23:21AM -0700, sdf@google.com wrote: > > On 10/18, Jiri Olsa wrote: > > > From: Jiri Olsa <olsajiri@gmail.com> > > > > > With just the forward declaration of the 'struct pt_regs' in > > > syscall_wrapper.h, the syscall stub functions: > > > > > __[x64|ia32]_sys_*(struct pt_regs *regs) > > > > > will have different definition of 'regs' argument in BTF data > > > based on which object file they are defined in. > > > > > If the syscall's object includes 'struct pt_regs' definition, > > > the BTF argument data will point to 'struct pt_regs' record, > > > like: > > > > > [226] STRUCT 'pt_regs' size=168 vlen=21 > > > 'r15' type_id=1 bits_offset=0 > > > 'r14' type_id=1 bits_offset=64 > > > 'r13' type_id=1 bits_offset=128 > > > ... > > > > > If not, it will point to fwd declaration record: > > > > > [15439] FWD 'pt_regs' fwd_kind=struct > > > > > and make bpf tracing program hooking on those functions unable > > > to access fields from 'struct pt_regs'. > > > > Is the core issue here is that we can't / don't resolve FWD declarations > > at load time (or dedup time)? I'm assuming 'struct pt_regs' is still > > exposed somewhere in BTF via some other obj file, it's just those > > syscall definitions that have FWD decl? > > yes, BTF is generated from dwarf debug info, so it's object based, > and in some objects the regs argument will point to full struct > definition and in some just to forward declaration > > > > > Applying this patch seems fine for now, but also seems fragile. Should > > we instead/also teach verifier/dedup/whatever to resolve those FWD > > declarations? > > I'm not sure how hard it'd be to connect forward declarations > to definitions.. it'd need to be probably in dedup or verifier > as you suggest > > and I think it'd need to be done just based on struct name search, > so I expect all sort of unexpected problems ;-) other than to be > sure you connect to proper struct exactly, which is why BTF dedup algorithm resolves FWD to STRUCT/UNION only when we have to outer structs/unions that we are deduplicating, and one of their field points to FWD on one side and STRUCT on another side. Looking up by name is ambiguous and could be incorrect, so BTF dedup needs sort of a "structural proof". As far as the patch goes, it's a good thing to have more complete types wherever possible, so: Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Andrii will have probably better idea if and where this is possible > > jirka > > > > > > Including asm/ptrace.h directly in syscall_wrapper.h to make > > > sure all syscalls see 'struct pt_regs' definition and resulted > > > BTF for '__*_sys_*(struct pt_regs *regs)' functions point to > > > actual struct, not just forward declaration. > > > > > Reported-by: Akihiro HARAI <jharai0815@gmail.com> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > > arch/x86/include/asm/syscall_wrapper.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > diff --git a/arch/x86/include/asm/syscall_wrapper.h > > > b/arch/x86/include/asm/syscall_wrapper.h > > > index 59358d1bf880..fd2669b1cb2d 100644 > > > --- a/arch/x86/include/asm/syscall_wrapper.h > > > +++ b/arch/x86/include/asm/syscall_wrapper.h > > > @@ -6,7 +6,7 @@ > > > #ifndef _ASM_X86_SYSCALL_WRAPPER_H > > > #define _ASM_X86_SYSCALL_WRAPPER_H > > > > > -struct pt_regs; > > > +#include <asm/ptrace.h> > > > > > extern long __x64_sys_ni_syscall(const struct pt_regs *regs); > > > extern long __ia32_sys_ni_syscall(const struct pt_regs *regs); > > > -- > > > 2.37.3 > >
On Tue, 18 Oct 2022, at 13:27, Jiri Olsa wrote: > From: Jiri Olsa <olsajiri@gmail.com> > > With just the forward declaration of the 'struct pt_regs' in > syscall_wrapper.h, the syscall stub functions: > > __[x64|ia32]_sys_*(struct pt_regs *regs) I think arm64 has a similar problem: https://elixir.bootlin.com/linux/v6.1-rc2/source/arch/arm64/include/asm/syscall_wrapper.h#L11 Best Lorenz
diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h index 59358d1bf880..fd2669b1cb2d 100644 --- a/arch/x86/include/asm/syscall_wrapper.h +++ b/arch/x86/include/asm/syscall_wrapper.h @@ -6,7 +6,7 @@ #ifndef _ASM_X86_SYSCALL_WRAPPER_H #define _ASM_X86_SYSCALL_WRAPPER_H -struct pt_regs; +#include <asm/ptrace.h> extern long __x64_sys_ni_syscall(const struct pt_regs *regs); extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);