On Tue, 2022-02-08 at 13:11 -0800, Alexei Starovoitov wrote:
> On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> wrote:
> >
> > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote:
> > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote:
> > > > orig_x0's location is used by libbpf tracing macros, therefore
> > > > it
> > > > should not be moved.
> > > >
> > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> > > > Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > > > ---
> > > > arch/arm64/include/asm/ptrace.h | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/include/asm/ptrace.h
> > > > b/arch/arm64/include/asm/ptrace.h
> > > > index 41b332c054ab..7e34c3737839 100644
> > > > --- a/arch/arm64/include/asm/ptrace.h
> > > > +++ b/arch/arm64/include/asm/ptrace.h
> > > > @@ -185,6 +185,10 @@ struct pt_regs {
> > > > u64 pstate;
> > > > };
> > > > };
> > > > + /*
> > > > + * orig_x0 is not exposed via struct user_pt_regs, but
> > > > its
> > > > location is
> > > > + * assumed by libbpf's tracing macros, so it should not
> > > > be
> > > > moved.
> > > > + */
> > >
> > > In other words this comment is saying that the layout is ABI.
> > > That's not the case. orig_x0 here and equivalent on s390 can be
> > > moved.
> > > It will break bpf progs written without CO-RE and that is
> > > expected.
> > > Non CO-RE programs often do all kinds of bpf_probe_read_kernel
> > > and
> > > will be breaking when kernel layout is changing.
> > > I suggest to drop this patch and patch 12.
> >
> > Yeah, that was the intention here: to promote orig_x0 to ABI using
> > a
> > comment, since doing this by extending user_pt_regs turned out to
> > be
> > infeasible. I'm actually ok with not doing this, since programs
> > compiled with kernel headers and using CO-RE macros will be fine.
>
> The comment like this doesn't convert kernel internal struct into
> ABI.
> The comment is just wrong. BPF progs access many kernel data structs.
> s390's and arm64's struct pr_regs is not special in that sense.
> It's an internal struct.
>
> > As you say, we don't care about programs that don't use CO-RE too
> > much
> > here - if they break after an incompatible kernel change, fine.
>
> Before CO-RE was introduced bpf progs included kernel headers
> and were breaking when kernel changes. Nothing new here.
> See the history of bcc tools. Some of them are littered
> with ifdef VERSION ==.
>
> > The question now is - how much do we care about programs that are
>
> > compiled with userspace headers? Andrii suggested to use
> > offsetofend to
> > make syscall macros work there, however, this now requires this ABI
> > promotion.
>
> Today s390 and arm64 have user_pt_regs as a first field in pt_regs.
> That is kernel internal behavior and that part can change if arch
> maintainers have a need for that.
> bpf progs without CO-RE would have to be adjusted when kernel
> changes.
> Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390.
> The progs with CO-RE will break too. The authors of tracing bpf progs
> have to expect that sooner or later their progs will break and they
> would have to adjust them.
When it comes to authors of tracing bpf progs, I agree that eventually
they will have to adjust their code, CO-RE or not. However, in patch 13
I introduce the following libbpf macro:
#if defined(__KERNEL__) || defined(__VMLINUX_H__)
...
#else
#define PT_REGS_PARM1_SYSCALL(x) \
(*(unsigned long *)(((char *)(x) + \
offsetofend(struct user_pt_regs,
pstate))))
If we merge this series without freezing orig_x0's offset, in case of
an incompatible kernel change the users of PT_REGS_PARMn_SYSCALL and
BPF_KPROBE_SYSCALL, who build against userspace headers, will not
simply have to update their code - they will have to upgrade libbpf.
It's also not clear how PT_REGS_PARM1_SYSCALL in the upgraded libbpf
will even look like, given that it would need to work on both old and
new kernels.
I've also briefly looked into MIPS' ptrace.h, and it looks as if their
user_pt_regs has no relationship to kernel pt_regs. IIUC this means
that it's not possible to correctly implement PT_REGS_PARMn_SYSCALL
using MIPS userspace headers.
So I wonder whether we should allow using PT_REGS_PARMn_SYSCALL and
BPF_KPROBE_SYSCALL with userspace headers at all? Would it make sense
to simply fail the compilation if PT_REGS_PARMn_SYSCALL is used without
including kernel headers?
