[PATCH v2 1/2] selftests/bpf: ksym_search won't check symbols exists

[PATCH v2 1/2] selftests/bpf: ksym_search won't check symbols exists

[Daniel T. Lee]

Currently, ksym_search located at trace_helpers won't check symbols are
existing or not.

In ksym_search, when symbol is not found, it will return &syms[0](_stext).
But when the kernel symbols are not loaded, it will return NULL, which is
not a desired action.

This commit will add verification logic whether symbols are loaded prior
to the symbol search.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 tools/testing/selftests/bpf/trace_helpers.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 4cdb63bf0521..9a9fc6c9b70b 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -52,6 +52,10 @@ struct ksym *ksym_search(long key)
 	int start = 0, end = sym_cnt;
 	int result;
 
+	/* kallsyms not loaded. return NULL */
+	if (sym_cnt <= 0)
+		return NULL;
+
 	while (start < end) {
 		size_t mid = start + (end - start) / 2;
 
-- 
2.17.1

[PATCH v2 2/2] samples, selftests/bpf: add NULL check for ksym_search

[Daniel T. Lee]

Since, ksym_search added with verification logic for symbols existence,
it could return NULL when the kernel symbols are not loaded.

This commit will add NULL check logic after ksym_search.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
Changes in v2:
    - Added NULL check for selftests/bpf/prog_tests/get_stack_raw_tp.c

 samples/bpf/offwaketime_user.c                            | 5 +++++
 samples/bpf/sampleip_user.c                               | 5 +++++
 samples/bpf/trace_event_user.c                            | 5 +++++
 tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c | 4 ++--
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/samples/bpf/offwaketime_user.c b/samples/bpf/offwaketime_user.c
index f06063af9fcb..bb315ce1b866 100644
--- a/samples/bpf/offwaketime_user.c
+++ b/samples/bpf/offwaketime_user.c
@@ -28,6 +28,11 @@ static void print_ksym(__u64 addr)
 	if (!addr)
 		return;
 	sym = ksym_search(addr);
+	if (!sym) {
+		printf("ksym not found. Is kallsyms loaded?\n");
+		return;
+	}
+
 	if (PRINT_RAW_ADDR)
 		printf("%s/%llx;", sym->name, addr);
 	else
diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
index 216c7ecbbbe9..23b90a45c802 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c
@@ -109,6 +109,11 @@ static void print_ip_map(int fd)
 	for (i = 0; i < max; i++) {
 		if (counts[i].ip > PAGE_OFFSET) {
 			sym = ksym_search(counts[i].ip);
+			if (!sym) {
+				printf("ksym not found. Is kallsyms loaded?\n");
+				continue;
+			}
+
 			printf("0x%-17llx %-32s %u\n", counts[i].ip, sym->name,
 			       counts[i].count);
 		} else {
diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index d08046ab81f0..d4178f60e075 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c
@@ -34,6 +34,11 @@ static void print_ksym(__u64 addr)
 	if (!addr)
 		return;
 	sym = ksym_search(addr);
+	if (!sym) {
+		printf("ksym not found. Is kallsyms loaded?\n");
+		return;
+	}
+
 	printf("%s;", sym->name);
 	if (!strcmp(sym->name, "sys_read"))
 		sys_read_seen = true;
diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
index d7bb5beb1c57..c2a0a9d5591b 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
@@ -39,7 +39,7 @@ static int get_stack_print_output(void *data, int size)
 		} else {
 			for (i = 0; i < num_stack; i++) {
 				ks = ksym_search(raw_data[i]);
-				if (strcmp(ks->name, nonjit_func) == 0) {
+				if (ks && (strcmp(ks->name, nonjit_func) == 0)) {
 					found = true;
 					break;
 				}
@@ -56,7 +56,7 @@ static int get_stack_print_output(void *data, int size)
 		} else {
 			for (i = 0; i < num_stack; i++) {
 				ks = ksym_search(e->kern_stack[i]);
-				if (strcmp(ks->name, nonjit_func) == 0) {
+				if (ks && (strcmp(ks->name, nonjit_func) == 0)) {
 					good_kern_stack = true;
 					break;
 				}
-- 
2.17.1

Re: [PATCH v2 2/2] samples, selftests/bpf: add NULL check for ksym_search

[Daniel Borkmann]

On 03/27/2019 03:45 PM, Daniel T. Lee wrote:
> Since, ksym_search added with verification logic for symbols existence,
> it could return NULL when the kernel symbols are not loaded.
> 
> This commit will add NULL check logic after ksym_search.
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
> Changes in v2:
>     - Added NULL check for selftests/bpf/prog_tests/get_stack_raw_tp.c
> 
>  samples/bpf/offwaketime_user.c                            | 5 +++++
>  samples/bpf/sampleip_user.c                               | 5 +++++
>  samples/bpf/trace_event_user.c                            | 5 +++++
>  tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c | 4 ++--
>  4 files changed, 17 insertions(+), 2 deletions(-)

Still not all occurrences from samples covered:

$ git grep -n ksym_search samples/bpf/
samples/bpf/offwaketime_user.c:30:      sym = ksym_search(addr);
samples/bpf/sampleip_user.c:111:                        sym = ksym_search(counts[i].ip);
samples/bpf/spintest_user.c:39:                 sym = ksym_search(value);
samples/bpf/trace_event_user.c:36:      sym = ksym_search(addr);

Re: [PATCH v2 2/2] samples, selftests/bpf: add NULL check for ksym_search

[Daniel T. Lee]

I'm aware that I didn't cover the "spintest_user.c".
Because, spintest sample isn't running currently since there is no
such symbol called 'spin_lock'.

At "spintest_kern.c", it has 'kprobe/spin_lock' and
'kprobe/spin_unlock' and some
others kernel symbols like below.

SEC("kprobe/spin_unlock")PROG(p1)
SEC("kprobe/spin_lock")PROG(p2)
SEC("kprobe/mutex_spin_on_owner")PROG(p3)
SEC("kprobe/rwsem_spin_on_owner")PROG(p4)
SEC("kprobe/spin_unlock_irqrestore")PROG(p5)
SEC("kprobe/_raw_spin_unlock_irqrestore")PROG(p6)
SEC("kprobe/_raw_spin_unlock_bh")PROG(p7)
SEC("kprobe/_raw_spin_unlock")PROG(p8)
SEC("kprobe/_raw_spin_lock_irqsave")PROG(p9)
....

But the thing is, 'spin_lock' kernel symbol won't exist, as it has
been __always_inline'd
at commit 349056 at Jul,13,2015.

https://cregit.linuxsources.org/code/4.20/include/linux/spinlock.h.html#327

$ uname -r
5.0.0+
$ grep "spin_lock" /proc/kallsyms
0000000000000000 T queued_spin_lock_slowpath
0000000000000000 t process_spin_lock
0000000000000000 T bpf_spin_lock
0000000000000000 T btf_find_spin_lock
0000000000000000 T _raw_spin_lock_bh
0000000000000000 T _raw_spin_lock_irqsave
0000000000000000 T _raw_spin_lock
0000000000000000 T _raw_spin_lock_irq
...

I'm pretty sure there is something I don't know due to my lack of skills.
Is there any way to run this sample without having such kernel symbol?

On Fri, Mar 29, 2019 at 1:30 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 03/27/2019 03:45 PM, Daniel T. Lee wrote:
> > Since, ksym_search added with verification logic for symbols existence,
> > it could return NULL when the kernel symbols are not loaded.
> >
> > This commit will add NULL check logic after ksym_search.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> > Changes in v2:
> >     - Added NULL check for selftests/bpf/prog_tests/get_stack_raw_tp.c
> >
> >  samples/bpf/offwaketime_user.c                            | 5 +++++
> >  samples/bpf/sampleip_user.c                               | 5 +++++
> >  samples/bpf/trace_event_user.c                            | 5 +++++
> >  tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c | 4 ++--
> >  4 files changed, 17 insertions(+), 2 deletions(-)
>
> Still not all occurrences from samples covered:
>
> $ git grep -n ksym_search samples/bpf/
> samples/bpf/offwaketime_user.c:30:      sym = ksym_search(addr);
> samples/bpf/sampleip_user.c:111:                        sym = ksym_search(counts[i].ip);
> samples/bpf/spintest_user.c:39:                 sym = ksym_search(value);
> samples/bpf/trace_event_user.c:36:      sym = ksym_search(addr);

Re: [PATCH v2 2/2] samples, selftests/bpf: add NULL check for ksym_search

[Daniel T. Lee]

ping?

On Fri, Mar 29, 2019 at 11:54 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> I'm aware that I didn't cover the "spintest_user.c".
> Because, spintest sample isn't running currently since there is no
> such symbol called 'spin_lock'.
>
> At "spintest_kern.c", it has 'kprobe/spin_lock' and
> 'kprobe/spin_unlock' and some
> others kernel symbols like below.
>
> SEC("kprobe/spin_unlock")PROG(p1)
> SEC("kprobe/spin_lock")PROG(p2)
> SEC("kprobe/mutex_spin_on_owner")PROG(p3)
> SEC("kprobe/rwsem_spin_on_owner")PROG(p4)
> SEC("kprobe/spin_unlock_irqrestore")PROG(p5)
> SEC("kprobe/_raw_spin_unlock_irqrestore")PROG(p6)
> SEC("kprobe/_raw_spin_unlock_bh")PROG(p7)
> SEC("kprobe/_raw_spin_unlock")PROG(p8)
> SEC("kprobe/_raw_spin_lock_irqsave")PROG(p9)
> ....
>
> But the thing is, 'spin_lock' kernel symbol won't exist, as it has
> been __always_inline'd
> at commit 349056 at Jul,13,2015.
>
> https://cregit.linuxsources.org/code/4.20/include/linux/spinlock.h.html#327
>
> $ uname -r
> 5.0.0+
> $ grep "spin_lock" /proc/kallsyms
> 0000000000000000 T queued_spin_lock_slowpath
> 0000000000000000 t process_spin_lock
> 0000000000000000 T bpf_spin_lock
> 0000000000000000 T btf_find_spin_lock
> 0000000000000000 T _raw_spin_lock_bh
> 0000000000000000 T _raw_spin_lock_irqsave
> 0000000000000000 T _raw_spin_lock
> 0000000000000000 T _raw_spin_lock_irq
> ...
>
> I'm pretty sure there is something I don't know due to my lack of skills.
> Is there any way to run this sample without having such kernel symbol?
>
> On Fri, Mar 29, 2019 at 1:30 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 03/27/2019 03:45 PM, Daniel T. Lee wrote:
> > > Since, ksym_search added with verification logic for symbols existence,
> > > it could return NULL when the kernel symbols are not loaded.
> > >
> > > This commit will add NULL check logic after ksym_search.
> > >
> > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > ---
> > > Changes in v2:
> > >     - Added NULL check for selftests/bpf/prog_tests/get_stack_raw_tp.c
> > >
> > >  samples/bpf/offwaketime_user.c                            | 5 +++++
> > >  samples/bpf/sampleip_user.c                               | 5 +++++
> > >  samples/bpf/trace_event_user.c                            | 5 +++++
> > >  tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c | 4 ++--
> > >  4 files changed, 17 insertions(+), 2 deletions(-)
> >
> > Still not all occurrences from samples covered:
> >
> > $ git grep -n ksym_search samples/bpf/
> > samples/bpf/offwaketime_user.c:30:      sym = ksym_search(addr);
> > samples/bpf/sampleip_user.c:111:                        sym = ksym_search(counts[i].ip);
> > samples/bpf/spintest_user.c:39:                 sym = ksym_search(value);
> > samples/bpf/trace_event_user.c:36:      sym = ksym_search(addr);