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:   Fri, 8 Jul 2022 09:24:22 +0100
From:   Jean-Philippe Brucker <jean-philippe@linaro.org>
To:     Xu Kuohai <xukuohai@huawei.com>
Cc:     bpf@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        Mark Rutland <mark.rutland@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Alexei Starovoitov <ast@kernel.org>,
        Zi Shen Lim <zlim.lnx@gmail.com>,
        Andrii Nakryiko <andrii@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@kernel.org>,
        "David S . Miller" <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        David Ahern <dsahern@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H . Peter Anvin" <hpa@zytor.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Russell King <rmk+kernel@armlinux.org.uk>,
        James Morse <james.morse@arm.com>,
        Hou Tao <houtao1@huawei.com>,
        Jason Wang <wangborong@cdjrlc.com>
Subject: Re: [PATCH bpf-next v6 4/4] bpf, arm64: bpf trampoline for arm64
Message-ID: <YsfptiexC0wFABFL@myrica>
References: <20220625161255.547944-1-xukuohai@huawei.com>
 <20220625161255.547944-5-xukuohai@huawei.com>
 <YscL4t1pYHYApIiK@larix>
 <a24109d5-b79a-99de-0fd5-66b0ec34e5ed@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a24109d5-b79a-99de-0fd5-66b0ec34e5ed@huawei.com>
Precedence: bulk

On Fri, Jul 08, 2022 at 12:35:33PM +0800, Xu Kuohai wrote:
> >> +
> >> +	emit(A64_ADD_I(1, A64_R(0), A64_SP, args_off), ctx);
> >> +	if (!p->jited)
> >> +		emit_addr_mov_i64(A64_R(1), (const u64)p->insnsi, ctx);
> >> +
> >> +	emit_call((const u64)p->bpf_func, ctx);
> >> +
> >> +	/* store return value */
> >> +	if (save_ret)
> >> +		emit(A64_STR64I(r0, A64_SP, retval_off), ctx);
> > 
> > Here too I think it should be x0. I'm guessing r0 may work for jitted
> > functions but not interpreted ones
> > 
> 
> Yes, r0 is only correct for jitted code, will fix it to:
> 
> if (save_ret)
>         emit(A64_STR64I(p->jited ? r0 : A64_R(0), A64_SP, retval_off),
>              ctx);

I don't think we need this test because x0 should be correct in all cases.
x7 happens to equal x0 when jitted due to the way build_epilogue() builds
the function at the moment, but we shouldn't rely on that.


> >> +	if (flags & BPF_TRAMP_F_CALL_ORIG) {
> >> +		restore_args(ctx, args_off, nargs);
> >> +		/* call original func */
> >> +		emit(A64_LDR64I(A64_R(10), A64_SP, retaddr_off), ctx);
> >> +		emit(A64_BLR(A64_R(10)), ctx);
> > 
> > I don't think we can do this when BTI is enabled because we're not jumping
> > to a BTI instruction. We could introduce one in a patched BPF function
> > (there currently is one if CONFIG_ARM64_PTR_AUTH_KERNEL), but probably not
> > in a kernel function.
> > 
> > We could fo like FUNCTION_GRAPH_TRACER does and return to the patched
> > function after modifying its LR. Not sure whether that works with pointer
> > auth though.
> > 
> 
> Yes, the blr instruction should be replaced with ret instruction, thanks!
> 
> The layout for bpf prog and regular kernel function is as follows, with
> bti always coming first and paciasp immediately after patchsite, so the
> ret instruction should work in all cases.
> 
> bpf prog or kernel function:
>         bti c // if BTI
>         mov x9, lr
>         bl <trampoline>    ------> trampoline:
>                                            ...
>                                            mov lr, <return_entry>
>                                            mov x10, <ORIG_CALL_entry>
> ORIG_CALL_entry:           <-------        ret x10
>                                    return_entry:
>                                            ...
>         paciasp // if PA
>         ...

Actually I just noticed that CONFIG_ARM64_BTI_KERNEL depends on
CONFIG_ARM64_PTR_AUTH_KERNEL, so we should be able to rely on there always
being a PACIASP at ORIG_CALL_entry, and since it's a landing pad for BLR
we don't need to make this a RET

 92e2294d870b ("arm64: bti: Support building kernel C code using BTI")

Thanks,
Jean