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; 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.
