From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 16555C433EF for ; Wed, 11 May 2022 03:12:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239546AbiEKDMg (ORCPT ); Tue, 10 May 2022 23:12:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59520 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236978AbiEKDMd (ORCPT ); Tue, 10 May 2022 23:12:33 -0400 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D3E9047560; Tue, 10 May 2022 20:12:31 -0700 (PDT) Received: from kwepemi500013.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Kyg1L3PrHzhZ1r; Wed, 11 May 2022 11:11:50 +0800 (CST) Received: from [10.67.111.192] (10.67.111.192) by kwepemi500013.china.huawei.com (7.221.188.120) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Wed, 11 May 2022 11:12:27 +0800 Message-ID: <5fb30cc0-dcf6-75ec-b6fa-38be3e99dca6@huawei.com> Date: Wed, 11 May 2022 11:12:27 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH bpf-next v3 5/7] bpf, arm64: Support to poke bpf prog Content-Language: en-US To: Jakub Sitnicki CC: , , , , , Catalin Marinas , Will Deacon , Steven Rostedt , Ingo Molnar , Daniel Borkmann , Alexei Starovoitov , Zi Shen Lim , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , "David S . Miller" , Hideaki YOSHIFUJI , David Ahern , Thomas Gleixner , Borislav Petkov , Dave Hansen , , , Shuah Khan , Jakub Kicinski , Jesper Dangaard Brouer , Mark Rutland , Pasha Tatashin , Ard Biesheuvel , Daniel Kiss , Steven Price , Sudeep Holla , Marc Zyngier , Peter Collingbourne , Mark Brown , Delyan Kratunov , Kumar Kartikeya Dwivedi References: <20220424154028.1698685-1-xukuohai@huawei.com> <20220424154028.1698685-6-xukuohai@huawei.com> <87ilqdobl1.fsf@cloudflare.com> From: Xu Kuohai In-Reply-To: <87ilqdobl1.fsf@cloudflare.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.111.192] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemi500013.china.huawei.com (7.221.188.120) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 5/10/2022 5:36 PM, Jakub Sitnicki wrote: > Thanks for incorporating the attach to BPF progs bits into the series. > > I have a couple minor comments. Please see below. > > On Sun, Apr 24, 2022 at 11:40 AM -04, Xu Kuohai wrote: >> 1. Set up the bpf prog entry in the same way as fentry to support >> trampoline. Now bpf prog entry looks like this: >> >> bti c // if BTI enabled >> mov x9, x30 // save lr >> nop // to be replaced with jump instruction >> paciasp // if PAC enabled >> >> 2. Update bpf_arch_text_poke() to poke bpf prog. If the instruction >> to be poked is bpf prog's first instruction, skip to the nop >> instruction in the prog entry. >> >> Signed-off-by: Xu Kuohai >> --- >> arch/arm64/net/bpf_jit.h | 1 + >> arch/arm64/net/bpf_jit_comp.c | 41 +++++++++++++++++++++++++++-------- >> 2 files changed, 33 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h >> index 194c95ccc1cf..1c4b0075a3e2 100644 >> --- a/arch/arm64/net/bpf_jit.h >> +++ b/arch/arm64/net/bpf_jit.h >> @@ -270,6 +270,7 @@ >> #define A64_BTI_C A64_HINT(AARCH64_INSN_HINT_BTIC) >> #define A64_BTI_J A64_HINT(AARCH64_INSN_HINT_BTIJ) >> #define A64_BTI_JC A64_HINT(AARCH64_INSN_HINT_BTIJC) >> +#define A64_NOP A64_HINT(AARCH64_INSN_HINT_NOP) >> >> /* DMB */ >> #define A64_DMB_ISH aarch64_insn_gen_dmb(AARCH64_INSN_MB_ISH) >> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c >> index 3f9bdfec54c4..293bdefc5d0c 100644 >> --- a/arch/arm64/net/bpf_jit_comp.c >> +++ b/arch/arm64/net/bpf_jit_comp.c >> @@ -237,14 +237,23 @@ static bool is_lsi_offset(int offset, int scale) >> return true; >> } >> >> -/* Tail call offset to jump into */ >> -#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) || \ >> - IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) >> -#define PROLOGUE_OFFSET 9 >> +#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) >> +#define BTI_INSNS 1 >> +#else >> +#define BTI_INSNS 0 >> +#endif >> + >> +#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) >> +#define PAC_INSNS 1 >> #else >> -#define PROLOGUE_OFFSET 8 >> +#define PAC_INSNS 0 >> #endif > > Above can be folded into: > > #define BTI_INSNS (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) ? 1 : 0) > #define PAC_INSNS (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) ? 1 : 0) > will fix in v4 >> >> +/* Tail call offset to jump into */ >> +#define PROLOGUE_OFFSET (BTI_INSNS + 2 + PAC_INSNS + 8) >> +/* Offset of nop instruction in bpf prog entry to be poked */ >> +#define POKE_OFFSET (BTI_INSNS + 1) >> + >> static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf) >> { >> const struct bpf_prog *prog = ctx->prog; >> @@ -281,12 +290,15 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf) >> * >> */ >> >> + if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)) >> + emit(A64_BTI_C, ctx); > > I'm no arm64 expert, but this looks like a fix for BTI. > > Currently we never emit BTI because ARM64_BTI_KERNEL depends on > ARM64_PTR_AUTH_KERNEL, while BTI must be the first instruction for the > jump target [1]. Am I following correctly? > > [1] https://lwn.net/Articles/804982/ > Not quite correct. When the jump target is a PACIASP instruction, no Branch Target Exception is generated, so there is no need to insert a BTI before PACIASP [2]. In order to attach trampoline to bpf prog, a MOV and NOP are inserted before the PACIASP, so BTI instruction is required to avoid Branch Target Exception. The reason for inserting NOP before PACIASP instead of after PACIASP is that no call frame is built before entering trampoline, so there is no return address on the stack and nothing to be protected by PACIASP. [2] https://developer.arm.com/documentation/ddi0596/2021-12/Base-Instructions/BTI--Branch-Target-Identification-?lang=en >> + >> + emit(A64_MOV(1, A64_R(9), A64_LR), ctx); >> + emit(A64_NOP, ctx); >> + >> /* Sign lr */ >> if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL)) >> emit(A64_PACIASP, ctx); >> - /* BTI landing pad */ >> - else if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)) >> - emit(A64_BTI_C, ctx); >> >> /* Save FP and LR registers to stay align with ARM64 AAPCS */ >> emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx); >> @@ -1552,9 +1564,11 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type, >> u32 old_insn; >> u32 new_insn; >> u32 replaced; >> + unsigned long offset = ~0UL; >> enum aarch64_insn_branch_type branch_type; >> + char namebuf[KSYM_NAME_LEN]; >> >> - if (!is_bpf_text_address((long)ip)) >> + if (!__bpf_address_lookup((unsigned long)ip, NULL, &offset, namebuf)) >> /* Only poking bpf text is supported. Since kernel function >> * entry is set up by ftrace, we reply on ftrace to poke kernel >> * functions. For kernel funcitons, bpf_arch_text_poke() is only >> @@ -1565,6 +1579,15 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type, >> */ >> return -EINVAL; >> >> + /* bpf entry */ >> + if (offset == 0UL) >> + /* skip to the nop instruction in bpf prog entry: >> + * bti c // if BTI enabled >> + * mov x9, x30 >> + * nop >> + */ >> + ip = (u32 *)ip + POKE_OFFSET; > > This is very much personal preference, however, I find the use pointer > arithmetic too clever here. Would go for a more verbose: > > offset = POKE_OFFSET * AARCH64_INSN_SIZE; > ip = (void *)((unsigned long)ip + offset); > will change in v4. >> + >> if (poke_type == BPF_MOD_CALL) >> branch_type = AARCH64_INSN_BRANCH_LINK; >> else > > I think it'd make more sense to merge this patch with patch 4 (the > preceding one). > > Initial implementation of of bpf_arch_text_poke() from patch 4 is not > fully functional, as it will always fail for bpf_arch_text_poke(ip, > BPF_MOD_CALL, ...) calls. At least, I find it a bit confusing. will merge in v4 > > Otherwise than that: > > Reviewed-by: Jakub Sitnicki > > . Thanks for the review! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D5EBFC433EF for ; Wed, 11 May 2022 03:13:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:CC:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=AoZb4+rUv6rBTUUcFEvUFnAQ6Kruh46cIeUb/+6xp2w=; b=iiYgcOsXTWQBqx LG9nGMBuBLNewKDUIY8vVDkEKKMHgR9ui3IsZi7vOs+s9YvLJd2HCEvXcKCvWarE5B1TCzppGczWr t30lWfj7SHPW9ro/iL/4aGKuck9zfLKZxV3PXo3R1qznVVVmmNA77JpDs/VsfSbVxEDI8+E0eZnG7 DBS4Z6r1yaejkhPExiZccoVwlNkt8hX9rBimNg4kuiGXHFLHtwCxEafFKyl2hLXlC1SudDI10EBe+ 3hdIYPrU4EmHTqZaD79fAp0NDS0K1mulj4KpLOJdPWRWIHAaTZyJlE7uSARZKid/ifjowBiTuYBKo dYz0f7hTceqQPunbnuhA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nocmH-004xXS-LP; Wed, 11 May 2022 03:12:41 +0000 Received: from szxga02-in.huawei.com ([45.249.212.188]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nocmD-004xUW-6X for linux-arm-kernel@lists.infradead.org; Wed, 11 May 2022 03:12:39 +0000 Received: from kwepemi500013.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Kyg1L3PrHzhZ1r; Wed, 11 May 2022 11:11:50 +0800 (CST) Received: from [10.67.111.192] (10.67.111.192) by kwepemi500013.china.huawei.com (7.221.188.120) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Wed, 11 May 2022 11:12:27 +0800 Message-ID: <5fb30cc0-dcf6-75ec-b6fa-38be3e99dca6@huawei.com> Date: Wed, 11 May 2022 11:12:27 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH bpf-next v3 5/7] bpf, arm64: Support to poke bpf prog Content-Language: en-US To: Jakub Sitnicki CC: , , , , , Catalin Marinas , Will Deacon , Steven Rostedt , Ingo Molnar , Daniel Borkmann , Alexei Starovoitov , Zi Shen Lim , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , "David S . Miller" , Hideaki YOSHIFUJI , David Ahern , Thomas Gleixner , Borislav Petkov , Dave Hansen , , , Shuah Khan , Jakub Kicinski , Jesper Dangaard Brouer , Mark Rutland , Pasha Tatashin , Ard Biesheuvel , Daniel Kiss , Steven Price , Sudeep Holla , Marc Zyngier , Peter Collingbourne , Mark Brown , Delyan Kratunov , Kumar Kartikeya Dwivedi References: <20220424154028.1698685-1-xukuohai@huawei.com> <20220424154028.1698685-6-xukuohai@huawei.com> <87ilqdobl1.fsf@cloudflare.com> From: Xu Kuohai In-Reply-To: <87ilqdobl1.fsf@cloudflare.com> X-Originating-IP: [10.67.111.192] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemi500013.china.huawei.com (7.221.188.120) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220510_201237_620307_83CDBF9C X-CRM114-Status: GOOD ( 29.21 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 5/10/2022 5:36 PM, Jakub Sitnicki wrote: > Thanks for incorporating the attach to BPF progs bits into the series. > > I have a couple minor comments. Please see below. > > On Sun, Apr 24, 2022 at 11:40 AM -04, Xu Kuohai wrote: >> 1. Set up the bpf prog entry in the same way as fentry to support >> trampoline. Now bpf prog entry looks like this: >> >> bti c // if BTI enabled >> mov x9, x30 // save lr >> nop // to be replaced with jump instruction >> paciasp // if PAC enabled >> >> 2. Update bpf_arch_text_poke() to poke bpf prog. If the instruction >> to be poked is bpf prog's first instruction, skip to the nop >> instruction in the prog entry. >> >> Signed-off-by: Xu Kuohai >> --- >> arch/arm64/net/bpf_jit.h | 1 + >> arch/arm64/net/bpf_jit_comp.c | 41 +++++++++++++++++++++++++++-------- >> 2 files changed, 33 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h >> index 194c95ccc1cf..1c4b0075a3e2 100644 >> --- a/arch/arm64/net/bpf_jit.h >> +++ b/arch/arm64/net/bpf_jit.h >> @@ -270,6 +270,7 @@ >> #define A64_BTI_C A64_HINT(AARCH64_INSN_HINT_BTIC) >> #define A64_BTI_J A64_HINT(AARCH64_INSN_HINT_BTIJ) >> #define A64_BTI_JC A64_HINT(AARCH64_INSN_HINT_BTIJC) >> +#define A64_NOP A64_HINT(AARCH64_INSN_HINT_NOP) >> >> /* DMB */ >> #define A64_DMB_ISH aarch64_insn_gen_dmb(AARCH64_INSN_MB_ISH) >> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c >> index 3f9bdfec54c4..293bdefc5d0c 100644 >> --- a/arch/arm64/net/bpf_jit_comp.c >> +++ b/arch/arm64/net/bpf_jit_comp.c >> @@ -237,14 +237,23 @@ static bool is_lsi_offset(int offset, int scale) >> return true; >> } >> >> -/* Tail call offset to jump into */ >> -#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) || \ >> - IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) >> -#define PROLOGUE_OFFSET 9 >> +#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) >> +#define BTI_INSNS 1 >> +#else >> +#define BTI_INSNS 0 >> +#endif >> + >> +#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) >> +#define PAC_INSNS 1 >> #else >> -#define PROLOGUE_OFFSET 8 >> +#define PAC_INSNS 0 >> #endif > > Above can be folded into: > > #define BTI_INSNS (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) ? 1 : 0) > #define PAC_INSNS (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL) ? 1 : 0) > will fix in v4 >> >> +/* Tail call offset to jump into */ >> +#define PROLOGUE_OFFSET (BTI_INSNS + 2 + PAC_INSNS + 8) >> +/* Offset of nop instruction in bpf prog entry to be poked */ >> +#define POKE_OFFSET (BTI_INSNS + 1) >> + >> static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf) >> { >> const struct bpf_prog *prog = ctx->prog; >> @@ -281,12 +290,15 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf) >> * >> */ >> >> + if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)) >> + emit(A64_BTI_C, ctx); > > I'm no arm64 expert, but this looks like a fix for BTI. > > Currently we never emit BTI because ARM64_BTI_KERNEL depends on > ARM64_PTR_AUTH_KERNEL, while BTI must be the first instruction for the > jump target [1]. Am I following correctly? > > [1] https://lwn.net/Articles/804982/ > Not quite correct. When the jump target is a PACIASP instruction, no Branch Target Exception is generated, so there is no need to insert a BTI before PACIASP [2]. In order to attach trampoline to bpf prog, a MOV and NOP are inserted before the PACIASP, so BTI instruction is required to avoid Branch Target Exception. The reason for inserting NOP before PACIASP instead of after PACIASP is that no call frame is built before entering trampoline, so there is no return address on the stack and nothing to be protected by PACIASP. [2] https://developer.arm.com/documentation/ddi0596/2021-12/Base-Instructions/BTI--Branch-Target-Identification-?lang=en >> + >> + emit(A64_MOV(1, A64_R(9), A64_LR), ctx); >> + emit(A64_NOP, ctx); >> + >> /* Sign lr */ >> if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL)) >> emit(A64_PACIASP, ctx); >> - /* BTI landing pad */ >> - else if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)) >> - emit(A64_BTI_C, ctx); >> >> /* Save FP and LR registers to stay align with ARM64 AAPCS */ >> emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx); >> @@ -1552,9 +1564,11 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type, >> u32 old_insn; >> u32 new_insn; >> u32 replaced; >> + unsigned long offset = ~0UL; >> enum aarch64_insn_branch_type branch_type; >> + char namebuf[KSYM_NAME_LEN]; >> >> - if (!is_bpf_text_address((long)ip)) >> + if (!__bpf_address_lookup((unsigned long)ip, NULL, &offset, namebuf)) >> /* Only poking bpf text is supported. Since kernel function >> * entry is set up by ftrace, we reply on ftrace to poke kernel >> * functions. For kernel funcitons, bpf_arch_text_poke() is only >> @@ -1565,6 +1579,15 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type, >> */ >> return -EINVAL; >> >> + /* bpf entry */ >> + if (offset == 0UL) >> + /* skip to the nop instruction in bpf prog entry: >> + * bti c // if BTI enabled >> + * mov x9, x30 >> + * nop >> + */ >> + ip = (u32 *)ip + POKE_OFFSET; > > This is very much personal preference, however, I find the use pointer > arithmetic too clever here. Would go for a more verbose: > > offset = POKE_OFFSET * AARCH64_INSN_SIZE; > ip = (void *)((unsigned long)ip + offset); > will change in v4. >> + >> if (poke_type == BPF_MOD_CALL) >> branch_type = AARCH64_INSN_BRANCH_LINK; >> else > > I think it'd make more sense to merge this patch with patch 4 (the > preceding one). > > Initial implementation of of bpf_arch_text_poke() from patch 4 is not > fully functional, as it will always fail for bpf_arch_text_poke(ip, > BPF_MOD_CALL, ...) calls. At least, I find it a bit confusing. will merge in v4 > > Otherwise than that: > > Reviewed-by: Jakub Sitnicki > > . Thanks for the review! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel