On Mon, Feb 7, 2022 at 3:52 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote: > > 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. Oh, you meant that cast would be necessary. Well, strictly speaking code like in your example is broken, it should use the type specified in struct bpf_perf_event_data: bpf_user_pt_regs_t. But the fix to satisfy compilation is trivial as well, so doesn't matter much. > > > 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. It's kind of dangerous as well, let's maybe leave a comment in pt_regs that this orig_gpr2 location is assumed by libbpf's tracing macros so shouldn't be willy-nilly moved > > > > > > > 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. >
