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

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

[Ilya Leoshkevich]

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

 arch/arm64/include/asm/ptrace.h      |  1 +
 arch/arm64/include/uapi/asm/ptrace.h |  7 +++++++
 arch/s390/include/asm/ptrace.h       |  1 +
 arch/s390/include/uapi/asm/ptrace.h  | 10 ++++++++--
 tools/lib/bpf/bpf_tracing.h          | 10 ++++++----
 5 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH bpf-next 1/2] s390/bpf: Introduce user_pt_regs_v2

[Ilya Leoshkevich]

Extending user_pt_regs breaks struct bpf_perf_event_data ABI, so
instead of exposing orig_gpr2 through it, create its copy with
orig_gpr2 at the end and use it in libbpf.

The existing members are copy-pasted, so now there are 3 copies in
total. It might be tempting to add a user_pt_regs member to
user_pt_regs_v2 instead, however, there is no guarantee that then
user_pt_regs_v2.orig_gpr2 would be at the same offset as
pt_regs.orig_gpr2.

Fixes: 61f88e88f263 ("s390/bpf: Add orig_gpr2 to user_pt_regs")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/include/asm/ptrace.h      |  1 +
 arch/s390/include/uapi/asm/ptrace.h | 10 ++++++++--
 tools/lib/bpf/bpf_tracing.h         |  4 ++--
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h
index c8698e643904..1a08f36395e5 100644
--- a/arch/s390/include/asm/ptrace.h
+++ b/arch/s390/include/asm/ptrace.h
@@ -79,6 +79,7 @@ enum {
 struct pt_regs {
 	union {
 		user_pt_regs user_regs;
+		user_pt_regs_v2 user_regs_v2;
 		struct {
 			unsigned long args[1];
 			psw_t psw;
diff --git a/arch/s390/include/uapi/asm/ptrace.h b/arch/s390/include/uapi/asm/ptrace.h
index b3dec603f507..b9405b8f0d47 100644
--- a/arch/s390/include/uapi/asm/ptrace.h
+++ b/arch/s390/include/uapi/asm/ptrace.h
@@ -288,16 +288,22 @@ typedef struct {
 } s390_regs;
 
 /*
- * The user_pt_regs structure exports the beginning of
+ * The user_pt_regs and user_pt_regs_v2 structures export the beginning of
  * the in-kernel pt_regs structure to user space.
  */
 typedef struct {
 	unsigned long args[1];
 	psw_t psw;
 	unsigned long gprs[NUM_GPRS];
-	unsigned long orig_gpr2;
 } user_pt_regs;
 
+typedef struct {
+	unsigned long args[1];
+	psw_t psw;
+	unsigned long gprs[NUM_GPRS];
+	unsigned long orig_gpr2;
+} user_pt_regs_v2;
+
 /*
  * Now for the user space program event recording (trace) definitions.
  * The following structures are used only for the ptrace interface, don't
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index cf980e54d331..76abbc5ff2e8 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -114,8 +114,8 @@
 
 #elif defined(bpf_target_s390)
 
-/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
-#define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))
+/* s390 provides user_pt_regs_v2 instead of struct pt_regs to userspace */
+#define __PT_REGS_CAST(x) ((const user_pt_regs_v2 *)(x))
 #define __PT_PARM1_REG gprs[2]
 #define __PT_PARM1_REG_SYSCALL orig_gpr2
 #define __PT_PARM2_REG gprs[3]
-- 
2.34.1

[PATCH bpf-next 2/2] arm64/bpf: Introduce struct user_pt_regs_v2

[Ilya Leoshkevich]

Extending struct user_pt_regs breaks struct bpf_perf_event_data ABI, so
instead of exposing orig_x0 through it, create its copy with orig_x0 at
the end and use it in libbpf.

The existing members are copy-pasted, so now there are 3 copies in
total. It might be tempting to add a user_pt_regs member to
user_pt_regs_v2 instead, however, there is no guarantee that then
user_pt_regs_v2.orig_x0 would be at the same offset as
pt_regs.orig_gpr2.

Fixes: d473f4062165 ("arm64/bpf: Add orig_x0 to user_pt_regs")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/arm64/include/asm/ptrace.h      | 1 +
 arch/arm64/include/uapi/asm/ptrace.h | 7 +++++++
 tools/lib/bpf/bpf_tracing.h          | 6 ++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 1be22f7870f8..c5e098af5b70 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -178,6 +178,7 @@ static inline unsigned long pstate_to_compat_psr(const unsigned long pstate)
 struct pt_regs {
 	union {
 		struct user_pt_regs user_regs;
+		struct user_pt_regs_v2 user_regs_v2;
 		struct {
 			u64 regs[31];
 			u64 sp;
diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
index 3c118c5b0893..ab7a2f0cdca8 100644
--- a/arch/arm64/include/uapi/asm/ptrace.h
+++ b/arch/arm64/include/uapi/asm/ptrace.h
@@ -90,6 +90,13 @@ struct user_pt_regs {
 	__u64		sp;
 	__u64		pc;
 	__u64		pstate;
+};
+
+struct user_pt_regs_v2 {
+	__u64		regs[31];
+	__u64		sp;
+	__u64		pc;
+	__u64		pstate;
 	__u64		orig_x0;
 };
 
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 76abbc5ff2e8..284cc4d6954e 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -143,8 +143,10 @@
 
 #elif defined(bpf_target_arm64)
 
-/* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
-#define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
+/*
+ * arm64 provides struct user_pt_regs_v2 instead of struct pt_regs to userspace
+ */
+#define __PT_REGS_CAST(x) ((const struct user_pt_regs_v2 *)(x))
 #define __PT_PARM1_REG regs[0]
 #define __PT_PARM1_REG_SYSCALL orig_x0
 #define __PT_PARM2_REG regs[1]
-- 
2.34.1

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

[Andrii Nakryiko]

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.


>
>  arch/arm64/include/asm/ptrace.h      |  1 +
>  arch/arm64/include/uapi/asm/ptrace.h |  7 +++++++
>  arch/s390/include/asm/ptrace.h       |  1 +
>  arch/s390/include/uapi/asm/ptrace.h  | 10 ++++++++--
>  tools/lib/bpf/bpf_tracing.h          | 10 ++++++----
>  5 files changed, 23 insertions(+), 6 deletions(-)
>
> --
> 2.34.1
>

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

[Ilya Leoshkevich]

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;

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.

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

[Andrii Nakryiko]

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

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

[Heiko Carstens]

On Sun, Feb 06, 2022 at 10:23:19PM -0800, Andrii Nakryiko wrote:
> 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?

user_pt_regs was introduced on s390 because struct pt_regs is _not_
stable. Only the first n entries (aka user_pt_regs) are supposed to be
stable.

We could of course reduce struct pt_regs to the bare minimum, which seems
to be the current user_pt_regs plus orig_gpr2; which semantically would
match more or less what x86 has.

Then move pt_regs to uapi, so it is clear that it cannot be changed
anymore, and have additional data in a separate structure on the stack,
which has pt_regs at the beginning, and access this additional data with
container_of & friends.

I guess that could work, even though this requires to keep user_pt_regs
"for historical reasons".

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

[Andrii Nakryiko]

On Mon, Feb 7, 2022 at 1:46 AM Heiko Carstens <hca@linux.ibm.com> wrote:
>
> On Sun, Feb 06, 2022 at 10:23:19PM -0800, Andrii Nakryiko wrote:
> > 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?
>
> user_pt_regs was introduced on s390 because struct pt_regs is _not_
> stable. Only the first n entries (aka user_pt_regs) are supposed to be
> stable.
>
> We could of course reduce struct pt_regs to the bare minimum, which seems
> to be the current user_pt_regs plus orig_gpr2; which semantically would
> match more or less what x86 has.
>
> Then move pt_regs to uapi, so it is clear that it cannot be changed
> anymore, and have additional data in a separate structure on the stack,
> which has pt_regs at the beginning, and access this additional data with
> container_of & friends.
>
> I guess that could work, even though this requires to keep user_pt_regs
> "for historical reasons".

It's just surprising and constraining that UAPI struct can be extended
by adding new fields at the end :( I'll let you guys decide it for
s390 and arm64. From libbpf's side, we have somewhat hacky ways to
work around that, it seems.

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

[Ilya Leoshkevich]

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.

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

[Andrii Nakryiko]

On Mon, Feb 7, 2022 at 3:52 AM Ilya Leoshkevich <iii@linux.ibm.com> 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@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.

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