Re: [PATCH bpf-next v4 05/14] libbpf: Generalize overriding syscall parameter access macros

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@linux.ibm.com>,
	Alexander Gordeev <agordeev@linux.ibm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	"Naveen N . Rao" <naveen.n.rao@linux.vnet.ibm.com>,
	Mark Rutland <mark.rutland@arm.com>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v4 05/14] libbpf: Generalize overriding syscall parameter access macros
Date: Tue, 8 Feb 2022 15:21:50 -0800	[thread overview]
Message-ID: <CAEf4BzYjWdp7JA2DY--GQ_miQTnyuAA1XspovvuE+Ui5fAFNxQ@mail.gmail.com> (raw)
In-Reply-To: <566fdad05cb0176b7dfcffb6d99c59567db91c8e.camel@linux.ibm.com>

On Tue, Feb 8, 2022 at 3:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Tue, 2022-02-08 at 14:05 -0800, Andrii Nakryiko wrote:
> > On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > Instead of conditionally overriding PT_REGS_PARM4_SYSCALL, provide
> > > default fallbacks for all __PT_PARMn_REG_SYSCALL macros, so that
> > > architectures can simply override a specific syscall parameter
> > > macro.
> > > Also allow completely overriding PT_REGS_PARM1_SYSCALL for
> > > non-trivial access sequences.
> > >
> > > Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  tools/lib/bpf/bpf_tracing.h | 48 +++++++++++++++++++++++++--------
> > > ----
> > >  1 file changed, 33 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf_tracing.h
> > > b/tools/lib/bpf/bpf_tracing.h
> > > index da7e8d5c939c..82f1e935d549 100644
> > > --- a/tools/lib/bpf/bpf_tracing.h
> > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > @@ -265,25 +265,43 @@ struct pt_regs;
> > >
> > >  #endif
> > >
> > > -#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > > -#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > > -#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > > -#ifdef __PT_PARM4_REG_SYSCALL
> > > +#ifndef __PT_PARM1_REG_SYSCALL
> > > +#define __PT_PARM1_REG_SYSCALL __PT_PARM1_REG
> > > +#endif
> > > +#ifndef __PT_PARM2_REG_SYSCALL
> > > +#define __PT_PARM2_REG_SYSCALL __PT_PARM2_REG
> > > +#endif
> > > +#ifndef __PT_PARM3_REG_SYSCALL
> > > +#define __PT_PARM3_REG_SYSCALL __PT_PARM3_REG
> > > +#endif
> > > +#ifndef __PT_PARM4_REG_SYSCALL
> > > +#define __PT_PARM4_REG_SYSCALL __PT_PARM4_REG
> > > +#endif
> > > +#ifndef __PT_PARM5_REG_SYSCALL
> > > +#define __PT_PARM5_REG_SYSCALL __PT_PARM5_REG
> > > +#endif
> > > +
> > > +#ifndef PT_REGS_PARM1_SYSCALL
> > > +#define PT_REGS_PARM1_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM1_REG_SYSCALL)
> > > +#endif
> > > +#ifndef PT_REGS_PARM2_SYSCALL
> > > +#define PT_REGS_PARM2_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM2_REG_SYSCALL)
> > > +#endif
> > > +#ifndef PT_REGS_PARM3_SYSCALL
> > > +#define PT_REGS_PARM3_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM3_REG_SYSCALL)
> > > +#endif
> > > +#ifndef PT_REGS_PARM4_SYSCALL
> > >  #define PT_REGS_PARM4_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM4_REG_SYSCALL)
> > > -#else /* __PT_PARM4_REG_SYSCALL */
> > > -#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x)
> > >  #endif
> > > -#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> > > +#ifndef PT_REGS_PARM5_SYSCALL
> > > +#define PT_REGS_PARM5_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM5_REG_SYSCALL)
> > > +#endif
> > >
> > > -#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > > -#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > > -#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > > -#ifdef __PT_PARM4_REG_SYSCALL
> > > +#define PT_REGS_PARM1_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM1_REG_SYSCALL)
> > > +#define PT_REGS_PARM2_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM2_REG_SYSCALL)
> > > +#define PT_REGS_PARM3_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM3_REG_SYSCALL)
> > >  #define PT_REGS_PARM4_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM4_REG_SYSCALL)
> > > -#else /* __PT_PARM4_REG_SYSCALL */
> > > -#define PT_REGS_PARM4_CORE_SYSCALL(x) PT_REGS_PARM4_CORE(x)
> > > -#endif
> > > -#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > > +#define PT_REGS_PARM5_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM5_REG_SYSCALL)
> > >
> >
> > No, please don't do it. It makes CORE variants too rigid. We agreed
> > w/
> > Naveen that the way you did it in v2 is better and more flexible and
> > in v3 you did it the other way. Why?
>
> As far as I remember we didn't discuss this proposal from Naveen [1] -
> there was another one about moving SYS_PREFIX to libbpf, where
> we agreed that it would have bad consequences.

Alright, I guess I never submitted my opposition to what Naveen
proposed. But I did land the v3 version of that patch, didn't I? Why
change something that's already accepted?

>
> Isn't this patch essentially equivalent to the one from my v3 [2],
> but with the added ability to override more things and better-looking?

No, it's not. We want to override entire PT_REGS_PARM1_CORE_SYSCALL
definition to be something like BPF_CORE_READ((struct pt_regs___s390x
*)x, orig_gpr2), while you are making  PT_REGS_PARM1_CORE_SYSCALL
definition very rigid.

> I.e.: if we define __PT_PARMn_REG_SYSCALL, then PT_REGS_PARMn_SYSCALL
> and PT_REGS_PARMn_CORE_SYSCALL use that, and __PT_PARMn_REG otherwise.
>
> [1]
> https://lore.kernel.org/bpf/1643990954.fs9q9mrdxt.naveen@linux.ibm.com/
> [2]
> https://lore.kernel.org/bpf/20220204145018.1983773-5-iii@linux.ibm.com/
>
> >
> > >  #else /* defined(bpf_target_defined) */
> > >
> > > --
> > > 2.34.1
> > >
>