Received-SPF: pass (google.com: domain of linux-kernel+bounces-126956-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240329094906.18147-1-ubizjak@gmail.com> <20240329094906.18147-3-ubizjak@gmail.com>
 <CAADnVQ+6D++hCXaP=aK+Q5wienMzhHo3h9YCvpA_7sHjMt+q6A@mail.gmail.com>
 <CAFULd4b6juiw3wC3Z61V9=-UnA+NGyUt4231vC14UnGAATk6tA@mail.gmail.com> <CAADnVQKqxQxLLQ6OwZCWni8==-fioeOZ_AH7YVjjCwvYNYmCqg@mail.gmail.com>
In-Reply-To: <CAADnVQKqxQxLLQ6OwZCWni8==-fioeOZ_AH7YVjjCwvYNYmCqg@mail.gmail.com>
From: Uros Bizjak <ubizjak@gmail.com>
Date: Mon, 1 Apr 2024 20:38:02 +0200
Message-ID: <CAFULd4ZvxSesz0QhMB7FWS-_H_PpEZhkfqHJ0kmhQUkgL=ASfQ@mail.gmail.com>
Subject: Re: [PATCH RESEND bpf 2/2] x86/bpf: Fix IP for relocating call depth accounting
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: X86 ML <x86@kernel.org>, bpf <bpf@vger.kernel.org>, 
	Network Development <netdev@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	=?UTF-8?Q?Joan_Bruguera_Mic=C3=B3?= <joanbrugueram@gmail.com>, 
	Ingo Molnar <mingo@kernel.org>, Alexei Starovoitov <ast@kernel.org>, 
	Daniel Borkmann <daniel@iogearbox.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 1, 2024 at 8:03=E2=80=AFPM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sat, Mar 30, 2024 at 2:01=E2=80=AFAM Uros Bizjak <ubizjak@gmail.com> w=
rote:
> >
> > On Fri, Mar 29, 2024 at 10:53=E2=80=AFPM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Mar 29, 2024 at 2:49=E2=80=AFAM Uros Bizjak <ubizjak@gmail.co=
m> wrote:
> > > >
> > > > From: Joan Bruguera Mic=C3=B3 <joanbrugueram@gmail.com>
> > > >
> > > > The recently introduced support for %rip-relative relocations in th=
e
> > > > call thunk template assumes that the code is being patched in-place=
,
> > > > so the destination of the relocation matches the address of the cod=
e.
> > > > This is not true for the call depth accounting emitted by the BPF J=
IT,
> > > > so the calculated address is wrong and usually causes a page fault.
> > >
> > > Could you share the link to what this 'rip-relative' relocation is ?
> >
> > Please see the "RIP relative addressing" section in [1].
> >
> > [1] https://compas.cs.stonybrook.edu/~nhonarmand/courses/sp17/cse506/re=
f/assembly.html
> >
> > In our case:
> >
> > The callthunks patching creates a call thunk template in the .rodata
> > section (please see arch/x86/kernel/callthunks.c)  that is later
> > copied to the .text section at the correct place. The template uses
> > X86_call_depth in the pcpu_hot structure. Previously, the template
> > used absolute location for X86_call_depth and the linker resolved the
> > address in the template to this absolute location. There is no issue
> > when this template is copied to the various places in the .text
> > section.
> >
> > When we want to use PC relative relocations (to reduce the code size),
> > then the linker calculates the address of the variable in the template
> > according to the PC in the .rodata section. If we want to copy the
> > template to its final location, then the address of X86_call_depth,
> > relative to the PC, has to be adjusted, as explained in
> > arch/x86/kernel/alternative.c, in the comment above apply_reloc_n
> > macro.
>
> I didn't mean to ask for info about the definition of rip-relative,
> but how it's used in this case and what you've been trying
> to achieve with commit 17bce3b2ae2d that broke call depth accounting.
> And the whole sequence of events that caused this breakage.
> Something like:
> commit 59bec00ace28 ("x86/percpu: Introduce %rip-relative addressing
> to PER_CPU_VAR()")
> made PER_CPU_VAR() to use rip-relative addressing,
> hence INCREMENT_CALL_DEPTH macro and skl_call_thunk_template
> got rip-relative asm code inside of it.
> Hence x86_call_depth_emit_accounting() was changed
> in commit 17bce3b2ae2d ("x86/callthunks: Handle %rip-relative
> relocations in call thunk template") to use apply_relocation(),
> but it was mistakenly made to use *pprog as dest ip,
> so jit-ed bpf progs on kernels with call depth tracking got broken.
> Such details should be in the commit log.

Oh, I was not sure that all those x86 specific details should be in
the commit log, since x86 maintainers already acked the patch. Sure,
I'll add your description of the fix to the patch commit message, it
really describes the problem in a way, understandable also to non-x86
people.

Thanks,
Uros.