[PATCH bpf-next 0/4] samples: bpf: refactor BPF map test with libbpf

[PATCH bpf-next 0/4] samples: bpf: refactor BPF map test with libbpf

[Daniel T. Lee]

There have been many changes in how the current bpf program defines
map. The development of libbbpf has led to the new method called 
BTF-defined map, which is a new way of defining BPF maps, and thus has
a lot of differences from the existing MAP definition method.

Although bpf_load was also internally using libbbpf, fragmentation in 
its implementation began to occur, such as using its own structure, 
bpf_load_map_def, to define the map.

Therefore, in this patch set, map test programs, which are closely
related to changes in the definition method of BPF map, were refactored
with libbbpf.

Daniel T. Lee (4):
  samples: bpf: fix bpf programs with kprobe/sys_connect event
  samples: bpf: refactor BPF map in map test with libbpf
  samples: bpf: refactor BPF map performance test with libbpf
  selftests: bpf: remove unused bpf_map_def_legacy struct

 samples/bpf/Makefile                     |   4 +-
 samples/bpf/map_perf_test_kern.c         | 182 +++++++++++------------
 samples/bpf/map_perf_test_user.c         | 130 +++++++++++-----
 samples/bpf/test_map_in_map_kern.c       |  87 ++++++-----
 samples/bpf/test_map_in_map_user.c       |  53 ++++++-
 samples/bpf/test_probe_write_user_kern.c |   2 +-
 tools/testing/selftests/bpf/bpf_legacy.h |  14 --
 7 files changed, 275 insertions(+), 197 deletions(-)

-- 
2.25.1

[PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Daniel T. Lee]

Currently, BPF programs with kprobe/sys_connect does not work properly.

Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
This commit modifies the bpf_load behavior of kprobe events in the x64
architecture. If the current kprobe event target starts with "sys_*",
add the prefix "__x64_" to the front of the event.

Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
solution to most of the problems caused by the commit below.

    commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
    pt_regs-based sys_*() to __x64_sys_*()")

However, there is a problem with the sys_connect kprobe event that does
not work properly. For __sys_connect event, parameters can be fetched
normally, but for __x64_sys_connect, parameters cannot be fetched.

Because of this problem, this commit fixes the sys_connect event by
specifying the __sys_connect directly and this will bypass the
"__x64_" appending rule of bpf_load.

Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/map_perf_test_kern.c         | 2 +-
 samples/bpf/test_map_in_map_kern.c       | 2 +-
 samples/bpf/test_probe_write_user_kern.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
index 12e91ae64d4d..cebe2098bb24 100644
--- a/samples/bpf/map_perf_test_kern.c
+++ b/samples/bpf/map_perf_test_kern.c
@@ -154,7 +154,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_connect")
+SEC("kprobe/__sys_connect")
 int stress_lru_hmap_alloc(struct pt_regs *ctx)
 {
 	char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
index 6cee61e8ce9b..b1562ba2f025 100644
--- a/samples/bpf/test_map_in_map_kern.c
+++ b/samples/bpf/test_map_in_map_kern.c
@@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
 	return result ? *result : -ENOENT;
 }
 
