* [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
@ 2020-02-20 23:05 Andrii Nakryiko
2020-02-21 2:04 ` Alexei Starovoitov
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2020-02-20 23:05 UTC (permalink / raw)
To: bpf, netdev, ast, daniel
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Jiri Olsa
Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
clean up is performed correctly.
Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
index 1f6ccdaed1ac..781c8d11604b 100644
--- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
+++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
@@ -55,31 +55,40 @@ void test_trampoline_count(void)
/* attach 'allowed' 40 trampoline programs */
for (i = 0; i < MAX_TRAMP_PROGS; i++) {
obj = bpf_object__open_file(object, NULL);
- if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
+ if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
+ obj = NULL;
goto cleanup;
+ }
err = bpf_object__load(obj);
if (CHECK(err, "obj_load", "err %d\n", err))
goto cleanup;
inst[i].obj = obj;
+ obj = NULL;
if (rand() % 2) {
- link = load(obj, fentry_name);
- if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
+ link = load(inst[i].obj, fentry_name);
+ if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
+ link = NULL;
goto cleanup;
+ }
inst[i].link_fentry = link;
} else {
- link = load(obj, fexit_name);
- if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
+ link = load(inst[i].obj, fexit_name);
+ if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
+ link = NULL;
goto cleanup;
+ }
inst[i].link_fexit = link;
}
}
/* and try 1 extra.. */
obj = bpf_object__open_file(object, NULL);
- if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
+ if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
+ obj = NULL;
goto cleanup;
+ }
err = bpf_object__load(obj);
if (CHECK(err, "obj_load", "err %d\n", err))
@@ -104,7 +113,9 @@ void test_trampoline_count(void)
cleanup_extra:
bpf_object__close(obj);
cleanup:
- while (--i) {
+ if (i >= MAX_TRAMP_PROGS)
+ i = MAX_TRAMP_PROGS - 1;
+ for (; i >= 0; i--) {
bpf_link__destroy(inst[i].link_fentry);
bpf_link__destroy(inst[i].link_fexit);
bpf_object__close(inst[i].obj);
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-20 23:05 [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic Andrii Nakryiko
@ 2020-02-21 2:04 ` Alexei Starovoitov
2020-02-21 2:06 ` Song Liu
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Alexei Starovoitov @ 2020-02-21 2:04 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team, Jiri Olsa
On Thu, Feb 20, 2020 at 03:05:46PM -0800, Andrii Nakryiko wrote:
> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
> clean up is performed correctly.
>
> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Applied, Thanks
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-20 23:05 [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic Andrii Nakryiko
2020-02-21 2:04 ` Alexei Starovoitov
@ 2020-02-21 2:06 ` Song Liu
2020-02-21 4:20 ` Andrii Nakryiko
2020-02-21 10:21 ` Jakub Sitnicki
2020-02-21 13:04 ` Jiri Olsa
3 siblings, 1 reply; 9+ messages in thread
From: Song Liu @ 2020-02-21 2:06 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Kernel Team, Jiri Olsa
On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
> clean up is performed correctly.
>
> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> index 1f6ccdaed1ac..781c8d11604b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> @@ -55,31 +55,40 @@ void test_trampoline_count(void)
> /* attach 'allowed' 40 trampoline programs */
> for (i = 0; i < MAX_TRAMP_PROGS; i++) {
> obj = bpf_object__open_file(object, NULL);
> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> + obj = NULL;
I think we don't need obj and link in cleanup? Did I miss anything?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-21 2:06 ` Song Liu
@ 2020-02-21 4:20 ` Andrii Nakryiko
2020-02-21 4:31 ` Song Liu
0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2020-02-21 4:20 UTC (permalink / raw)
To: Song Liu
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Kernel Team, Jiri Olsa
On 2/20/20 6:06 PM, Song Liu wrote:
> On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote:
>>
>> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
>> clean up is performed correctly.
>>
>> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>> ---
>> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
>> index 1f6ccdaed1ac..781c8d11604b 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
>> @@ -55,31 +55,40 @@ void test_trampoline_count(void)
>> /* attach 'allowed' 40 trampoline programs */
>> for (i = 0; i < MAX_TRAMP_PROGS; i++) {
>> obj = bpf_object__open_file(object, NULL);
>> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
>> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
>> + obj = NULL;
>
> I think we don't need obj and link in cleanup? Did I miss anything?
>
We do set obj below (line 87) after this loop, so need to clean it up as
well. As for link, yeah, technically link doesn't have to be set to
NULL, but I kind of did it for completeness without thinking too much.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-21 4:20 ` Andrii Nakryiko
@ 2020-02-21 4:31 ` Song Liu
2020-02-21 4:45 ` Andrii Nakryiko
0 siblings, 1 reply; 9+ messages in thread
From: Song Liu @ 2020-02-21 4:31 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Song Liu, bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Kernel Team, Jiri Olsa
> On Feb 20, 2020, at 8:20 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>
> On 2/20/20 6:06 PM, Song Liu wrote:
>> On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote:
>>>
>>> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
>>> clean up is performed correctly.
>>>
>>> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>> ---
>>> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
>>> 1 file changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
>>> index 1f6ccdaed1ac..781c8d11604b 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
>>> @@ -55,31 +55,40 @@ void test_trampoline_count(void)
>>> /* attach 'allowed' 40 trampoline programs */
>>> for (i = 0; i < MAX_TRAMP_PROGS; i++) {
>>> obj = bpf_object__open_file(object, NULL);
>>> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
>>> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
>>> + obj = NULL;
>> I think we don't need obj and link in cleanup? Did I miss anything?
>
> We do set obj below (line 87) after this loop, so need to clean it up as well. As for link, yeah, technically link doesn't have to be set to NULL, but I kind of did it for completeness without thinking too much.
I meant "obj = NULL;" before "goto cleanup;", as we don't use obj in the
cleanup path.
Anyway, this is not a real issue.
Thanks,
Song
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-21 4:31 ` Song Liu
@ 2020-02-21 4:45 ` Andrii Nakryiko
0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2020-02-21 4:45 UTC (permalink / raw)
To: Song Liu
Cc: Andrii Nakryiko, Song Liu, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team, Jiri Olsa
On Thu, Feb 20, 2020 at 8:31 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Feb 20, 2020, at 8:20 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > On 2/20/20 6:06 PM, Song Liu wrote:
> >> On Thu, Feb 20, 2020 at 3:07 PM Andrii Nakryiko <andriin@fb.com> wrote:
> >>>
> >>> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
> >>> clean up is performed correctly.
> >>>
> >>> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
> >>> Cc: Jiri Olsa <jolsa@kernel.org>
> >>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >>> ---
> >>> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
> >>> 1 file changed, 18 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> >>> index 1f6ccdaed1ac..781c8d11604b 100644
> >>> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> >>> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> >>> @@ -55,31 +55,40 @@ void test_trampoline_count(void)
> >>> /* attach 'allowed' 40 trampoline programs */
> >>> for (i = 0; i < MAX_TRAMP_PROGS; i++) {
> >>> obj = bpf_object__open_file(object, NULL);
> >>> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> >>> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> >>> + obj = NULL;
> >> I think we don't need obj and link in cleanup? Did I miss anything?
> >
> > We do set obj below (line 87) after this loop, so need to clean it up as well. As for link, yeah, technically link doesn't have to be set to NULL, but I kind of did it for completeness without thinking too much.
>
> I meant "obj = NULL;" before "goto cleanup;", as we don't use obj in the
> cleanup path.
>
> Anyway, this is not a real issue.
Ah, I see what you are saying, we skip over that bpf_object__close()
call, right.
>
> Thanks,
> Song
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-20 23:05 [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic Andrii Nakryiko
2020-02-21 2:04 ` Alexei Starovoitov
2020-02-21 2:06 ` Song Liu
@ 2020-02-21 10:21 ` Jakub Sitnicki
2020-02-22 0:21 ` Andrii Nakryiko
2020-02-21 13:04 ` Jiri Olsa
3 siblings, 1 reply; 9+ messages in thread
From: Jakub Sitnicki @ 2020-02-21 10:21 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team, Jiri Olsa
On Thu, Feb 20, 2020 at 11:05 PM GMT, Andrii Nakryiko wrote:
> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
> clean up is performed correctly.
>
> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> index 1f6ccdaed1ac..781c8d11604b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> @@ -55,31 +55,40 @@ void test_trampoline_count(void)
> /* attach 'allowed' 40 trampoline programs */
> for (i = 0; i < MAX_TRAMP_PROGS; i++) {
> obj = bpf_object__open_file(object, NULL);
> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> + obj = NULL;
> goto cleanup;
> + }
>
> err = bpf_object__load(obj);
> if (CHECK(err, "obj_load", "err %d\n", err))
> goto cleanup;
> inst[i].obj = obj;
> + obj = NULL;
>
> if (rand() % 2) {
> - link = load(obj, fentry_name);
> - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
> + link = load(inst[i].obj, fentry_name);
> + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
> + link = NULL;
> goto cleanup;
> + }
> inst[i].link_fentry = link;
> } else {
> - link = load(obj, fexit_name);
> - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
> + link = load(inst[i].obj, fexit_name);
> + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
> + link = NULL;
> goto cleanup;
> + }
> inst[i].link_fexit = link;
> }
> }
>
> /* and try 1 extra.. */
> obj = bpf_object__open_file(object, NULL);
> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> + obj = NULL;
> goto cleanup;
> + }
>
> err = bpf_object__load(obj);
> if (CHECK(err, "obj_load", "err %d\n", err))
> @@ -104,7 +113,9 @@ void test_trampoline_count(void)
> cleanup_extra:
> bpf_object__close(obj);
> cleanup:
> - while (--i) {
> + if (i >= MAX_TRAMP_PROGS)
> + i = MAX_TRAMP_PROGS - 1;
> + for (; i >= 0; i--) {
> bpf_link__destroy(inst[i].link_fentry);
> bpf_link__destroy(inst[i].link_fexit);
> bpf_object__close(inst[i].obj);
I'm not sure I'm grasping what the fix is about.
We don't access obj or link from cleanup, so what is the point of
setting them to NULL before jumping there?
Or is it all just an ancillary change to clamping the loop index
variable to (MAX_TRAMP_PROGS - 1)?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-20 23:05 [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic Andrii Nakryiko
` (2 preceding siblings ...)
2020-02-21 10:21 ` Jakub Sitnicki
@ 2020-02-21 13:04 ` Jiri Olsa
3 siblings, 0 replies; 9+ messages in thread
From: Jiri Olsa @ 2020-02-21 13:04 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team, Jiri Olsa
On Thu, Feb 20, 2020 at 03:05:46PM -0800, Andrii Nakryiko wrote:
> Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
> clean up is performed correctly.
>
> Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> index 1f6ccdaed1ac..781c8d11604b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> @@ -55,31 +55,40 @@ void test_trampoline_count(void)
> /* attach 'allowed' 40 trampoline programs */
> for (i = 0; i < MAX_TRAMP_PROGS; i++) {
> obj = bpf_object__open_file(object, NULL);
> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> + obj = NULL;
> goto cleanup;
> + }
>
> err = bpf_object__load(obj);
> if (CHECK(err, "obj_load", "err %d\n", err))
> goto cleanup;
> inst[i].obj = obj;
> + obj = NULL;
>
> if (rand() % 2) {
> - link = load(obj, fentry_name);
> - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
> + link = load(inst[i].obj, fentry_name);
> + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
> + link = NULL;
> goto cleanup;
> + }
> inst[i].link_fentry = link;
> } else {
> - link = load(obj, fexit_name);
> - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
> + link = load(inst[i].obj, fexit_name);
> + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
> + link = NULL;
> goto cleanup;
> + }
> inst[i].link_fexit = link;
> }
> }
>
> /* and try 1 extra.. */
> obj = bpf_object__open_file(object, NULL);
> - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> + obj = NULL;
> goto cleanup;
> + }
>
> err = bpf_object__load(obj);
> if (CHECK(err, "obj_load", "err %d\n", err))
> @@ -104,7 +113,9 @@ void test_trampoline_count(void)
> cleanup_extra:
> bpf_object__close(obj);
> cleanup:
> - while (--i) {
> + if (i >= MAX_TRAMP_PROGS)
> + i = MAX_TRAMP_PROGS - 1;
> + for (; i >= 0; i--) {
ugh right, if we fail in first iteration, 'i' would get get -1 in here
thanks,
jirka
> bpf_link__destroy(inst[i].link_fentry);
> bpf_link__destroy(inst[i].link_fexit);
> bpf_object__close(inst[i].obj);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic
2020-02-21 10:21 ` Jakub Sitnicki
@ 2020-02-22 0:21 ` Andrii Nakryiko
0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2020-02-22 0:21 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team, Jiri Olsa
On Fri, Feb 21, 2020 at 2:21 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Thu, Feb 20, 2020 at 11:05 PM GMT, Andrii Nakryiko wrote:
> > Libbpf's Travis CI tests caught this issue. Ensure bpf_link and bpf_object
> > clean up is performed correctly.
> >
> > Fixes: d633d57902a5 ("selftest/bpf: Add test for allowed trampolines count")
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > .../bpf/prog_tests/trampoline_count.c | 25 +++++++++++++------
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> > index 1f6ccdaed1ac..781c8d11604b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
> > @@ -55,31 +55,40 @@ void test_trampoline_count(void)
> > /* attach 'allowed' 40 trampoline programs */
> > for (i = 0; i < MAX_TRAMP_PROGS; i++) {
> > obj = bpf_object__open_file(object, NULL);
> > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> > + obj = NULL;
> > goto cleanup;
> > + }
> >
> > err = bpf_object__load(obj);
> > if (CHECK(err, "obj_load", "err %d\n", err))
> > goto cleanup;
> > inst[i].obj = obj;
> > + obj = NULL;
> >
> > if (rand() % 2) {
> > - link = load(obj, fentry_name);
> > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
> > + link = load(inst[i].obj, fentry_name);
> > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
> > + link = NULL;
> > goto cleanup;
> > + }
> > inst[i].link_fentry = link;
> > } else {
> > - link = load(obj, fexit_name);
> > - if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link)))
> > + link = load(inst[i].obj, fexit_name);
> > + if (CHECK(IS_ERR(link), "attach prog", "err %ld\n", PTR_ERR(link))) {
> > + link = NULL;
> > goto cleanup;
> > + }
> > inst[i].link_fexit = link;
> > }
> > }
> >
> > /* and try 1 extra.. */
> > obj = bpf_object__open_file(object, NULL);
> > - if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
> > + if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj))) {
> > + obj = NULL;
> > goto cleanup;
> > + }
> >
> > err = bpf_object__load(obj);
> > if (CHECK(err, "obj_load", "err %d\n", err))
> > @@ -104,7 +113,9 @@ void test_trampoline_count(void)
> > cleanup_extra:
> > bpf_object__close(obj);
> > cleanup:
> > - while (--i) {
> > + if (i >= MAX_TRAMP_PROGS)
> > + i = MAX_TRAMP_PROGS - 1;
> > + for (; i >= 0; i--) {
> > bpf_link__destroy(inst[i].link_fentry);
> > bpf_link__destroy(inst[i].link_fexit);
> > bpf_object__close(inst[i].obj);
>
> I'm not sure I'm grasping what the fix is about.
>
> We don't access obj or link from cleanup, so what is the point of
> setting them to NULL before jumping there?
>
> Or is it all just an ancillary change to clamping the loop index
> variable to (MAX_TRAMP_PROGS - 1)?
Yeah, mostly ancillary. But this is not just clamping to
MAX_TRAMP_PROGS-1. As Jiri pointed out, it's also handling a case of i
== 0.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-22 0:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-20 23:05 [PATCH bpf-next] selftests/bpf: fix trampoline_count clean up logic Andrii Nakryiko
2020-02-21 2:04 ` Alexei Starovoitov
2020-02-21 2:06 ` Song Liu
2020-02-21 4:20 ` Andrii Nakryiko
2020-02-21 4:31 ` Song Liu
2020-02-21 4:45 ` Andrii Nakryiko
2020-02-21 10:21 ` Jakub Sitnicki
2020-02-22 0:21 ` Andrii Nakryiko
2020-02-21 13:04 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).