diff mbox series

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

Message ID 20220208051635.2160304-6-iii@linux.ibm.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Fix accessing syscall arguments | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail PR summary
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 7 maintainers not CCed: andrii@kernel.org kpsingh@kernel.org john.fastabend@gmail.com kafai@fb.com songliubraving@fb.com yhs@fb.com netdev@vger.kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 94 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next fail VM_Test

Commit Message

Ilya Leoshkevich Feb. 8, 2022, 5:16 a.m. UTC 
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(-)

Comments

Andrii Nakryiko Feb. 8, 2022, 10:05 p.m. UTC | #1 
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?

>  #else /* defined(bpf_target_defined) */
>
> --
> 2.34.1
>
Ilya Leoshkevich Feb. 8, 2022, 11:08 p.m. UTC | #2 
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.

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? 
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
> >
Andrii Nakryiko Feb. 8, 2022, 11:21 p.m. UTC | #3 
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
> > >
>
Ilya Leoshkevich Feb. 8, 2022, 11:30 p.m. UTC | #4 
On 2/9/22 00:21, Andrii Nakryiko wrote:
> 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?

Right. Sorry, I just wanted to use this opportunity to clean up things a
little.

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

Right, now that we've decided to use flavors, this is no longer useful.
I'll drop it for v5.

> 
> 
>> 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
>>>>
>>
diff mbox series

Patch

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)
 
 #else /* defined(bpf_target_defined) */