-SEC("kprobe/sys_connect")
+SEC("kprobe/__sys_connect")
 int trace_sys_connect(struct pt_regs *ctx)
 {
 	struct sockaddr_in6 *in6;
diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
index 6579639a83b2..9b3c3918c37d 100644
--- a/samples/bpf/test_probe_write_user_kern.c
+++ b/samples/bpf/test_probe_write_user_kern.c
@@ -26,7 +26,7 @@ struct {
  * This example sits on a syscall, and the syscall ABI is relatively stable
  * of course, across platforms, and over time, the ABI may change.
  */
-SEC("kprobe/sys_connect")
+SEC("kprobe/__sys_connect")
 int bpf_prog1(struct pt_regs *ctx)
 {
 	struct sockaddr_in new_addr, orig_addr = {};
-- 
2.25.1

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Yonghong Song]

On 7/1/20 7:16 PM, Daniel T. Lee wrote:
> Currently, BPF programs with kprobe/sys_connect does not work properly.
> 
> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> This commit modifies the bpf_load behavior of kprobe events in the x64
> architecture. If the current kprobe event target starts with "sys_*",
> add the prefix "__x64_" to the front of the event.
> 
> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> solution to most of the problems caused by the commit below.
> 
>      commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
>      pt_regs-based sys_*() to __x64_sys_*()")
> 
> However, there is a problem with the sys_connect kprobe event that does
> not work properly. For __sys_connect event, parameters can be fetched
> normally, but for __x64_sys_connect, parameters cannot be fetched.
> 
> Because of this problem, this commit fixes the sys_connect event by
> specifying the __sys_connect directly and this will bypass the
> "__x64_" appending rule of bpf_load.

In the kernel code, we have

SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
                 int, addrlen)
{
         return __sys_connect(fd, uservaddr, addrlen);
}

Depending on compiler, there is no guarantee that __sys_connect will
not be inlined. I would prefer to still use the entry point
__x64_sys_* e.g.,
    SEC("kprobe/" SYSCALL(sys_write))

> 
> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
>   samples/bpf/map_perf_test_kern.c         | 2 +-
>   samples/bpf/test_map_in_map_kern.c       | 2 +-
>   samples/bpf/test_probe_write_user_kern.c | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
> index 12e91ae64d4d..cebe2098bb24 100644
> --- a/samples/bpf/map_perf_test_kern.c
> +++ b/samples/bpf/map_perf_test_kern.c
> @@ -154,7 +154,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
>   	return 0;
>   }
>   
> -SEC("kprobe/sys_connect")
> +SEC("kprobe/__sys_connect")
>   int stress_lru_hmap_alloc(struct pt_regs *ctx)
>   {
>   	char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
> diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
> index 6cee61e8ce9b..b1562ba2f025 100644
> --- a/samples/bpf/test_map_in_map_kern.c
> +++ b/samples/bpf/test_map_in_map_kern.c
> @@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
>   	return result ? *result : -ENOENT;
>   }
>   
> -SEC("kprobe/sys_connect")
> +SEC("kprobe/__sys_connect")
>   int trace_sys_connect(struct pt_regs *ctx)
>   {
>   	struct sockaddr_in6 *in6;
> diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
> index 6579639a83b2..9b3c3918c37d 100644
> --- a/samples/bpf/test_probe_write_user_kern.c
> +++ b/samples/bpf/test_probe_write_user_kern.c
> @@ -26,7 +26,7 @@ struct {
>    * This example sits on a syscall, and the syscall ABI is relatively stable
>    * of course, across platforms, and over time, the ABI may change.
>    */
> -SEC("kprobe/sys_connect")
> +SEC("kprobe/__sys_connect")
>   int bpf_prog1(struct pt_regs *ctx)
>   {
>   	struct sockaddr_in new_addr, orig_addr = {};
> 

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Daniel T. Lee]

On Thu, Jul 2, 2020 at 2:13 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/1/20 7:16 PM, Daniel T. Lee wrote:
> > Currently, BPF programs with kprobe/sys_connect does not work properly.
> >
> > Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > This commit modifies the bpf_load behavior of kprobe events in the x64
> > architecture. If the current kprobe event target starts with "sys_*",
> > add the prefix "__x64_" to the front of the event.
> >
> > Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> > solution to most of the problems caused by the commit below.
> >
> >      commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
> >      pt_regs-based sys_*() to __x64_sys_*()")
> >
> > However, there is a problem with the sys_connect kprobe event that does
> > not work properly. For __sys_connect event, parameters can be fetched
> > normally, but for __x64_sys_connect, parameters cannot be fetched.
> >
> > Because of this problem, this commit fixes the sys_connect event by
> > specifying the __sys_connect directly and this will bypass the
> > "__x64_" appending rule of bpf_load.
>
> In the kernel code, we have
>
> SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
>                  int, addrlen)
> {
>          return __sys_connect(fd, uservaddr, addrlen);
> }
>
> Depending on compiler, there is no guarantee that __sys_connect will
> not be inlined. I would prefer to still use the entry point
> __x64_sys_* e.g.,
>     SEC("kprobe/" SYSCALL(sys_write))
>

As you mentioned, there is clearly a possibility that problems may arise
because the symbol does not exist according to the compiler.

However, in x64, when using Kprobe for __x64_sys_connect event, the
tests are not working properly because the parameters cannot be fetched,
and the test under selftests/bpf is using "kprobe/_sys_connect" directly.

I'm not sure how to deal with this problem. Any advice and suggestions
will be greatly appreciated.

Thanks for your time and effort for the review.
Daniel

> >
> > Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> >   samples/bpf/map_perf_test_kern.c         | 2 +-
> >   samples/bpf/test_map_in_map_kern.c       | 2 +-
> >   samples/bpf/test_probe_write_user_kern.c | 2 +-
> >   3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
> > index 12e91ae64d4d..cebe2098bb24 100644
> > --- a/samples/bpf/map_perf_test_kern.c
> > +++ b/samples/bpf/map_perf_test_kern.c
> > @@ -154,7 +154,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
> >       return 0;
> >   }
> >
> > -SEC("kprobe/sys_connect")
> > +SEC("kprobe/__sys_connect")
> >   int stress_lru_hmap_alloc(struct pt_regs *ctx)
> >   {
> >       char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
> > diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
> > index 6cee61e8ce9b..b1562ba2f025 100644
> > --- a/samples/bpf/test_map_in_map_kern.c
> > +++ b/samples/bpf/test_map_in_map_kern.c
> > @@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
> >       return result ? *result : -ENOENT;
> >   }
> >
> > -SEC("kprobe/sys_connect")
> > +SEC("kprobe/__sys_connect")
> >   int trace_sys_connect(struct pt_regs *ctx)
> >   {
> >       struct sockaddr_in6 *in6;
> > diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
> > index 6579639a83b2..9b3c3918c37d 100644
> > --- a/samples/bpf/test_probe_write_user_kern.c
> > +++ b/samples/bpf/test_probe_write_user_kern.c
> > @@ -26,7 +26,7 @@ struct {
> >    * This example sits on a syscall, and the syscall ABI is relatively stable
> >    * of course, across platforms, and over time, the ABI may change.
> >    */
> > -SEC("kprobe/sys_connect")
> > +SEC("kprobe/__sys_connect")
> >   int bpf_prog1(struct pt_regs *ctx)
> >   {
> >       struct sockaddr_in new_addr, orig_addr = {};
> >

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Yonghong Song]

On 7/2/20 4:13 AM, Daniel T. Lee wrote:
> On Thu, Jul 2, 2020 at 2:13 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 7/1/20 7:16 PM, Daniel T. Lee wrote:
>>> Currently, BPF programs with kprobe/sys_connect does not work properly.
>>>
>>> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
>>> This commit modifies the bpf_load behavior of kprobe events in the x64
>>> architecture. If the current kprobe event target starts with "sys_*",
>>> add the prefix "__x64_" to the front of the event.
>>>
>>> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
>>> solution to most of the problems caused by the commit below.
>>>
>>>       commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
>>>       pt_regs-based sys_*() to __x64_sys_*()")
>>>
>>> However, there is a problem with the sys_connect kprobe event that does
>>> not work properly. For __sys_connect event, parameters can be fetched
>>> normally, but for __x64_sys_connect, parameters cannot be fetched.
>>>
>>> Because of this problem, this commit fixes the sys_connect event by
>>> specifying the __sys_connect directly and this will bypass the
>>> "__x64_" appending rule of bpf_load.
>>
>> In the kernel code, we have
>>
>> SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
>>                   int, addrlen)
>> {
>>           return __sys_connect(fd, uservaddr, addrlen);
>> }
>>
>> Depending on compiler, there is no guarantee that __sys_connect will
>> not be inlined. I would prefer to still use the entry point
>> __x64_sys_* e.g.,
>>      SEC("kprobe/" SYSCALL(sys_write))
>>
> 
> As you mentioned, there is clearly a possibility that problems may arise
> because the symbol does not exist according to the compiler.
> 
> However, in x64, when using Kprobe for __x64_sys_connect event, the
> tests are not working properly because the parameters cannot be fetched,
> and the test under selftests/bpf is using "kprobe/_sys_connect" directly.

This is the assembly code for __x64_sys_connect.

ffffffff818d3520 <__x64_sys_connect>:
ffffffff818d3520: e8 fb df 32 00        callq   0xffffffff81c01520 
<__fentry__>
ffffffff818d3525: 48 8b 57 60           movq    96(%rdi), %rdx
ffffffff818d3529: 48 8b 77 68           movq    104(%rdi), %rsi
ffffffff818d352d: 48 8b 7f 70           movq    112(%rdi), %rdi
ffffffff818d3531: e8 1a ff ff ff        callq   0xffffffff818d3450 
<__sys_connect>
ffffffff818d3536: 48 98                 cltq
ffffffff818d3538: c3                    retq
ffffffff818d3539: 0f 1f 80 00 00 00 00  nopl    (%rax)

In bpf program, the step is:
       struct pt_regs *real_regs = PT_REGS_PARM1(pt_regs);
       param1 = PT_REGS_PARM1(real_regs);
       param2 = PT_REGS_PARM2(real_regs);
       param3 = PT_REGS_PARM3(real_regs);
The same for s390.

For other architectures, no above indirection is needed.

I guess you can abstract the above into trace_common.h?

> 
> I'm not sure how to deal with this problem. Any advice and suggestions
> will be greatly appreciated.
> 
> Thanks for your time and effort for the review.
> Daniel
> 
>>>
>>> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
>>> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
>>> ---
>>>    samples/bpf/map_perf_test_kern.c         | 2 +-
>>>    samples/bpf/test_map_in_map_kern.c       | 2 +-
>>>    samples/bpf/test_probe_write_user_kern.c | 2 +-
>>>    3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
>>> index 12e91ae64d4d..cebe2098bb24 100644
>>> --- a/samples/bpf/map_perf_test_kern.c
>>> +++ b/samples/bpf/map_perf_test_kern.c
>>> @@ -154,7 +154,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
>>>        return 0;
>>>    }
>>>
>>> -SEC("kprobe/sys_connect")
>>> +SEC("kprobe/__sys_connect")
>>>    int stress_lru_hmap_alloc(struct pt_regs *ctx)
>>>    {
>>>        char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
>>> diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
>>> index 6cee61e8ce9b..b1562ba2f025 100644
>>> --- a/samples/bpf/test_map_in_map_kern.c
>>> +++ b/samples/bpf/test_map_in_map_kern.c
>>> @@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
>>>        return result ? *result : -ENOENT;
>>>    }
>>>
>>> -SEC("kprobe/sys_connect")
>>> +SEC("kprobe/__sys_connect")
>>>    int trace_sys_connect(struct pt_regs *ctx)
>>>    {
>>>        struct sockaddr_in6 *in6;
>>> diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
>>> index 6579639a83b2..9b3c3918c37d 100644
>>> --- a/samples/bpf/test_probe_write_user_kern.c
>>> +++ b/samples/bpf/test_probe_write_user_kern.c
>>> @@ -26,7 +26,7 @@ struct {
>>>     * This example sits on a syscall, and the syscall ABI is relatively stable
>>>     * of course, across platforms, and over time, the ABI may change.
>>>     */
>>> -SEC("kprobe/sys_connect")
>>> +SEC("kprobe/__sys_connect")
>>>    int bpf_prog1(struct pt_regs *ctx)
>>>    {
>>>        struct sockaddr_in new_addr, orig_addr = {};
>>>

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Daniel T. Lee]

On Fri, Jul 3, 2020 at 1:04 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/2/20 4:13 AM, Daniel T. Lee wrote:
> > On Thu, Jul 2, 2020 at 2:13 PM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 7/1/20 7:16 PM, Daniel T. Lee wrote:
> >>> Currently, BPF programs with kprobe/sys_connect does not work properly.
> >>>
> >>> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> >>> This commit modifies the bpf_load behavior of kprobe events in the x64
> >>> architecture. If the current kprobe event target starts with "sys_*",
> >>> add the prefix "__x64_" to the front of the event.
> >>>
> >>> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> >>> solution to most of the problems caused by the commit below.
> >>>
> >>>       commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
> >>>       pt_regs-based sys_*() to __x64_sys_*()")
> >>>
> >>> However, there is a problem with the sys_connect kprobe event that does
> >>> not work properly. For __sys_connect event, parameters can be fetched
> >>> normally, but for __x64_sys_connect, parameters cannot be fetched.
> >>>
> >>> Because of this problem, this commit fixes the sys_connect event by
> >>> specifying the __sys_connect directly and this will bypass the
> >>> "__x64_" appending rule of bpf_load.
> >>
> >> In the kernel code, we have
> >>
> >> SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
> >>                   int, addrlen)
> >> {
> >>           return __sys_connect(fd, uservaddr, addrlen);
> >> }
> >>
> >> Depending on compiler, there is no guarantee that __sys_connect will
> >> not be inlined. I would prefer to still use the entry point
> >> __x64_sys_* e.g.,
> >>      SEC("kprobe/" SYSCALL(sys_write))
> >>
> >
> > As you mentioned, there is clearly a possibility that problems may arise
> > because the symbol does not exist according to the compiler.
> >
> > However, in x64, when using Kprobe for __x64_sys_connect event, the
> > tests are not working properly because the parameters cannot be fetched,
> > and the test under selftests/bpf is using "kprobe/_sys_connect" directly.
>
> This is the assembly code for __x64_sys_connect.
>
> ffffffff818d3520 <__x64_sys_connect>:
> ffffffff818d3520: e8 fb df 32 00        callq   0xffffffff81c01520
> <__fentry__>
> ffffffff818d3525: 48 8b 57 60           movq    96(%rdi), %rdx
> ffffffff818d3529: 48 8b 77 68           movq    104(%rdi), %rsi
> ffffffff818d352d: 48 8b 7f 70           movq    112(%rdi), %rdi
> ffffffff818d3531: e8 1a ff ff ff        callq   0xffffffff818d3450
> <__sys_connect>
> ffffffff818d3536: 48 98                 cltq
> ffffffff818d3538: c3                    retq
> ffffffff818d3539: 0f 1f 80 00 00 00 00  nopl    (%rax)
>
> In bpf program, the step is:
>        struct pt_regs *real_regs = PT_REGS_PARM1(pt_regs);
>        param1 = PT_REGS_PARM1(real_regs);
>        param2 = PT_REGS_PARM2(real_regs);
>        param3 = PT_REGS_PARM3(real_regs);
> The same for s390.
>

I'm sorry that I seem to get it wrong,
But is it available to access 'struct pt_regs *' recursively?

It seems nested use of PT_REGS_PARM causes invalid memory access.

    $ sudo ./test_probe_write_user
    libbpf: load bpf program failed: Permission denied
    libbpf: -- BEGIN DUMP LOG ---
    libbpf:
    Unrecognized arg#0 type PTR
    ; struct pt_regs *real_regs = PT_REGS_PARM1(ctx);
    0: (79) r1 = *(u64 *)(r1 +112)
    ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
    1: (79) r6 = *(u64 *)(r1 +104)
    R1 invalid mem access 'inv'
    processed 2 insns (limit 1000000) max_states_per_insn 0
total_states 0 peak_states 0 mark_read 0

    libbpf: -- END LOG --
    libbpf: failed to load program 'kprobe/__x64_sys_connect'
    libbpf: failed to load object './test_probe_write_user_kern.o'
    ERROR: loading BPF object file failed

I'm not fully aware of the BPF verifier's internal structure.
Is there any workaround to solve this problem?

Thanks for your time and effort for the review.
Daniel.

>
> For other architectures, no above indirection is needed.
>
> I guess you can abstract the above into trace_common.h?
>
> >
> > I'm not sure how to deal with this problem. Any advice and suggestions
> > will be greatly appreciated.
> >
> > Thanks for your time and effort for the review.
> > Daniel
> >
> >>>
> >>> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> >>> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> >>> ---
> >>>    samples/bpf/map_perf_test_kern.c         | 2 +-
> >>>    samples/bpf/test_map_in_map_kern.c       | 2 +-
> >>>    samples/bpf/test_probe_write_user_kern.c | 2 +-
> >>>    3 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
> >>> index 12e91ae64d4d..cebe2098bb24 100644
> >>> --- a/samples/bpf/map_perf_test_kern.c
> >>> +++ b/samples/bpf/map_perf_test_kern.c
> >>> @@ -154,7 +154,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
> >>>        return 0;
> >>>    }
> >>>
> >>> -SEC("kprobe/sys_connect")
> >>> +SEC("kprobe/__sys_connect")
> >>>    int stress_lru_hmap_alloc(struct pt_regs *ctx)
> >>>    {
> >>>        char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
> >>> diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
> >>> index 6cee61e8ce9b..b1562ba2f025 100644
> >>> --- a/samples/bpf/test_map_in_map_kern.c
> >>> +++ b/samples/bpf/test_map_in_map_kern.c
> >>> @@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
> >>>        return result ? *result : -ENOENT;
> >>>    }
> >>>
> >>> -SEC("kprobe/sys_connect")
> >>> +SEC("kprobe/__sys_connect")
> >>>    int trace_sys_connect(struct pt_regs *ctx)
> >>>    {
> >>>        struct sockaddr_in6 *in6;
> >>> diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
> >>> index 6579639a83b2..9b3c3918c37d 100644
> >>> --- a/samples/bpf/test_probe_write_user_kern.c
> >>> +++ b/samples/bpf/test_probe_write_user_kern.c
> >>> @@ -26,7 +26,7 @@ struct {
> >>>     * This example sits on a syscall, and the syscall ABI is relatively stable
> >>>     * of course, across platforms, and over time, the ABI may change.
> >>>     */
> >>> -SEC("kprobe/sys_connect")
> >>> +SEC("kprobe/__sys_connect")
> >>>    int bpf_prog1(struct pt_regs *ctx)
> >>>    {
> >>>        struct sockaddr_in new_addr, orig_addr = {};
> >>>

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Andrii Nakryiko]

On Mon, Jul 6, 2020 at 3:28 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> On Fri, Jul 3, 2020 at 1:04 AM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 7/2/20 4:13 AM, Daniel T. Lee wrote:
> > > On Thu, Jul 2, 2020 at 2:13 PM Yonghong Song <yhs@fb.com> wrote:
> > >>
> > >>
> > >>
> > >> On 7/1/20 7:16 PM, Daniel T. Lee wrote:
> > >>> Currently, BPF programs with kprobe/sys_connect does not work properly.
> > >>>
> > >>> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > >>> This commit modifies the bpf_load behavior of kprobe events in the x64
> > >>> architecture. If the current kprobe event target starts with "sys_*",
> > >>> add the prefix "__x64_" to the front of the event.
> > >>>
> > >>> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> > >>> solution to most of the problems caused by the commit below.
> > >>>
> > >>>       commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
> > >>>       pt_regs-based sys_*() to __x64_sys_*()")
> > >>>
> > >>> However, there is a problem with the sys_connect kprobe event that does
> > >>> not work properly. For __sys_connect event, parameters can be fetched
> > >>> normally, but for __x64_sys_connect, parameters cannot be fetched.
> > >>>
> > >>> Because of this problem, this commit fixes the sys_connect event by
> > >>> specifying the __sys_connect directly and this will bypass the
> > >>> "__x64_" appending rule of bpf_load.
> > >>
> > >> In the kernel code, we have
> > >>
> > >> SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
> > >>                   int, addrlen)
> > >> {
> > >>           return __sys_connect(fd, uservaddr, addrlen);
> > >> }
> > >>
> > >> Depending on compiler, there is no guarantee that __sys_connect will
> > >> not be inlined. I would prefer to still use the entry point
> > >> __x64_sys_* e.g.,
> > >>      SEC("kprobe/" SYSCALL(sys_write))
> > >>
> > >
> > > As you mentioned, there is clearly a possibility that problems may arise
> > > because the symbol does not exist according to the compiler.
> > >
> > > However, in x64, when using Kprobe for __x64_sys_connect event, the
> > > tests are not working properly because the parameters cannot be fetched,
> > > and the test under selftests/bpf is using "kprobe/_sys_connect" directly.
> >
> > This is the assembly code for __x64_sys_connect.
> >
> > ffffffff818d3520 <__x64_sys_connect>:
> > ffffffff818d3520: e8 fb df 32 00        callq   0xffffffff81c01520
> > <__fentry__>
> > ffffffff818d3525: 48 8b 57 60           movq    96(%rdi), %rdx
> > ffffffff818d3529: 48 8b 77 68           movq    104(%rdi), %rsi
> > ffffffff818d352d: 48 8b 7f 70           movq    112(%rdi), %rdi
> > ffffffff818d3531: e8 1a ff ff ff        callq   0xffffffff818d3450
> > <__sys_connect>
> > ffffffff818d3536: 48 98                 cltq
> > ffffffff818d3538: c3                    retq
> > ffffffff818d3539: 0f 1f 80 00 00 00 00  nopl    (%rax)
> >
> > In bpf program, the step is:
> >        struct pt_regs *real_regs = PT_REGS_PARM1(pt_regs);
> >        param1 = PT_REGS_PARM1(real_regs);
> >        param2 = PT_REGS_PARM2(real_regs);
> >        param3 = PT_REGS_PARM3(real_regs);
> > The same for s390.
> >
>
> I'm sorry that I seem to get it wrong,
> But is it available to access 'struct pt_regs *' recursively?
>
> It seems nested use of PT_REGS_PARM causes invalid memory access.
>
>     $ sudo ./test_probe_write_user
>     libbpf: load bpf program failed: Permission denied
>     libbpf: -- BEGIN DUMP LOG ---
>     libbpf:
>     Unrecognized arg#0 type PTR
>     ; struct pt_regs *real_regs = PT_REGS_PARM1(ctx);
>     0: (79) r1 = *(u64 *)(r1 +112)
>     ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
>     1: (79) r6 = *(u64 *)(r1 +104)
>     R1 invalid mem access 'inv'
>     processed 2 insns (limit 1000000) max_states_per_insn 0
> total_states 0 peak_states 0 mark_read 0
>
>     libbpf: -- END LOG --
>     libbpf: failed to load program 'kprobe/__x64_sys_connect'
>     libbpf: failed to load object './test_probe_write_user_kern.o'
>     ERROR: loading BPF object file failed
>
> I'm not fully aware of the BPF verifier's internal structure.
> Is there any workaround to solve this problem?

You need to use bpf_probe_read_kernel() to get those arguments from
real_args. Or better just use PT_REGS_PARM1_CORE(x) and others, which
does that for you (+ CO-RE relocation).


>
> Thanks for your time and effort for the review.
> Daniel.
>
> >
> > For other architectures, no above indirection is needed.
> >
> > I guess you can abstract the above into trace_common.h?
> >
> > >
> > > I'm not sure how to deal with this problem. Any advice and suggestions
> > > will be greatly appreciated.
> > >
> > > Thanks for your time and effort for the review.
> > > Daniel
> > >
> > >>>
> > >>> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > >>> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > >>> ---
> > >>>    samples/bpf/map_perf_test_kern.c         | 2 +-
> > >>>    samples/bpf/test_map_in_map_kern.c       | 2 +-
> > >>>    samples/bpf/test_probe_write_user_kern.c | 2 +-
> > >>>    3 files changed, 3 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
> > >>> index 12e91ae64d4d..cebe2098bb24 100644
> > >>> --- a/samples/bpf/map_perf_test_kern.c
> > >>> +++ b/samples/bpf/map_perf_test_kern.c
> > >>> @@ -154,7 +154,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
> > >>>        return 0;
> > >>>    }
> > >>>
> > >>> -SEC("kprobe/sys_connect")
> > >>> +SEC("kprobe/__sys_connect")
> > >>>    int stress_lru_hmap_alloc(struct pt_regs *ctx)
> > >>>    {
> > >>>        char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
> > >>> diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
> > >>> index 6cee61e8ce9b..b1562ba2f025 100644
> > >>> --- a/samples/bpf/test_map_in_map_kern.c
> > >>> +++ b/samples/bpf/test_map_in_map_kern.c
> > >>> @@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
> > >>>        return result ? *result : -ENOENT;
> > >>>    }
> > >>>
> > >>> -SEC("kprobe/sys_connect")
> > >>> +SEC("kprobe/__sys_connect")
> > >>>    int trace_sys_connect(struct pt_regs *ctx)
> > >>>    {
> > >>>        struct sockaddr_in6 *in6;
> > >>> diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
> > >>> index 6579639a83b2..9b3c3918c37d 100644
> > >>> --- a/samples/bpf/test_probe_write_user_kern.c
> > >>> +++ b/samples/bpf/test_probe_write_user_kern.c
> > >>> @@ -26,7 +26,7 @@ struct {
> > >>>     * This example sits on a syscall, and the syscall ABI is relatively stable
> > >>>     * of course, across platforms, and over time, the ABI may change.
> > >>>     */
> > >>> -SEC("kprobe/sys_connect")
> > >>> +SEC("kprobe/__sys_connect")
> > >>>    int bpf_prog1(struct pt_regs *ctx)
> > >>>    {
> > >>>        struct sockaddr_in new_addr, orig_addr = {};
> > >>>

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Daniel T. Lee]

On Tue, Jul 7, 2020 at 8:50 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 6, 2020 at 3:28 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > On Fri, Jul 3, 2020 at 1:04 AM Yonghong Song <yhs@fb.com> wrote:
> > >
> > >
> > >
> > > On 7/2/20 4:13 AM, Daniel T. Lee wrote:
> > > > On Thu, Jul 2, 2020 at 2:13 PM Yonghong Song <yhs@fb.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 7/1/20 7:16 PM, Daniel T. Lee wrote:
> > > >>> Currently, BPF programs with kprobe/sys_connect does not work properly.
> > > >>>
> > > >>> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > > >>> This commit modifies the bpf_load behavior of kprobe events in the x64
> > > >>> architecture. If the current kprobe event target starts with "sys_*",
> > > >>> add the prefix "__x64_" to the front of the event.
> > > >>>
> > > >>> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> > > >>> solution to most of the problems caused by the commit below.
> > > >>>
> > > >>>       commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
> > > >>>       pt_regs-based sys_*() to __x64_sys_*()")
> > > >>>
> > > >>> However, there is a problem with the sys_connect kprobe event that does
> > > >>> not work properly. For __sys_connect event, parameters can be fetched
> > > >>> normally, but for __x64_sys_connect, parameters cannot be fetched.
> > > >>>
> > > >>> Because of this problem, this commit fixes the sys_connect event by
> > > >>> specifying the __sys_connect directly and this will bypass the
> > > >>> "__x64_" appending rule of bpf_load.
> > > >>
> > > >> In the kernel code, we have
> > > >>
> > > >> SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
> > > >>                   int, addrlen)
> > > >> {
> > > >>           return __sys_connect(fd, uservaddr, addrlen);
> > > >> }
> > > >>
> > > >> Depending on compiler, there is no guarantee that __sys_connect will
> > > >> not be inlined. I would prefer to still use the entry point
> > > >> __x64_sys_* e.g.,
> > > >>      SEC("kprobe/" SYSCALL(sys_write))
> > > >>
> > > >
> > > > As you mentioned, there is clearly a possibility that problems may arise
> > > > because the symbol does not exist according to the compiler.
> > > >
> > > > However, in x64, when using Kprobe for __x64_sys_connect event, the
> > > > tests are not working properly because the parameters cannot be fetched,
> > > > and the test under selftests/bpf is using "kprobe/_sys_connect" directly.
> > >
> > > This is the assembly code for __x64_sys_connect.
> > >
> > > ffffffff818d3520 <__x64_sys_connect>:
> > > ffffffff818d3520: e8 fb df 32 00        callq   0xffffffff81c01520
> > > <__fentry__>
> > > ffffffff818d3525: 48 8b 57 60           movq    96(%rdi), %rdx
> > > ffffffff818d3529: 48 8b 77 68           movq    104(%rdi), %rsi
> > > ffffffff818d352d: 48 8b 7f 70           movq    112(%rdi), %rdi
> > > ffffffff818d3531: e8 1a ff ff ff        callq   0xffffffff818d3450
> > > <__sys_connect>
> > > ffffffff818d3536: 48 98                 cltq
> > > ffffffff818d3538: c3                    retq
> > > ffffffff818d3539: 0f 1f 80 00 00 00 00  nopl    (%rax)
> > >
> > > In bpf program, the step is:
> > >        struct pt_regs *real_regs = PT_REGS_PARM1(pt_regs);
> > >        param1 = PT_REGS_PARM1(real_regs);
> > >        param2 = PT_REGS_PARM2(real_regs);
> > >        param3 = PT_REGS_PARM3(real_regs);
> > > The same for s390.
> > >
> >
> > I'm sorry that I seem to get it wrong,
> > But is it available to access 'struct pt_regs *' recursively?
> >
> > It seems nested use of PT_REGS_PARM causes invalid memory access.
> >
> >     $ sudo ./test_probe_write_user
> >     libbpf: load bpf program failed: Permission denied
> >     libbpf: -- BEGIN DUMP LOG ---
> >     libbpf:
> >     Unrecognized arg#0 type PTR
> >     ; struct pt_regs *real_regs = PT_REGS_PARM1(ctx);
> >     0: (79) r1 = *(u64 *)(r1 +112)
> >     ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
> >     1: (79) r6 = *(u64 *)(r1 +104)
> >     R1 invalid mem access 'inv'
> >     processed 2 insns (limit 1000000) max_states_per_insn 0
> > total_states 0 peak_states 0 mark_read 0
> >
> >     libbpf: -- END LOG --
> >     libbpf: failed to load program 'kprobe/__x64_sys_connect'
> >     libbpf: failed to load object './test_probe_write_user_kern.o'
> >     ERROR: loading BPF object file failed
> >
> > I'm not fully aware of the BPF verifier's internal structure.
> > Is there any workaround to solve this problem?
>
> You need to use bpf_probe_read_kernel() to get those arguments from
> real_args. Or better just use PT_REGS_PARM1_CORE(x) and others, which
> does that for you (+ CO-RE relocation).
>
>

Thanks for the tip!

I've just tried the old hack '_(P)':
(which is similar implementation with BPF_CORE_READ())

    #define _(P) ({typeof(P) val = 0; bpf_probe_read(&val,
sizeof(val), &P); val;})
    [...]
    struct pt_regs *regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
    void *sockaddr_arg = (void *)_(PT_REGS_PARM2(regs));
    int sockaddr_len = (int)_(PT_REGS_PARM3(regs));

