* [PATCH] bpf: selftests: global_funcs: check err_str before strstr
@ 2020-08-19 2:34 Yauheni Kaliuta
2020-08-19 5:19 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Yauheni Kaliuta @ 2020-08-19 2:34 UTC (permalink / raw)
To: bpf; +Cc: Daniel Borkmann, Alexei Starovoitov, Song Liu
The error path in libbpf.c:load_program() has calls to pr_warn()
which ends up for global_funcs tests to
test_global_funcs.c:libbpf_debug_print().
For the tests with no struct test_def::err_str initialized with a
string, it causes call of strstr() with NULL as the second argument
and it segfaults.
Fix it by calling strstr() only for non-NULL err_str.
The patch does not fix the test itself.
Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
tools/testing/selftests/bpf/prog_tests/test_global_funcs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
index 25b068591e9a..6ad14c5465eb 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
@@ -19,7 +19,7 @@ static int libbpf_debug_print(enum libbpf_print_level level,
log_buf = va_arg(args, char *);
if (!log_buf)
goto out;
- if (strstr(log_buf, err_str) == 0)
+ if ((err_str != NULL) && (strstr(log_buf, err_str) == 0))
found = true;
out:
printf(format, log_buf);
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: selftests: global_funcs: check err_str before strstr
2020-08-19 2:34 [PATCH] bpf: selftests: global_funcs: check err_str before strstr Yauheni Kaliuta
@ 2020-08-19 5:19 ` Yonghong Song
2020-08-19 7:05 ` Yauheni Kaliuta
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2020-08-19 5:19 UTC (permalink / raw)
To: Yauheni Kaliuta, bpf; +Cc: Daniel Borkmann, Alexei Starovoitov, Song Liu
On 8/18/20 7:34 PM, Yauheni Kaliuta wrote:
> The error path in libbpf.c:load_program() has calls to pr_warn()
> which ends up for global_funcs tests to
> test_global_funcs.c:libbpf_debug_print().
>
> For the tests with no struct test_def::err_str initialized with a
> string, it causes call of strstr() with NULL as the second argument
> and it segfaults.
>
> Fix it by calling strstr() only for non-NULL err_str.
>
> The patch does not fix the test itself.
So this happens in older kernel, right? Could you clarify more
in which kernel and what environment? It probably no need to
fix the issue for really old kernel but some clarification
will be good.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
> tools/testing/selftests/bpf/prog_tests/test_global_funcs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> index 25b068591e9a..6ad14c5465eb 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> @@ -19,7 +19,7 @@ static int libbpf_debug_print(enum libbpf_print_level level,
> log_buf = va_arg(args, char *);
> if (!log_buf)
> goto out;
> - if (strstr(log_buf, err_str) == 0)
> + if ((err_str != NULL) && (strstr(log_buf, err_str) == 0))
Looks good but the code can be simplified as
if (err_str && strstr(log_buf, err_str) == 0)
found = true;
> found = true;
> out:
> printf(format, log_buf);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: selftests: global_funcs: check err_str before strstr
2020-08-19 5:19 ` Yonghong Song
@ 2020-08-19 7:05 ` Yauheni Kaliuta
2020-08-19 13:36 ` Yauheni Kaliuta
2020-08-19 14:56 ` Yonghong Song
0 siblings, 2 replies; 6+ messages in thread
From: Yauheni Kaliuta @ 2020-08-19 7:05 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, Daniel Borkmann, Alexei Starovoitov, Song Liu
On Wed, Aug 19, 2020 at 8:19 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 8/18/20 7:34 PM, Yauheni Kaliuta wrote:
> > The error path in libbpf.c:load_program() has calls to pr_warn()
> > which ends up for global_funcs tests to
> > test_global_funcs.c:libbpf_debug_print().
> >
> > For the tests with no struct test_def::err_str initialized with a
> > string, it causes call of strstr() with NULL as the second argument
> > and it segfaults.
> >
> > Fix it by calling strstr() only for non-NULL err_str.
> >
> > The patch does not fix the test itself.
>
> So this happens in older kernel, right? Could you clarify more
> in which kernel and what environment? It probably no need to
> fix the issue for really old kernel but some clarification
> will be good.
I'll test it with the very recent kernel on that architecture soon,
for sure. But it's not related to the patch.
>
> >
> > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> > ---
> > tools/testing/selftests/bpf/prog_tests/test_global_funcs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> > index 25b068591e9a..6ad14c5465eb 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> > @@ -19,7 +19,7 @@ static int libbpf_debug_print(enum libbpf_print_level level,
> > log_buf = va_arg(args, char *);
> > if (!log_buf)
> > goto out;
> > - if (strstr(log_buf, err_str) == 0)
> > + if ((err_str != NULL) && (strstr(log_buf, err_str) == 0))
>
> Looks good but the code can be simplified as
> if (err_str && strstr(log_buf, err_str) == 0)
> found = true;
Yes, but I prefer to use NULL explicitly when I deal with pointers. It
demonstrates intention better. You also can simplify strstr() == 0
with !. Actually, strstr() returns char *, so comparation to 0
(totally legal by standard) breaks my feelings too :).
If you insist, I'll send v2 of course.
> > found = true;
> > out:
> > printf(format, log_buf);
> >
>
--
WBR, Yauheni
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: selftests: global_funcs: check err_str before strstr
2020-08-19 7:05 ` Yauheni Kaliuta
@ 2020-08-19 13:36 ` Yauheni Kaliuta
2020-08-19 14:56 ` Yonghong Song
1 sibling, 0 replies; 6+ messages in thread
From: Yauheni Kaliuta @ 2020-08-19 13:36 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, Daniel Borkmann, Alexei Starovoitov, Song Liu
On Wed, Aug 19, 2020 at 10:05 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> On Wed, Aug 19, 2020 at 8:19 AM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 8/18/20 7:34 PM, Yauheni Kaliuta wrote:
> > > The error path in libbpf.c:load_program() has calls to pr_warn()
> > > which ends up for global_funcs tests to
> > > test_global_funcs.c:libbpf_debug_print().
> > >
> > > For the tests with no struct test_def::err_str initialized with a
> > > string, it causes call of strstr() with NULL as the second argument
> > > and it segfaults.
> > >
> > > Fix it by calling strstr() only for non-NULL err_str.
> > >
> > > The patch does not fix the test itself.
> >
> > So this happens in older kernel, right? Could you clarify more
> > in which kernel and what environment? It probably no need to
> > fix the issue for really old kernel but some clarification
> > will be good.
>
> I'll test it with the very recent kernel on that architecture soon,
> for sure. But it's not related to the patch.
./test_progs -t global_func still fails for me on s390 with
18445bf405cb331117bc98427b1ba6f12418ad17
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: selftests: global_funcs: check err_str before strstr
2020-08-19 7:05 ` Yauheni Kaliuta
2020-08-19 13:36 ` Yauheni Kaliuta
@ 2020-08-19 14:56 ` Yonghong Song
2020-08-19 20:46 ` Yauheni Kaliuta
1 sibling, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2020-08-19 14:56 UTC (permalink / raw)
To: Yauheni Kaliuta; +Cc: bpf, Daniel Borkmann, Alexei Starovoitov, Song Liu
On 8/19/20 12:05 AM, Yauheni Kaliuta wrote:
> On Wed, Aug 19, 2020 at 8:19 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 8/18/20 7:34 PM, Yauheni Kaliuta wrote:
>>> The error path in libbpf.c:load_program() has calls to pr_warn()
>>> which ends up for global_funcs tests to
>>> test_global_funcs.c:libbpf_debug_print().
>>>
>>> For the tests with no struct test_def::err_str initialized with a
>>> string, it causes call of strstr() with NULL as the second argument
>>> and it segfaults.
>>>
>>> Fix it by calling strstr() only for non-NULL err_str.
>>>
>>> The patch does not fix the test itself.
>>
>> So this happens in older kernel, right? Could you clarify more
>> in which kernel and what environment? It probably no need to
>> fix the issue for really old kernel but some clarification
>> will be good.
>
> I'll test it with the very recent kernel on that architecture soon,
> for sure. But it's not related to the patch.
The above "The patch does not fix the test itself" a little bit vague.
You can say that "The test may fail in old kernels where <why it fails
...> and this patch is to fix the segfault rather the test failure.".
This way people can easily understand why and the purpose of this patch.
>
>>
>>>
>>> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Ok, ack with the above nit and one nit below.
Acked-by: Yonghong Song <yhs@fb.com>
I guess it is better to send a v2 carrying my ack.
>>> ---
>>> tools/testing/selftests/bpf/prog_tests/test_global_funcs.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
>>> index 25b068591e9a..6ad14c5465eb 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
>>> @@ -19,7 +19,7 @@ static int libbpf_debug_print(enum libbpf_print_level level,
>>> log_buf = va_arg(args, char *);
>>> if (!log_buf)
>>> goto out;
>>> - if (strstr(log_buf, err_str) == 0)
>>> + if ((err_str != NULL) && (strstr(log_buf, err_str) == 0))
>>
>> Looks good but the code can be simplified as
>> if (err_str && strstr(log_buf, err_str) == 0)
>> found = true;
>
> Yes, but I prefer to use NULL explicitly when I deal with pointers. It
> demonstrates intention better. You also can simplify strstr() == 0
> with !. Actually, strstr() returns char *, so comparation to 0
> (totally legal by standard) breaks my feelings too :).
comparison with NULL is okay. You can just do
(err_str != NULL && strstr(log_buf, err_str) == 0)
there is no need for extra parenthesis.
>
> If you insist, I'll send v2 of course.
>
>>> found = true;
>>> out:
>>> printf(format, log_buf);
>>>
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: selftests: global_funcs: check err_str before strstr
2020-08-19 14:56 ` Yonghong Song
@ 2020-08-19 20:46 ` Yauheni Kaliuta
0 siblings, 0 replies; 6+ messages in thread
From: Yauheni Kaliuta @ 2020-08-19 20:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, Daniel Borkmann, Alexei Starovoitov, Song Liu
On Wed, Aug 19, 2020 at 5:57 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 8/19/20 12:05 AM, Yauheni Kaliuta wrote:
> > On Wed, Aug 19, 2020 at 8:19 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 8/18/20 7:34 PM, Yauheni Kaliuta wrote:
> >>> The error path in libbpf.c:load_program() has calls to pr_warn()
> >>> which ends up for global_funcs tests to
> >>> test_global_funcs.c:libbpf_debug_print().
> >>>
> >>> For the tests with no struct test_def::err_str initialized with a
> >>> string, it causes call of strstr() with NULL as the second argument
> >>> and it segfaults.
> >>>
> >>> Fix it by calling strstr() only for non-NULL err_str.
> >>>
> >>> The patch does not fix the test itself.
> >>
> >> So this happens in older kernel, right? Could you clarify more
> >> in which kernel and what environment? It probably no need to
> >> fix the issue for really old kernel but some clarification
> >> will be good.
> >
> > I'll test it with the very recent kernel on that architecture soon,
> > for sure. But it's not related to the patch.
>
> The above "The patch does not fix the test itself" a little bit vague.
> You can say that "The test may fail in old kernels where <why it fails
> ...> and this patch is to fix the segfault rather the test failure.".
> This way people can easily understand why and the purpose of this patch.
Ok, I'll remove it completely (as I mentioned in the follow-up email,
the test still fails for me for the Linus' HEAD).
>
> >
> >>
> >>>
> >>> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
>
> Ok, ack with the above nit and one nit below.
> Acked-by: Yonghong Song <yhs@fb.com>
> I guess it is better to send a v2 carrying my ack.
>
> >>> ---
> >>> tools/testing/selftests/bpf/prog_tests/test_global_funcs.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> >>> index 25b068591e9a..6ad14c5465eb 100644
> >>> --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> >>> +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
> >>> @@ -19,7 +19,7 @@ static int libbpf_debug_print(enum libbpf_print_level level,
> >>> log_buf = va_arg(args, char *);
> >>> if (!log_buf)
> >>> goto out;
> >>> - if (strstr(log_buf, err_str) == 0)
> >>> + if ((err_str != NULL) && (strstr(log_buf, err_str) == 0))
> >>
> >> Looks good but the code can be simplified as
> >> if (err_str && strstr(log_buf, err_str) == 0)
> >> found = true;
> >
> > Yes, but I prefer to use NULL explicitly when I deal with pointers. It
> > demonstrates intention better. You also can simplify strstr() == 0
> > with !. Actually, strstr() returns char *, so comparation to 0
> > (totally legal by standard) breaks my feelings too :).
>
> comparison with NULL is okay. You can just do
> (err_str != NULL && strstr(log_buf, err_str) == 0)
> there is no need for extra parenthesis.
Ah, ok. Inconsistency with the strstr check bothers me, but it would
be unrelated change.
Thank you for review!
>
> >
> > If you insist, I'll send v2 of course.
> >
> >>> found = true;
> >>> out:
> >>> printf(format, log_buf);
> >>>
> >>
> >
> >
>
--
WBR, Yauheni
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-19 20:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-19 2:34 [PATCH] bpf: selftests: global_funcs: check err_str before strstr Yauheni Kaliuta
2020-08-19 5:19 ` Yonghong Song
2020-08-19 7:05 ` Yauheni Kaliuta
2020-08-19 13:36 ` Yauheni Kaliuta
2020-08-19 14:56 ` Yonghong Song
2020-08-19 20:46 ` Yauheni Kaliuta
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.