Re: [PATCH bpf-next 0/2] Fix bpf_perf_event_data ABI breakage

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Sun, 2022-02-06 at 22:23 -0800, Andrii Nakryiko wrote:
> On Sun, Feb 6, 2022 at 11:57 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Sun, 2022-02-06 at 11:31 -0800, Andrii Nakryiko wrote:
> > > On Sun, Feb 6, 2022 at 6:54 AM Ilya Leoshkevich
> > > <iii@linux.ibm.com>
> > > wrote:
> > > > 
> > > > libbpf CI noticed that my recent changes broke
> > > > bpf_perf_event_data
> > > > ABI
> > > > on s390 [1]. Testing shows that they introduced a similar
> > > > breakage
> > > > on
> > > > arm64. The problem is that we are not allowed to extend
> > > > user_pt_regs,
> > > > since it's used by bpf_perf_event_data.
> > > > 
> > > > This series fixes these problems by removing the new members
> > > > and
> > > > introducing user_pt_regs_v2 instead.
> > > > 
> > > > [1] https://github.com/libbpf/libbpf/runs/5079938810
> > > > 
> > > > Ilya Leoshkevich (2):
> > > >   s390/bpf: Introduce user_pt_regs_v2
> > > >   arm64/bpf: Introduce struct user_pt_regs_v2
> > > 
> > > Given it is bpf_perf_event_data and thus bpf_user_pt_regs_t
> > > definitions that are set in stone now, wouldn't it be better to
> > > instead just change
> > > 
> > > typedef user_pt_regs bpf_user_pt_regs_t; (s390x)
> > > typedef struct user_pt_regs bpf_user_pt_regs_t; (arm64)
> > > 
> > > to just define that fixed layout instead of reusing
> > > user_ptr_regs?
> > > 
> > > This whole v2 business looks really ugly.
> > 
> > Wouldn't it break compilation of code like this?
> > 
> >     bpf_perf_event_data data;
> >     user_pt_regs *regs = &data.regs;
> 
> why would it break? user_pt_regs gained extra fields at the end, so
> whoever works with the assumption of an old definition of
> user_pt_regs
> *through pointer* should be totally fine. The problem with
> bpf_perf_event_data is that user_pt_regs are embedded in the struct
> directly, so adding anything to it changes bpf_perf_event_data
> layout.

I meant only building from source, at runtime it should be fine. At
compile time, the compiler should at least warn that pointer types
don't match.

> I, of course, can't know if this breaks any other use case (including
> ones you mentioned below), but using user_pt_regs_v2 will probably
> not
> work with CO-RE, because older kernels won't have such type defined
> (and thus relocations will fail).
> 
> I'm not sure the origins of the need for user_pt_regs (as opposed to
> using pt_regs directly, like x86-64 does), but with CO-RE and
> vmlinux.h it would be more reliable and straightforward to just stick
> to kernel-internal struct pt_regs everywhere. And for non-CO-RE
> macros
> maybe just using an offset within struct pt_regs (i.e.,
> offsetofend(gprs)) would do it?

offsetofend sounds like a nice compromise. I'll give it a try, thanks.

> > 
> > Additionaly, after this I'm no longer sure I haven't missed any
> > other
> > places where user_pt_regs might be used. For example, arm64 seems
> > to be
> > using it not only for BPF, but also for ptrace?
> > 
> > static int gpr_get(struct task_struct *target,
> >                    const struct user_regset *regset,
> >                    struct membuf to)
> > {
> >         struct user_pt_regs *uregs = &task_pt_regs(target)-
> > >user_regs;
> >         return membuf_write(&to, uregs, sizeof(*uregs));
> > }
> > 
> > and then in e.g. gdb:
> > 
> > static void
> > aarch64_fill_gregset (struct regcache *regcache, void *buf)
> > {
> >   struct user_pt_regs *regset = (struct user_pt_regs *) buf;
> >   ...
> > 
> > I'm also not a big fan of the _v2 solution, but it looked the
> > safest
> > to me. At least for s390, a viable alternative that Vasily proposed
> > would be to go ahead with replacing args[1] with orig_gpr2 and then
> > also backporting the patch, so that the new libbpf would still work
> > on
> > the old stable kernels. But this won't work for arm64.