and it works properly.

Just wondering, why is the pointer chasing of the original ctx
considered as an unsafe pointer here?

    ; struct pt_regs *real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
    0: (79) r1 = *(u64 *)(r1 +112)
    [...]
    ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
    4: (79) r6 = *(u64 *)(r1 +104)

Is it considered as an unsafe pointer since it is unknown what exists
in the pointer (r1 + 104), but the instruction is trying to access it?


I am a little concerned about using PT_REGS_PARM1_CORE
because it is not a CORE-related patch, but if using CORE is the
direction BPF wants to take, I will use PT_REGS_PARM1_CORE()
instead of _(P) hack using bpf_probe_read().

In addition, PT_REGS_PARM1_CORE() allows me to write code
neatly without having to define additional macro _(P).

Thank you for your time and effort for the review.
Daniel

> >
> > Thanks for your time and effort for the review.
> > Daniel.
> >
> > >
> > > For other architectures, no above indirection is needed.
> > >
> > > I guess you can abstract the above into trace_common.h?
> > >
> > > >
> > > > I'm not sure how to deal with this problem. Any advice and suggestions
> > > > will be greatly appreciated.
> > > >
> > > > Thanks for your time and effort for the review.
> > > > Daniel
> > > >
> > > >>>
> > > >>> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > > >>> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > >>> ---
> > > >>>    samples/bpf/map_perf_test_kern.c         | 2 +-
> > > >>>    samples/bpf/test_map_in_map_kern.c       | 2 +-
> > > >>>    samples/bpf/test_probe_write_user_kern.c | 2 +-
> > > >>>    3 files changed, 3 insertions(+), 3 deletions(-)
> > > >>>
> > > >>> diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
> > > >>> index 12e91ae64d4d..cebe2098bb24 100644
> > > >>> --- a/samples/bpf/map_perf_test_kern.c
> > > >>> +++ b/samples/bpf/map_perf_test_kern.c
> > > >>> @@ -154,7 +154,7 @@ int stress_percpu_hmap_alloc(struct pt_regs *ctx)
> > > >>>        return 0;
> > > >>>    }
> > > >>>
> > > >>> -SEC("kprobe/sys_connect")
> > > >>> +SEC("kprobe/__sys_connect")
> > > >>>    int stress_lru_hmap_alloc(struct pt_regs *ctx)
> > > >>>    {
> > > >>>        char fmt[] = "Failed at stress_lru_hmap_alloc. ret:%dn";
> > > >>> diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
> > > >>> index 6cee61e8ce9b..b1562ba2f025 100644
> > > >>> --- a/samples/bpf/test_map_in_map_kern.c
> > > >>> +++ b/samples/bpf/test_map_in_map_kern.c
> > > >>> @@ -102,7 +102,7 @@ static __always_inline int do_inline_hash_lookup(void *inner_map, u32 port)
> > > >>>        return result ? *result : -ENOENT;
> > > >>>    }
> > > >>>
> > > >>> -SEC("kprobe/sys_connect")
> > > >>> +SEC("kprobe/__sys_connect")
> > > >>>    int trace_sys_connect(struct pt_regs *ctx)
> > > >>>    {
> > > >>>        struct sockaddr_in6 *in6;
> > > >>> diff --git a/samples/bpf/test_probe_write_user_kern.c b/samples/bpf/test_probe_write_user_kern.c
> > > >>> index 6579639a83b2..9b3c3918c37d 100644
> > > >>> --- a/samples/bpf/test_probe_write_user_kern.c
> > > >>> +++ b/samples/bpf/test_probe_write_user_kern.c
> > > >>> @@ -26,7 +26,7 @@ struct {
> > > >>>     * This example sits on a syscall, and the syscall ABI is relatively stable
> > > >>>     * of course, across platforms, and over time, the ABI may change.
> > > >>>     */
> > > >>> -SEC("kprobe/sys_connect")
> > > >>> +SEC("kprobe/__sys_connect")
> > > >>>    int bpf_prog1(struct pt_regs *ctx)
> > > >>>    {
> > > >>>        struct sockaddr_in new_addr, orig_addr = {};
> > > >>>

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Andrii Nakryiko]

On Mon, Jul 6, 2020 at 7:33 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> On Tue, Jul 7, 2020 at 8:50 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Jul 6, 2020 at 3:28 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > >
> > > On Fri, Jul 3, 2020 at 1:04 AM Yonghong Song <yhs@fb.com> wrote:
> > > >
> > > >
> > > >
> > > > On 7/2/20 4:13 AM, Daniel T. Lee wrote:
> > > > > On Thu, Jul 2, 2020 at 2:13 PM Yonghong Song <yhs@fb.com> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 7/1/20 7:16 PM, Daniel T. Lee wrote:
> > > > >>> Currently, BPF programs with kprobe/sys_connect does not work properly.
> > > > >>>
> > > > >>> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > > > >>> This commit modifies the bpf_load behavior of kprobe events in the x64
> > > > >>> architecture. If the current kprobe event target starts with "sys_*",
> > > > >>> add the prefix "__x64_" to the front of the event.
> > > > >>>
> > > > >>> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> > > > >>> solution to most of the problems caused by the commit below.
> > > > >>>
> > > > >>>       commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
> > > > >>>       pt_regs-based sys_*() to __x64_sys_*()")
> > > > >>>
> > > > >>> However, there is a problem with the sys_connect kprobe event that does
> > > > >>> not work properly. For __sys_connect event, parameters can be fetched
> > > > >>> normally, but for __x64_sys_connect, parameters cannot be fetched.
> > > > >>>
> > > > >>> Because of this problem, this commit fixes the sys_connect event by
> > > > >>> specifying the __sys_connect directly and this will bypass the
> > > > >>> "__x64_" appending rule of bpf_load.
> > > > >>
> > > > >> In the kernel code, we have
> > > > >>
> > > > >> SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
> > > > >>                   int, addrlen)
> > > > >> {
> > > > >>           return __sys_connect(fd, uservaddr, addrlen);
> > > > >> }
> > > > >>
> > > > >> Depending on compiler, there is no guarantee that __sys_connect will
> > > > >> not be inlined. I would prefer to still use the entry point
> > > > >> __x64_sys_* e.g.,
> > > > >>      SEC("kprobe/" SYSCALL(sys_write))
> > > > >>
> > > > >
> > > > > As you mentioned, there is clearly a possibility that problems may arise
> > > > > because the symbol does not exist according to the compiler.
> > > > >
> > > > > However, in x64, when using Kprobe for __x64_sys_connect event, the
> > > > > tests are not working properly because the parameters cannot be fetched,
> > > > > and the test under selftests/bpf is using "kprobe/_sys_connect" directly.
> > > >
> > > > This is the assembly code for __x64_sys_connect.
> > > >
> > > > ffffffff818d3520 <__x64_sys_connect>:
> > > > ffffffff818d3520: e8 fb df 32 00        callq   0xffffffff81c01520
> > > > <__fentry__>
> > > > ffffffff818d3525: 48 8b 57 60           movq    96(%rdi), %rdx
> > > > ffffffff818d3529: 48 8b 77 68           movq    104(%rdi), %rsi
> > > > ffffffff818d352d: 48 8b 7f 70           movq    112(%rdi), %rdi
> > > > ffffffff818d3531: e8 1a ff ff ff        callq   0xffffffff818d3450
> > > > <__sys_connect>
> > > > ffffffff818d3536: 48 98                 cltq
> > > > ffffffff818d3538: c3                    retq
> > > > ffffffff818d3539: 0f 1f 80 00 00 00 00  nopl    (%rax)
> > > >
> > > > In bpf program, the step is:
> > > >        struct pt_regs *real_regs = PT_REGS_PARM1(pt_regs);
> > > >        param1 = PT_REGS_PARM1(real_regs);
> > > >        param2 = PT_REGS_PARM2(real_regs);
> > > >        param3 = PT_REGS_PARM3(real_regs);
> > > > The same for s390.
> > > >
> > >
> > > I'm sorry that I seem to get it wrong,
> > > But is it available to access 'struct pt_regs *' recursively?
> > >
> > > It seems nested use of PT_REGS_PARM causes invalid memory access.
> > >
> > >     $ sudo ./test_probe_write_user
> > >     libbpf: load bpf program failed: Permission denied
> > >     libbpf: -- BEGIN DUMP LOG ---
> > >     libbpf:
> > >     Unrecognized arg#0 type PTR
> > >     ; struct pt_regs *real_regs = PT_REGS_PARM1(ctx);
> > >     0: (79) r1 = *(u64 *)(r1 +112)
> > >     ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
> > >     1: (79) r6 = *(u64 *)(r1 +104)
> > >     R1 invalid mem access 'inv'
> > >     processed 2 insns (limit 1000000) max_states_per_insn 0
> > > total_states 0 peak_states 0 mark_read 0
> > >
> > >     libbpf: -- END LOG --
> > >     libbpf: failed to load program 'kprobe/__x64_sys_connect'
> > >     libbpf: failed to load object './test_probe_write_user_kern.o'
> > >     ERROR: loading BPF object file failed
> > >
> > > I'm not fully aware of the BPF verifier's internal structure.
> > > Is there any workaround to solve this problem?
> >
> > You need to use bpf_probe_read_kernel() to get those arguments from
> > real_args. Or better just use PT_REGS_PARM1_CORE(x) and others, which
> > does that for you (+ CO-RE relocation).
> >
> >
>
> Thanks for the tip!
>
> I've just tried the old hack '_(P)':
> (which is similar implementation with BPF_CORE_READ())
>
>     #define _(P) ({typeof(P) val = 0; bpf_probe_read(&val,
> sizeof(val), &P); val;})
>     [...]
>     struct pt_regs *regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
>     void *sockaddr_arg = (void *)_(PT_REGS_PARM2(regs));
>     int sockaddr_len = (int)_(PT_REGS_PARM3(regs));
>
> and it works properly.
>
> Just wondering, why is the pointer chasing of the original ctx
> considered as an unsafe pointer here?
>
>     ; struct pt_regs *real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
>     0: (79) r1 = *(u64 *)(r1 +112)
>     [...]
>     ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
>     4: (79) r6 = *(u64 *)(r1 +104)
>
> Is it considered as an unsafe pointer since it is unknown what exists
> in the pointer (r1 + 104), but the instruction is trying to access it?
>

