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, 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? > > 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.
