On Sun, 2022-02-06 at 22:23 -0800, Andrii Nakryiko wrote: > On Sun, Feb 6, 2022 at 11:57 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> > 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@xxxxxxxxxxxxx> > > > 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.