Yes.
Because after the initial pointer read, the verifier assumes that you
are reading a random piece of memory.

>
> I am a little concerned about using PT_REGS_PARM1_CORE
> because it is not a CORE-related patch, but if using CORE is the
> direction BPF wants to take, I will use PT_REGS_PARM1_CORE()
> instead of _(P) hack using bpf_probe_read().

bpf_probe_read() works as well. But yeah, BPF CO-RE is the way modern
tracing applications are leaning, look at selftests and see how many
are using CO-RE already. It's pretty much the only way to write
portable tracing BPF applications, short of taking Clang/LLVM
**runtime** dependency, the way BCC makes you do.

>
> In addition, PT_REGS_PARM1_CORE() allows me to write code
> neatly without having to define additional macro _(P).
>
> Thank you for your time and effort for the review.
> Daniel
>
> > >
> > > Thanks for your time and effort for the review.
> > > Daniel.
> > >
> > > >
> > > > For other architectures, no above indirection is needed.
> > > >
> > > > I guess you can abstract the above into trace_common.h?
> > > >
> > > > >
> > > > > I'm not sure how to deal with this problem. Any advice and suggestions
> > > > > will be greatly appreciated.
> > > > >
> > > > > Thanks for your time and effort for the review.
> > > > > Daniel
> > > > >
> > > > >>>
> > > > >>> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > > > >>> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > > >>> ---

[...]

Re: [PATCH bpf-next 1/4] samples: bpf: fix bpf programs with kprobe/sys_connect event

[Daniel T. Lee]

