| Message ID | 20200819023427.267182-1-yauheni.kaliuta@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: selftests: global_funcs: check err_str before strstr | expand |
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); >
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); > > >
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
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); >>> >> > >
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); > >>> > >> > > > > >
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);
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(-)