On Tue, Jul 7, 2020 at 2:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Jul 6, 2020 at 7:33 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > On Tue, Jul 7, 2020 at 8:50 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Jul 6, 2020 at 3:28 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > > >
> > > > On Fri, Jul 3, 2020 at 1:04 AM Yonghong Song <yhs@fb.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 7/2/20 4:13 AM, Daniel T. Lee wrote:
> > > > > > On Thu, Jul 2, 2020 at 2:13 PM Yonghong Song <yhs@fb.com> wrote:
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> On 7/1/20 7:16 PM, Daniel T. Lee wrote:
> > > > > >>> Currently, BPF programs with kprobe/sys_connect does not work properly.
> > > > > >>>
> > > > > >>> Commit 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > > > > >>> This commit modifies the bpf_load behavior of kprobe events in the x64
> > > > > >>> architecture. If the current kprobe event target starts with "sys_*",
> > > > > >>> add the prefix "__x64_" to the front of the event.
> > > > > >>>
> > > > > >>> Appending "__x64_" prefix with kprobe/sys_* event was appropriate as a
> > > > > >>> solution to most of the problems caused by the commit below.
> > > > > >>>
> > > > > >>>       commit d5a00528b58c ("syscalls/core, syscalls/x86: Rename struct
> > > > > >>>       pt_regs-based sys_*() to __x64_sys_*()")
> > > > > >>>
> > > > > >>> However, there is a problem with the sys_connect kprobe event that does
> > > > > >>> not work properly. For __sys_connect event, parameters can be fetched
> > > > > >>> normally, but for __x64_sys_connect, parameters cannot be fetched.
> > > > > >>>
> > > > > >>> Because of this problem, this commit fixes the sys_connect event by
> > > > > >>> specifying the __sys_connect directly and this will bypass the
> > > > > >>> "__x64_" appending rule of bpf_load.
> > > > > >>
> > > > > >> In the kernel code, we have
> > > > > >>
> > > > > >> SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
> > > > > >>                   int, addrlen)
> > > > > >> {
> > > > > >>           return __sys_connect(fd, uservaddr, addrlen);
> > > > > >> }
> > > > > >>
> > > > > >> Depending on compiler, there is no guarantee that __sys_connect will
> > > > > >> not be inlined. I would prefer to still use the entry point
> > > > > >> __x64_sys_* e.g.,
> > > > > >>      SEC("kprobe/" SYSCALL(sys_write))
> > > > > >>
> > > > > >
> > > > > > As you mentioned, there is clearly a possibility that problems may arise
> > > > > > because the symbol does not exist according to the compiler.
> > > > > >
> > > > > > However, in x64, when using Kprobe for __x64_sys_connect event, the
> > > > > > tests are not working properly because the parameters cannot be fetched,
> > > > > > and the test under selftests/bpf is using "kprobe/_sys_connect" directly.
> > > > >
> > > > > This is the assembly code for __x64_sys_connect.
> > > > >
> > > > > ffffffff818d3520 <__x64_sys_connect>:
> > > > > ffffffff818d3520: e8 fb df 32 00        callq   0xffffffff81c01520
> > > > > <__fentry__>
> > > > > ffffffff818d3525: 48 8b 57 60           movq    96(%rdi), %rdx
> > > > > ffffffff818d3529: 48 8b 77 68           movq    104(%rdi), %rsi
> > > > > ffffffff818d352d: 48 8b 7f 70           movq    112(%rdi), %rdi
> > > > > ffffffff818d3531: e8 1a ff ff ff        callq   0xffffffff818d3450
> > > > > <__sys_connect>
> > > > > ffffffff818d3536: 48 98                 cltq
> > > > > ffffffff818d3538: c3                    retq
> > > > > ffffffff818d3539: 0f 1f 80 00 00 00 00  nopl    (%rax)
> > > > >
> > > > > In bpf program, the step is:
> > > > >        struct pt_regs *real_regs = PT_REGS_PARM1(pt_regs);
> > > > >        param1 = PT_REGS_PARM1(real_regs);
> > > > >        param2 = PT_REGS_PARM2(real_regs);
> > > > >        param3 = PT_REGS_PARM3(real_regs);
> > > > > The same for s390.
> > > > >
> > > >
> > > > I'm sorry that I seem to get it wrong,
> > > > But is it available to access 'struct pt_regs *' recursively?
> > > >
> > > > It seems nested use of PT_REGS_PARM causes invalid memory access.
> > > >
> > > >     $ sudo ./test_probe_write_user
> > > >     libbpf: load bpf program failed: Permission denied
> > > >     libbpf: -- BEGIN DUMP LOG ---
> > > >     libbpf:
> > > >     Unrecognized arg#0 type PTR
> > > >     ; struct pt_regs *real_regs = PT_REGS_PARM1(ctx);
> > > >     0: (79) r1 = *(u64 *)(r1 +112)
> > > >     ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
> > > >     1: (79) r6 = *(u64 *)(r1 +104)
> > > >     R1 invalid mem access 'inv'
> > > >     processed 2 insns (limit 1000000) max_states_per_insn 0
> > > > total_states 0 peak_states 0 mark_read 0
> > > >
> > > >     libbpf: -- END LOG --
> > > >     libbpf: failed to load program 'kprobe/__x64_sys_connect'
> > > >     libbpf: failed to load object './test_probe_write_user_kern.o'
> > > >     ERROR: loading BPF object file failed
> > > >
> > > > I'm not fully aware of the BPF verifier's internal structure.
> > > > Is there any workaround to solve this problem?
> > >
> > > You need to use bpf_probe_read_kernel() to get those arguments from
> > > real_args. Or better just use PT_REGS_PARM1_CORE(x) and others, which
> > > does that for you (+ CO-RE relocation).
> > >
> > >
> >
> > Thanks for the tip!
> >
> > I've just tried the old hack '_(P)':
> > (which is similar implementation with BPF_CORE_READ())
> >
> >     #define _(P) ({typeof(P) val = 0; bpf_probe_read(&val,
> > sizeof(val), &P); val;})
> >     [...]
> >     struct pt_regs *regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
> >     void *sockaddr_arg = (void *)_(PT_REGS_PARM2(regs));
> >     int sockaddr_len = (int)_(PT_REGS_PARM3(regs));
> >
> > and it works properly.
> >
> > Just wondering, why is the pointer chasing of the original ctx
> > considered as an unsafe pointer here?
> >
> >     ; struct pt_regs *real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
> >     0: (79) r1 = *(u64 *)(r1 +112)
> >     [...]
> >     ; void *sockaddr_arg = (void *)PT_REGS_PARM2(real_regs);
> >     4: (79) r6 = *(u64 *)(r1 +104)
> >
> > Is it considered as an unsafe pointer since it is unknown what exists
> > in the pointer (r1 + 104), but the instruction is trying to access it?
> >
>
> Yes.
> Because after the initial pointer read, the verifier assumes that you
> are reading a random piece of memory.
>

Thanks for the confirmation. It's very helpful to me.

> >
> > I am a little concerned about using PT_REGS_PARM1_CORE
> > because it is not a CORE-related patch, but if using CORE is the
> > direction BPF wants to take, I will use PT_REGS_PARM1_CORE()
> > instead of _(P) hack using bpf_probe_read().
>
> bpf_probe_read() works as well. But yeah, BPF CO-RE is the way modern
> tracing applications are leaning, look at selftests and see how many
> are using CO-RE already. It's pretty much the only way to write
> portable tracing BPF applications, short of taking Clang/LLVM
> **runtime** dependency, the way BCC makes you do.
>

I see. I'll stick with PT_REGS_PARM1_CORE approach.

I will send the next version of the patch soon.

Thank you for your time and effort for the review.
Daniel

> >
> > In addition, PT_REGS_PARM1_CORE() allows me to write code
> > neatly without having to define additional macro _(P).
> >
> > Thank you for your time and effort for the review.
> > Daniel
> >
> > > >
> > > > Thanks for your time and effort for the review.
> > > > Daniel.
> > > >
> > > > >
> > > > > For other architectures, no above indirection is needed.
> > > > >
> > > > > I guess you can abstract the above into trace_common.h?
> > > > >
> > > > > >
> > > > > > I'm not sure how to deal with this problem. Any advice and suggestions
> > > > > > will be greatly appreciated.
> > > > > >
> > > > > > Thanks for your time and effort for the review.
> > > > > > Daniel
> > > > > >
> > > > > >>>
> > > > > >>> Fixes: 34745aed515c ("samples/bpf: fix kprobe attachment issue on x64")
> > > > > >>> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > > > >>> ---
>
> [...]

[PATCH bpf-next 2/4] samples: bpf: refactor BPF map in map test with libbpf

[Daniel T. Lee]

From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
support"), a way to define internal map in BTF-defined map has been
added.

Instead of using previous 'inner_map_idx' definition, the structure to
be used for the inner map can be directly defined using array directive.

    __array(values, struct inner_map)

This commit refactors map in map test program with libbpf by explicitly
defining inner map with BTF-defined format.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/Makefile               |  2 +-
 samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
 samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
 3 files changed, 91 insertions(+), 49 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index ffd0fda536da..78678d4e6842 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -93,7 +93,7 @@ sampleip-objs := sampleip_user.o $(TRACE_HELPERS)
 tc_l2_redirect-objs := bpf_load.o tc_l2_redirect_user.o
 lwt_len_hist-objs := bpf_load.o lwt_len_hist_user.o
 xdp_tx_iptunnel-objs := xdp_tx_iptunnel_user.o
-test_map_in_map-objs := bpf_load.o test_map_in_map_user.o
+test_map_in_map-objs := test_map_in_map_user.o
 per_socket_stats_example-objs := cookie_uid_helper_example.o
 xdp_redirect-objs := xdp_redirect_user.o
 xdp_redirect_map-objs := xdp_redirect_map_user.o
diff --git a/samples/bpf/test_map_in_map_kern.c b/samples/bpf/test_map_in_map_kern.c
index b1562ba2f025..d3f56ed78541 100644
--- a/samples/bpf/test_map_in_map_kern.c
+++ b/samples/bpf/test_map_in_map_kern.c
@@ -11,66 +11,65 @@
 #include <uapi/linux/bpf.h>
 #include <uapi/linux/in6.h>
 #include <bpf/bpf_helpers.h>
-#include "bpf_legacy.h"
 #include <bpf/bpf_tracing.h>
 
 #define MAX_NR_PORTS 65536
 
 /* map #0 */
-struct bpf_map_def_legacy SEC("maps") port_a = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = MAX_NR_PORTS,
-};
+struct inner_a {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, MAX_NR_PORTS);
+} port_a SEC(".maps");
 
 /* map #1 */
-struct bpf_map_def_legacy SEC("maps") port_h = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = 1,
-};
+struct inner_h {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, 1);
+} port_h SEC(".maps");
 
 /* map #2 */
-struct bpf_map_def_legacy SEC("maps") reg_result_h = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, 1);
+} reg_result_h SEC(".maps");
 
 /* map #3 */
-struct bpf_map_def_legacy SEC("maps") inline_result_h = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(int),
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, int);
+	__uint(max_entries, 1);
+} inline_result_h SEC(".maps");
 
 /* map #4 */ /* Test case #0 */
-struct bpf_map_def_legacy SEC("maps") a_of_port_a = {
-	.type = BPF_MAP_TYPE_ARRAY_OF_MAPS,
-	.key_size = sizeof(u32),
-	.inner_map_idx = 0, /* map_fd[0] is port_a */
-	.max_entries = MAX_NR_PORTS,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+	__uint(max_entries, MAX_NR_PORTS);
+	__uint(key_size, sizeof(u32));
+	__array(values, struct inner_a); /* use inner_a as inner map */
+} a_of_port_a SEC(".maps");
 
 /* map #5 */ /* Test case #1 */
-struct bpf_map_def_legacy SEC("maps") h_of_port_a = {
-	.type = BPF_MAP_TYPE_HASH_OF_MAPS,
-	.key_size = sizeof(u32),
-	.inner_map_idx = 0, /* map_fd[0] is port_a */
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(u32));
+	__array(values, struct inner_a); /* use inner_a as inner map */
+} h_of_port_a SEC(".maps");
 
 /* map #6 */ /* Test case #2 */
-struct bpf_map_def_legacy SEC("maps") h_of_port_h = {
-	.type = BPF_MAP_TYPE_HASH_OF_MAPS,
-	.key_size = sizeof(u32),
-	.inner_map_idx = 1, /* map_fd[1] is port_h */
-	.max_entries = 1,
-};
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(u32));
+	__array(values, struct inner_h); /* use inner_h as inner map */
+} h_of_port_h SEC(".maps");
 
 static __always_inline int do_reg_lookup(void *inner_map, u32 port)
 {
diff --git a/samples/bpf/test_map_in_map_user.c b/samples/bpf/test_map_in_map_user.c
index eb29bcb76f3f..e5bddfff696f 100644
--- a/samples/bpf/test_map_in_map_user.c
+++ b/samples/bpf/test_map_in_map_user.c
@@ -11,7 +11,9 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
+
+static int map_fd[7];
 
 #define PORT_A		(map_fd[0])
 #define PORT_H		(map_fd[1])
@@ -113,18 +115,59 @@ static void test_map_in_map(void)
 int main(int argc, char **argv)
 {
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	struct bpf_link *link = NULL;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
 	char filename[256];
 
-	assert(!setrlimit(RLIMIT_MEMLOCK, &r));
+	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
+		perror("setrlimit(RLIMIT_MEMLOCK)");
+		return 1;
+	}
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+	obj = bpf_object__open_file(filename, NULL);
+	if (libbpf_get_error(obj)) {
+		fprintf(stderr, "ERROR: opening BPF object file failed\n");
+		return 0;
+	}
 
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
+	if (libbpf_get_error(prog)) {
+		printf("finding a prog in obj file failed\n");
+		goto cleanup;
+	}
+
+	/* load BPF program */
+	if (bpf_object__load(obj)) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	map_fd[0] = bpf_object__find_map_fd_by_name(obj, "port_a");
+	map_fd[1] = bpf_object__find_map_fd_by_name(obj, "port_h");
+	map_fd[2] = bpf_object__find_map_fd_by_name(obj, "reg_result_h");
+	map_fd[3] = bpf_object__find_map_fd_by_name(obj, "inline_result_h");
+	map_fd[4] = bpf_object__find_map_fd_by_name(obj, "a_of_port_a");
+	map_fd[5] = bpf_object__find_map_fd_by_name(obj, "h_of_port_a");
+	map_fd[6] = bpf_object__find_map_fd_by_name(obj, "h_of_port_h");
+	if (map_fd[0] < 0 || map_fd[1] < 0 || map_fd[2] < 0 ||
+	    map_fd[3] < 0 || map_fd[4] < 0 || map_fd[5] < 0 || map_fd[6] < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	link = bpf_program__attach(prog);
+	if (libbpf_get_error(link)) {
+		fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+		link = NULL;
+		goto cleanup;
 	}
 
 	test_map_in_map();
 
+cleanup:
+	bpf_link__destroy(link);
+	bpf_object__close(obj);
 	return 0;
 }
-- 
2.25.1

Re: [PATCH bpf-next 2/4] samples: bpf: refactor BPF map in map test with libbpf

[Andrii Nakryiko]

On Wed, Jul 1, 2020 at 7:17 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
> support"), a way to define internal map in BTF-defined map has been
> added.
>
> Instead of using previous 'inner_map_idx' definition, the structure to
> be used for the inner map can be directly defined using array directive.
>
>     __array(values, struct inner_map)
>
> This commit refactors map in map test program with libbpf by explicitly
> defining inner map with BTF-defined format.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
>  samples/bpf/Makefile               |  2 +-
>  samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
>  samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
>  3 files changed, 91 insertions(+), 49 deletions(-)
>

[...]

> -       if (load_bpf_file(filename)) {
> -               printf("%s", bpf_log_buf);
> -               return 1;
> +       prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
> +       if (libbpf_get_error(prog)) {

still wrong, just `if (!prog)`

> +               printf("finding a prog in obj file failed\n");
> +               goto cleanup;
> +       }
> +

[...]

Re: [PATCH bpf-next 2/4] samples: bpf: refactor BPF map in map test with libbpf

[Daniel T. Lee]

On Thu, Jul 2, 2020 at 1:26 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 1, 2020 at 7:17 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > From commit 646f02ffdd49 ("libbpf: Add BTF-defined map-in-map
> > support"), a way to define internal map in BTF-defined map has been
> > added.
> >
> > Instead of using previous 'inner_map_idx' definition, the structure to
> > be used for the inner map can be directly defined using array directive.
> >
> >     __array(values, struct inner_map)
> >
> > This commit refactors map in map test program with libbpf by explicitly
> > defining inner map with BTF-defined format.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> >  samples/bpf/Makefile               |  2 +-
> >  samples/bpf/test_map_in_map_kern.c | 85 +++++++++++++++---------------
> >  samples/bpf/test_map_in_map_user.c | 53 +++++++++++++++++--
> >  3 files changed, 91 insertions(+), 49 deletions(-)
> >
>
> [...]
>
> > -       if (load_bpf_file(filename)) {
> > -               printf("%s", bpf_log_buf);
> > -               return 1;
> > +       prog = bpf_object__find_program_by_name(obj, "trace_sys_connect");
> > +       if (libbpf_get_error(prog)) {
>
> still wrong, just `if (!prog)`
>

Oops, my bad.
Will fix right away.

Thanks for your time and effort for the review.
Daniel.

> > +               printf("finding a prog in obj file failed\n");
> > +               goto cleanup;
> > +       }
> > +
>
> [...]

[PATCH bpf-next 3/4] samples: bpf: refactor BPF map performance test with libbpf

[Daniel T. Lee]

Previously, in order to set the numa_node attribute at the time of map
creation using "libbpf", it was necessary to call bpf_create_map_node()
directly (bpf_load approach), instead of calling bpf_object_load()
that handles everything on its own, including map creation. And because
of this problem, this sample had problems with refactoring from bpf_load
to libbbpf.

However, by commit 1bdb6c9a1c43 ("libbpf: Add a bunch of attribute
getters/setters for map definitions"), a helper function which allows
the numa_node attribute to be set in the map prior to calling
bpf_object_load() has been added.

By using libbpf instead of bpf_load, the inner map definition has
been explicitly declared with BTF-defined format. And for this reason
some logic in fixup_map() was not needed and changed or removed.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 samples/bpf/Makefile             |   2 +-
 samples/bpf/map_perf_test_kern.c | 180 +++++++++++++++----------------
 samples/bpf/map_perf_test_user.c | 130 +++++++++++++++-------
 3 files changed, 181 insertions(+), 131 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 78678d4e6842..0cc7f18370c6 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -76,7 +76,7 @@ trace_output-objs := trace_output_user.o $(TRACE_HELPERS)
 lathist-objs := lathist_user.o
 offwaketime-objs := offwaketime_user.o $(TRACE_HELPERS)
 spintest-objs := spintest_user.o $(TRACE_HELPERS)
-map_perf_test-objs := bpf_load.o map_perf_test_user.o
+map_perf_test-objs := map_perf_test_user.o
 test_overhead-objs := bpf_load.o test_overhead_user.o
 test_cgrp2_array_pin-objs := test_cgrp2_array_pin.o
 test_cgrp2_attach-objs := test_cgrp2_attach.o
diff --git a/samples/bpf/map_perf_test_kern.c b/samples/bpf/map_perf_test_kern.c
index cebe2098bb24..13ca14e34f66 100644
--- a/samples/bpf/map_perf_test_kern.c
+++ b/samples/bpf/map_perf_test_kern.c
@@ -9,95 +9,95 @@
 #include <linux/version.h>
 #include <uapi/linux/bpf.h>
 #include <bpf/bpf_helpers.h>
-#include "bpf_legacy.h"
 #include <bpf/bpf_tracing.h>
+#include "trace_common.h"
 
 #define MAX_ENTRIES 1000
 #define MAX_NR_CPUS 1024
 
-struct bpf_map_def_legacy SEC("maps") hash_map = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = MAX_ENTRIES,
-};
-
-struct bpf_map_def_legacy SEC("maps") lru_hash_map = {
-	.type = BPF_MAP_TYPE_LRU_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = 10000,
-};
-
-struct bpf_map_def_legacy SEC("maps") nocommon_lru_hash_map = {
-	.type = BPF_MAP_TYPE_LRU_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = 10000,
-	.map_flags = BPF_F_NO_COMMON_LRU,
-};
-
-struct bpf_map_def_legacy SEC("maps") inner_lru_hash_map = {
-	.type = BPF_MAP_TYPE_LRU_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = MAX_ENTRIES,
-	.map_flags = BPF_F_NUMA_NODE,
-	.numa_node = 0,
-};
-
-struct bpf_map_def_legacy SEC("maps") array_of_lru_hashs = {
-	.type = BPF_MAP_TYPE_ARRAY_OF_MAPS,
-	.key_size = sizeof(u32),
-	.max_entries = MAX_NR_CPUS,
-};
-
-struct bpf_map_def_legacy SEC("maps") percpu_hash_map = {
-	.type = BPF_MAP_TYPE_PERCPU_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = MAX_ENTRIES,
-};
-
-struct bpf_map_def_legacy SEC("maps") hash_map_alloc = {
-	.type = BPF_MAP_TYPE_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = MAX_ENTRIES,
-	.map_flags = BPF_F_NO_PREALLOC,
-};
-
-struct bpf_map_def_legacy SEC("maps") percpu_hash_map_alloc = {
-	.type = BPF_MAP_TYPE_PERCPU_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = MAX_ENTRIES,
-	.map_flags = BPF_F_NO_PREALLOC,
-};
-
-struct bpf_map_def_legacy SEC("maps") lpm_trie_map_alloc = {
-	.type = BPF_MAP_TYPE_LPM_TRIE,
-	.key_size = 8,
-	.value_size = sizeof(long),
-	.max_entries = 10000,
-	.map_flags = BPF_F_NO_PREALLOC,
-};
-
-struct bpf_map_def_legacy SEC("maps") array_map = {
-	.type = BPF_MAP_TYPE_ARRAY,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = MAX_ENTRIES,
-};
-
-struct bpf_map_def_legacy SEC("maps") lru_hash_lookup_map = {
-	.type = BPF_MAP_TYPE_LRU_HASH,
-	.key_size = sizeof(u32),
-	.value_size = sizeof(long),
-	.max_entries = MAX_ENTRIES,
-};
-
-SEC("kprobe/sys_getuid")
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, long);
+	__uint(max_entries, MAX_ENTRIES);
+} hash_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_LRU_HASH);
+	__type(key, u32);
+	__type(value, long);
+	__uint(max_entries, 10000);
+} lru_hash_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_LRU_HASH);
+	__type(key, u32);
+	__type(value, long);
+	__uint(max_entries, 10000);
+	__uint(map_flags, BPF_F_NO_COMMON_LRU);
+} nocommon_lru_hash_map SEC(".maps");
+
+struct inner_lru {
+	__uint(type, BPF_MAP_TYPE_LRU_HASH);
+	__type(key, u32);
+	__type(value, long);
+	__uint(max_entries, MAX_ENTRIES);
+	__uint(map_flags, BPF_F_NUMA_NODE); /* from _user.c, set numa_node to 0 */
+} inner_lru_hash_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+	__uint(max_entries, MAX_NR_CPUS);
+	__uint(key_size, sizeof(u32));
+	__array(values, struct inner_lru); /* use inner_lru as inner map */
+} array_of_lru_hashs SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_HASH);
+	__uint(key_size, sizeof(u32));
+	__uint(value_size, sizeof(long));
+	__uint(max_entries, MAX_ENTRIES);
+} percpu_hash_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, u32);
+	__type(value, long);
+	__uint(max_entries, MAX_ENTRIES);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+} hash_map_alloc SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_HASH);
+	__uint(key_size, sizeof(u32));
+	__uint(value_size, sizeof(long));
+	__uint(max_entries, MAX_ENTRIES);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+} percpu_hash_map_alloc SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_LPM_TRIE);
+	__uint(key_size, 8);
+	__uint(value_size, sizeof(long));
+	__uint(max_entries, 10000);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+} lpm_trie_map_alloc SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, u32);
+	__type(value, long);
+	__uint(max_entries, MAX_ENTRIES);
+} array_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_LRU_HASH);
+	__type(key, u32);
+	__type(value, long);
+	__uint(max_entries, MAX_ENTRIES);
+} lru_hash_lookup_map SEC(".maps");
+
+SEC("kprobe/" SYSCALL(sys_getuid))
 int stress_hmap(struct pt_regs *ctx)
 {
 	u32 key = bpf_get_current_pid_tgid();
@@ -112,7 +112,7 @@ int stress_hmap(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_geteuid")
+SEC("kprobe/" SYSCALL(sys_geteuid))
 int stress_percpu_hmap(struct pt_regs *ctx)
 {
 	u32 key = bpf_get_current_pid_tgid();
@@ -126,7 +126,7 @@ int stress_percpu_hmap(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_getgid")
+SEC("kprobe/" SYSCALL(sys_getgid))
 int stress_hmap_alloc(struct pt_regs *ctx)
 {
 	u32 key = bpf_get_current_pid_tgid();
@@ -140,7 +140,7 @@ int stress_hmap_alloc(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_getegid")
+SEC("kprobe/" SYSCALL(sys_getegid))
 int stress_percpu_hmap_alloc(struct pt_regs *ctx)
 {
 	u32 key = bpf_get_current_pid_tgid();
@@ -233,7 +233,7 @@ int stress_lru_hmap_alloc(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_gettid")
+SEC("kprobe/" SYSCALL(sys_gettid))
 int stress_lpm_trie_map_alloc(struct pt_regs *ctx)
 {
 	union {
@@ -255,7 +255,7 @@ int stress_lpm_trie_map_alloc(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_getpgid")
+SEC("kprobe/" SYSCALL(sys_getpgid))
 int stress_hash_map_lookup(struct pt_regs *ctx)
 {
 	u32 key = 1, i;
@@ -268,7 +268,7 @@ int stress_hash_map_lookup(struct pt_regs *ctx)
 	return 0;
 }
 
-SEC("kprobe/sys_getppid")
+SEC("kprobe/" SYSCALL(sys_getppid))
 int stress_array_map_lookup(struct pt_regs *ctx)
 {
 	u32 key = 1, i;
diff --git a/samples/bpf/map_perf_test_user.c b/samples/bpf/map_perf_test_user.c
index fe5564bff39b..e067bda07fd7 100644
--- a/samples/bpf/map_perf_test_user.c
+++ b/samples/bpf/map_perf_test_user.c
@@ -11,7 +11,6 @@
 #include <sys/wait.h>
 #include <stdlib.h>
 #include <signal.h>
-#include <linux/bpf.h>
 #include <string.h>
 #include <time.h>
 #include <sys/resource.h>
@@ -19,7 +18,7 @@
 #include <errno.h>
 
 #include <bpf/bpf.h>
-#include "bpf_load.h"
+#include <bpf/libbpf.h>
 
 #define TEST_BIT(t) (1U << (t))
 #define MAX_NR_CPUS 1024
@@ -61,12 +60,19 @@ const char *test_map_names[NR_TESTS] = {
 	[LRU_HASH_LOOKUP] = "lru_hash_lookup_map",
 };
 
+enum map_idx {
+	inner_lru_hash_idx,
+	array_of_lru_hashs_idx,
+	hash_map_alloc_idx,
+	lru_hash_lookup_idx,
+	NR_IDXES,
+};
+
+static int map_fd[NR_IDXES];
+
 static int test_flags = ~0;
 static uint32_t num_map_entries;
 static uint32_t inner_lru_hash_size;
-static int inner_lru_hash_idx = -1;
-static int array_of_lru_hashs_idx = -1;
-static int lru_hash_lookup_idx = -1;
 static int lru_hash_lookup_test_entries = 32;
 static uint32_t max_cnt = 1000000;
 
@@ -377,7 +383,8 @@ static void fill_lpm_trie(void)
 		key->data[1] = rand() & 0xff;
 		key->data[2] = rand() & 0xff;
 		key->data[3] = rand() & 0xff;
-		r = bpf_map_update_elem(map_fd[6], key, &value, 0);
+		r = bpf_map_update_elem(map_fd[hash_map_alloc_idx],
+					key, &value, 0);
 		assert(!r);
 	}
 
@@ -388,59 +395,52 @@ static void fill_lpm_trie(void)
 	key->data[3] = 1;
 	value = 128;
 
-	r = bpf_map_update_elem(map_fd[6], key, &value, 0);
+	r = bpf_map_update_elem(map_fd[hash_map_alloc_idx], key, &value, 0);
 	assert(!r);
 }
 
-static void fixup_map(struct bpf_map_data *map, int idx)
+static void fixup_map(struct bpf_object *obj)
 {
+	struct bpf_map *map;
 	int i;
 
-	if (!strcmp("inner_lru_hash_map", map->name)) {
-		inner_lru_hash_idx = idx;
-		inner_lru_hash_size = map->def.max_entries;
-	}
+	bpf_object__for_each_map(map, obj) {
+		const char *name = bpf_map__name(map);
 
-	if (!strcmp("array_of_lru_hashs", map->name)) {
-		if (inner_lru_hash_idx == -1) {
-			printf("inner_lru_hash_map must be defined before array_of_lru_hashs\n");
-			exit(1);
+		/* Only change the max_entries for the enabled test(s) */
+		for (i = 0; i < NR_TESTS; i++) {
+			if (!strcmp(test_map_names[i], name) &&
+			    (check_test_flags(i))) {
+				bpf_map__resize(map, num_map_entries);
+				continue;
+			}
 		}
-		map->def.inner_map_idx = inner_lru_hash_idx;
-		array_of_lru_hashs_idx = idx;
 	}
 
-	if (!strcmp("lru_hash_lookup_map", map->name))
-		lru_hash_lookup_idx = idx;
-
-	if (num_map_entries <= 0)
-		return;
-
 	inner_lru_hash_size = num_map_entries;
-
-	/* Only change the max_entries for the enabled test(s) */
-	for (i = 0; i < NR_TESTS; i++) {
-		if (!strcmp(test_map_names[i], map->name) &&
-		    (check_test_flags(i))) {
-			map->def.max_entries = num_map_entries;
-		}
-	}
 }
 
 int main(int argc, char **argv)
 {
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+	struct bpf_link *links[8];
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	struct bpf_map *map;
 	char filename[256];
-	int num_cpu = 8;
+	int err, i = 0;
 
-	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-	setrlimit(RLIMIT_MEMLOCK, &r);
+	if (setrlimit(RLIMIT_MEMLOCK, &r)) {
+		perror("setrlimit(RLIMIT_MEMLOCK)");
+		return 1;
+	}
 
 	if (argc > 1)
 		test_flags = atoi(argv[1]) ? : test_flags;
 
 	if (argc > 2)
-		num_cpu = atoi(argv[2]) ? : num_cpu;
+		nr_cpus = atoi(argv[2]) ? : nr_cpus;
 
 	if (argc > 3)
 		num_map_entries = atoi(argv[3]);
@@ -448,14 +448,64 @@ int main(int argc, char **argv)
 	if (argc > 4)
 		max_cnt = atoi(argv[4]);
 
-	if (load_bpf_file_fixup_map(filename, fixup_map)) {
-		printf("%s", bpf_log_buf);
-		return 1;
+	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+	obj = bpf_object__open_file(filename, NULL);
+	if (libbpf_get_error(obj)) {
+		fprintf(stderr, "ERROR: opening BPF object file failed\n");
+		return 0;
+	}
+
+	map = bpf_object__find_map_by_name(obj, "inner_lru_hash_map");
+	if (libbpf_get_error(map)) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	/* set inner_lru_hash_map numa_node attribute to 0 */
+	err = bpf_map__set_numa_node(map, 0);
+	inner_lru_hash_size = bpf_map__max_entries(map);
+	if (err || !inner_lru_hash_size) {
+		fprintf(stderr, "ERROR: failed to set/get map attribute\n");
+		goto cleanup;
+	}
+
+	/* resize BPF map prior to loading */
+	if (num_map_entries > 0)
+		fixup_map(obj);
+
+	/* load BPF program */
+	if (bpf_object__load(obj)) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	map_fd[0] = bpf_map__fd(map);
+	map_fd[1] = bpf_object__find_map_fd_by_name(obj, "array_of_lru_hashs");
+	map_fd[2] = bpf_object__find_map_fd_by_name(obj, "hash_map_alloc");
+	map_fd[3] = bpf_object__find_map_fd_by_name(obj, "lru_hash_lookup_map");
+	if (map_fd[0] < 0 || map_fd[1] < 0 || map_fd[2] < 0 || map_fd[3] < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		goto cleanup;
+	}
+
+	bpf_object__for_each_program(prog, obj) {
+		links[i] = bpf_program__attach(prog);
+		if (libbpf_get_error(links[i])) {
+			fprintf(stderr, "ERROR: bpf_program__attach failed\n");
+			links[i] = NULL;
+			goto cleanup;
+		}
+		i++;
 	}
 
 	fill_lpm_trie();
 
-	run_perf_test(num_cpu);
+	run_perf_test(nr_cpus);
+
+cleanup:
+	for (i--; i >= 0; i--)
+		bpf_link__destroy(links[i]);
 
+	bpf_object__close(obj);
 	return 0;
 }
-- 
2.25.1

Re: [PATCH bpf-next 3/4] samples: bpf: refactor BPF map performance test with libbpf

[Andrii Nakryiko]

On Wed, Jul 1, 2020 at 7:17 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> Previously, in order to set the numa_node attribute at the time of map
> creation using "libbpf", it was necessary to call bpf_create_map_node()
> directly (bpf_load approach), instead of calling bpf_object_load()
> that handles everything on its own, including map creation. And because
> of this problem, this sample had problems with refactoring from bpf_load
> to libbbpf.
>
> However, by commit 1bdb6c9a1c43 ("libbpf: Add a bunch of attribute
> getters/setters for map definitions"), a helper function which allows
> the numa_node attribute to be set in the map prior to calling
> bpf_object_load() has been added.
>
> By using libbpf instead of bpf_load, the inner map definition has
> been explicitly declared with BTF-defined format. And for this reason
> some logic in fixup_map() was not needed and changed or removed.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
>  samples/bpf/Makefile             |   2 +-
>  samples/bpf/map_perf_test_kern.c | 180 +++++++++++++++----------------
>  samples/bpf/map_perf_test_user.c | 130 +++++++++++++++-------
>  3 files changed, 181 insertions(+), 131 deletions(-)
>

[...]

> +struct inner_lru {
> +       __uint(type, BPF_MAP_TYPE_LRU_HASH);
> +       __type(key, u32);
> +       __type(value, long);
> +       __uint(max_entries, MAX_ENTRIES);
> +       __uint(map_flags, BPF_F_NUMA_NODE); /* from _user.c, set numa_node to 0 */
> +} inner_lru_hash_map SEC(".maps");

you can declaratively set numa_node here with __uint(numa_node, 0),
which is actually a default, but for explicitness it's better

> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> +       __uint(max_entries, MAX_NR_CPUS);
> +       __uint(key_size, sizeof(u32));
> +       __array(values, struct inner_lru); /* use inner_lru as inner map */
> +} array_of_lru_hashs SEC(".maps");
> +

[...]

> -static void fixup_map(struct bpf_map_data *map, int idx)
> +static void fixup_map(struct bpf_object *obj)
>  {
> +       struct bpf_map *map;
>         int i;
>
> -       if (!strcmp("inner_lru_hash_map", map->name)) {
> -               inner_lru_hash_idx = idx;
> -               inner_lru_hash_size = map->def.max_entries;
> -       }
> +       bpf_object__for_each_map(map, obj) {
> +               const char *name = bpf_map__name(map);
>
> -       if (!strcmp("array_of_lru_hashs", map->name)) {

I'm a bit too lazy right now to figure out exact logic here, but just
wanted to mention that it is possible to statically set inner map
elements for array_of_maps and hash_of_maps. Please check
tools/testing/selftests/bpf/progs/test_btf_map_in_map.c and see if you
can use this feature to simplify this logic a bit.

> -               if (inner_lru_hash_idx == -1) {
> -                       printf("inner_lru_hash_map must be defined before array_of_lru_hashs\n");
> -                       exit(1);
> +               /* Only change the max_entries for the enabled test(s) */
> +               for (i = 0; i < NR_TESTS; i++) {
> +                       if (!strcmp(test_map_names[i], name) &&
> +                           (check_test_flags(i))) {
> +                               bpf_map__resize(map, num_map_entries);
> +                               continue;
> +                       }
>                 }
> -               map->def.inner_map_idx = inner_lru_hash_idx;
> -               array_of_lru_hashs_idx = idx;
>         }
>

[...]

Re: [PATCH bpf-next 3/4] samples: bpf: refactor BPF map performance test with libbpf

[Daniel T. Lee]

On Thu, Jul 2, 2020 at 1:34 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 1, 2020 at 7:17 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > Previously, in order to set the numa_node attribute at the time of map
> > creation using "libbpf", it was necessary to call bpf_create_map_node()
> > directly (bpf_load approach), instead of calling bpf_object_load()
> > that handles everything on its own, including map creation. And because
> > of this problem, this sample had problems with refactoring from bpf_load
> > to libbbpf.
> >
> > However, by commit 1bdb6c9a1c43 ("libbpf: Add a bunch of attribute
> > getters/setters for map definitions"), a helper function which allows
> > the numa_node attribute to be set in the map prior to calling
> > bpf_object_load() has been added.
> >
> > By using libbpf instead of bpf_load, the inner map definition has
> > been explicitly declared with BTF-defined format. And for this reason
> > some logic in fixup_map() was not needed and changed or removed.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> >  samples/bpf/Makefile             |   2 +-
> >  samples/bpf/map_perf_test_kern.c | 180 +++++++++++++++----------------
> >  samples/bpf/map_perf_test_user.c | 130 +++++++++++++++-------
> >  3 files changed, 181 insertions(+), 131 deletions(-)
> >
>
> [...]
>
> > +struct inner_lru {
> > +       __uint(type, BPF_MAP_TYPE_LRU_HASH);
> > +       __type(key, u32);
> > +       __type(value, long);
> > +       __uint(max_entries, MAX_ENTRIES);
> > +       __uint(map_flags, BPF_F_NUMA_NODE); /* from _user.c, set numa_node to 0 */
> > +} inner_lru_hash_map SEC(".maps");
>
> you can declaratively set numa_node here with __uint(numa_node, 0),
> which is actually a default, but for explicitness it's better
>

It would make _user.c code cleaner, but as you said,
I'll keep with this implementation.

> > +
> > +struct {
> > +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> > +       __uint(max_entries, MAX_NR_CPUS);
> > +       __uint(key_size, sizeof(u32));
> > +       __array(values, struct inner_lru); /* use inner_lru as inner map */
> > +} array_of_lru_hashs SEC(".maps");
> > +
>
> [...]
>
> > -static void fixup_map(struct bpf_map_data *map, int idx)
> > +static void fixup_map(struct bpf_object *obj)
> >  {
> > +       struct bpf_map *map;
> >         int i;
> >
> > -       if (!strcmp("inner_lru_hash_map", map->name)) {
> > -               inner_lru_hash_idx = idx;
> > -               inner_lru_hash_size = map->def.max_entries;
> > -       }
> > +       bpf_object__for_each_map(map, obj) {
> > +               const char *name = bpf_map__name(map);
> >
> > -       if (!strcmp("array_of_lru_hashs", map->name)) {
>
> I'm a bit too lazy right now to figure out exact logic here, but just
> wanted to mention that it is possible to statically set inner map
> elements for array_of_maps and hash_of_maps. Please check
> tools/testing/selftests/bpf/progs/test_btf_map_in_map.c and see if you
> can use this feature to simplify this logic a bit.
>

Thanks for the feedback! But I'm not sure I'm following properly.

If what you are talking about is specifying the inner_map_idx of
array_of_lru_hashes, I've changed it by using the __array() directives
of the BTF-defined MAP.

Since inner_map_idx logic has been replaced with BTF-defined map
definition, the only thing left at here fixup_map() is just resizing map size
with bpf_map__resize.

Thanks for your time and effort for the review.
Daniel

> > -               if (inner_lru_hash_idx == -1) {
> > -                       printf("inner_lru_hash_map must be defined before array_of_lru_hashs\n");
> > -                       exit(1);
> > +               /* Only change the max_entries for the enabled test(s) */
> > +               for (i = 0; i < NR_TESTS; i++) {
> > +                       if (!strcmp(test_map_names[i], name) &&
> > +                           (check_test_flags(i))) {
> > +                               bpf_map__resize(map, num_map_entries);
> > +                               continue;
> > +                       }
> >                 }
> > -               map->def.inner_map_idx = inner_lru_hash_idx;
> > -               array_of_lru_hashs_idx = idx;
> >         }
> >
>
> [...]

Re: [PATCH bpf-next 3/4] samples: bpf: refactor BPF map performance test with libbpf

[Andrii Nakryiko]

On Thu, Jul 2, 2020 at 4:24 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> On Thu, Jul 2, 2020 at 1:34 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Jul 1, 2020 at 7:17 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > >
> > > Previously, in order to set the numa_node attribute at the time of map
> > > creation using "libbpf", it was necessary to call bpf_create_map_node()
> > > directly (bpf_load approach), instead of calling bpf_object_load()
> > > that handles everything on its own, including map creation. And because
> > > of this problem, this sample had problems with refactoring from bpf_load
> > > to libbbpf.
> > >
> > > However, by commit 1bdb6c9a1c43 ("libbpf: Add a bunch of attribute
> > > getters/setters for map definitions"), a helper function which allows
> > > the numa_node attribute to be set in the map prior to calling
> > > bpf_object_load() has been added.
> > >
> > > By using libbpf instead of bpf_load, the inner map definition has
> > > been explicitly declared with BTF-defined format. And for this reason
> > > some logic in fixup_map() was not needed and changed or removed.
> > >
> > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > ---
> > >  samples/bpf/Makefile             |   2 +-
> > >  samples/bpf/map_perf_test_kern.c | 180 +++++++++++++++----------------
> > >  samples/bpf/map_perf_test_user.c | 130 +++++++++++++++-------
> > >  3 files changed, 181 insertions(+), 131 deletions(-)
> > >
> >
> > [...]
> >
> > > +struct inner_lru {
> > > +       __uint(type, BPF_MAP_TYPE_LRU_HASH);
> > > +       __type(key, u32);
> > > +       __type(value, long);
> > > +       __uint(max_entries, MAX_ENTRIES);
> > > +       __uint(map_flags, BPF_F_NUMA_NODE); /* from _user.c, set numa_node to 0 */
> > > +} inner_lru_hash_map SEC(".maps");
> >
> > you can declaratively set numa_node here with __uint(numa_node, 0),
> > which is actually a default, but for explicitness it's better
> >
>
> It would make _user.c code cleaner, but as you said,
> I'll keep with this implementation.

I meant to do __uint(numa_node, 0) declaratively, even if it's a bit
redundant (because default value is already zero).

user-space bpf_program__numa_node() is inferior for such a simple
static use case.

>
> > > +
> > > +struct {
> > > +       __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
> > > +       __uint(max_entries, MAX_NR_CPUS);
> > > +       __uint(key_size, sizeof(u32));
> > > +       __array(values, struct inner_lru); /* use inner_lru as inner map */
> > > +} array_of_lru_hashs SEC(".maps");
> > > +
> >
> > [...]
> >
> > > -static void fixup_map(struct bpf_map_data *map, int idx)
> > > +static void fixup_map(struct bpf_object *obj)
> > >  {
> > > +       struct bpf_map *map;
> > >         int i;
> > >
> > > -       if (!strcmp("inner_lru_hash_map", map->name)) {
> > > -               inner_lru_hash_idx = idx;
> > > -               inner_lru_hash_size = map->def.max_entries;
> > > -       }
> > > +       bpf_object__for_each_map(map, obj) {
> > > +               const char *name = bpf_map__name(map);
> > >
> > > -       if (!strcmp("array_of_lru_hashs", map->name)) {
> >
> > I'm a bit too lazy right now to figure out exact logic here, but just
> > wanted to mention that it is possible to statically set inner map
> > elements for array_of_maps and hash_of_maps. Please check
> > tools/testing/selftests/bpf/progs/test_btf_map_in_map.c and see if you
> > can use this feature to simplify this logic a bit.
> >
>
> Thanks for the feedback! But I'm not sure I'm following properly.
>
> If what you are talking about is specifying the inner_map_idx of
> array_of_lru_hashes, I've changed it by using the __array() directives
> of the BTF-defined MAP.
>
> Since inner_map_idx logic has been replaced with BTF-defined map
> definition, the only thing left at here fixup_map() is just resizing map size
> with bpf_map__resize.

Ok, as I said, a bit too lazy to try to figure out the entire logic of
this sample. My point was to check if static initialization of
ARRAY_OF_MAPS/HASH_OF_MAPS elements is doable. If not, it's fine as is
as well.

>
> Thanks for your time and effort for the review.
> Daniel
>
> > > -               if (inner_lru_hash_idx == -1) {
> > > -                       printf("inner_lru_hash_map must be defined before array_of_lru_hashs\n");
> > > -                       exit(1);
> > > +               /* Only change the max_entries for the enabled test(s) */
> > > +               for (i = 0; i < NR_TESTS; i++) {
> > > +                       if (!strcmp(test_map_names[i], name) &&
> > > +                           (check_test_flags(i))) {
> > > +                               bpf_map__resize(map, num_map_entries);
> > > +                               continue;
> > > +                       }
> > >                 }
> > > -               map->def.inner_map_idx = inner_lru_hash_idx;
> > > -               array_of_lru_hashs_idx = idx;
> > >         }
> > >
> >
> > [...]

[PATCH bpf-next 4/4] selftests: bpf: remove unused bpf_map_def_legacy struct

[Daniel T. Lee]

samples/bpf no longer use bpf_map_def_legacy and instead use the
libbpf's bpf_map_def or new BTF-defined MAP format. This commit removes
unused bpf_map_def_legacy struct from selftests/bpf/bpf_legacy.h.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 tools/testing/selftests/bpf/bpf_legacy.h | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_legacy.h b/tools/testing/selftests/bpf/bpf_legacy.h
index 6f8988738bc1..719ab56cdb5d 100644
--- a/tools/testing/selftests/bpf/bpf_legacy.h
+++ b/tools/testing/selftests/bpf/bpf_legacy.h
@@ -2,20 +2,6 @@
 #ifndef __BPF_LEGACY__
 #define __BPF_LEGACY__
 
-/*
- * legacy bpf_map_def with extra fields supported only by bpf_load(), do not
- * use outside of samples/bpf
- */
-struct bpf_map_def_legacy {
-	unsigned int type;
-	unsigned int key_size;
-	unsigned int value_size;
-	unsigned int max_entries;
-	unsigned int map_flags;
-	unsigned int inner_map_idx;
-	unsigned int numa_node;
-};
-
 #define BPF_ANNOTATE_KV_PAIR(name, type_key, type_val)		\
 	struct ____btf_map_##name {				\
 		type_key key;					\
-- 
2.25.1

Re: [PATCH bpf-next 4/4] selftests: bpf: remove unused bpf_map_def_legacy struct

[Andrii Nakryiko]

On Wed, Jul 1, 2020 at 7:18 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> samples/bpf no longer use bpf_map_def_legacy and instead use the
> libbpf's bpf_map_def or new BTF-defined MAP format. This commit removes
> unused bpf_map_def_legacy struct from selftests/bpf/bpf_legacy.h.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---

Great, long time coming! Thank you! Some day entire bpf_load.{c,h}
will also be gone :)

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/bpf_legacy.h | 14 --------------
>  1 file changed, 14 deletions(-)
>

[...]