* [PATCH] selftests/bpf:fix a resource leak in main()
@ 2024-07-11 7:10 Ma Ke
2024-07-12 0:53 ` Stanislav Fomichev
2024-07-12 8:05 ` Markus Elfring
0 siblings, 2 replies; 6+ messages in thread
From: Ma Ke @ 2024-07-11 7:10 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, mykolal, shuah,
sowmini.varadhan
Cc: bpf, linux-kselftest, linux-kernel, Ma Ke
The requested resources should be closed before return in main(), otherwise
resource leak will occur. Add a check of cg_fd before close().
Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
tools/testing/selftests/bpf/test_tcpnotify_user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
index 595194453ff8..f81c60db586e 100644
--- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
+++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
@@ -161,7 +161,8 @@ int main(int argc, char **argv)
error = 0;
err:
bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
- close(cg_fd);
+ if (cg_fd >= 0)
+ close(cg_fd);
cleanup_cgroup_environment();
perf_buffer__free(pb);
return error;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] selftests/bpf:fix a resource leak in main()
2024-07-11 7:10 [PATCH] selftests/bpf:fix a resource leak in main() Ma Ke
@ 2024-07-12 0:53 ` Stanislav Fomichev
2024-07-12 8:05 ` Markus Elfring
1 sibling, 0 replies; 6+ messages in thread
From: Stanislav Fomichev @ 2024-07-12 0:53 UTC (permalink / raw)
To: Ma Ke
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, haoluo, jolsa, mykolal, shuah,
sowmini.varadhan, bpf, linux-kselftest, linux-kernel
On 07/11, Ma Ke wrote:
> The requested resources should be closed before return in main(), otherwise
> resource leak will occur. Add a check of cg_fd before close().
>
> Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
> tools/testing/selftests/bpf/test_tcpnotify_user.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> index 595194453ff8..f81c60db586e 100644
> --- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
> +++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> @@ -161,7 +161,8 @@ int main(int argc, char **argv)
> error = 0;
> err:
> bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
> - close(cg_fd);
Worst case that happens here is that we call close(-1) which returns
EBADFS or something similar. Don't think there is any problem. This
applies to the rest of that patches that add an extra 'if' check.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] selftests/bpf:fix a resource leak in main()
2024-07-11 7:10 [PATCH] selftests/bpf:fix a resource leak in main() Ma Ke
2024-07-12 0:53 ` Stanislav Fomichev
@ 2024-07-12 8:05 ` Markus Elfring
2024-07-13 0:59 ` Stanislav Fomichev
1 sibling, 1 reply; 6+ messages in thread
From: Markus Elfring @ 2024-07-12 8:05 UTC (permalink / raw)
To: make24, bpf, linux-kselftest, kernel-janitors
Cc: LKML, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Eduard Zingerman, Hao Luo, Jiri Olsa, John Fastabend, KP Singh,
Martin KaFai Lau, Mykola Lysenko, Shuah Khan, Stanislav Fomichev,
Song Liu, Sowmini Varadhan, Yonghong Song
> The requested resources should be closed before return in main(), otherwise
> resource leak will occur. Add a check of cg_fd before close().
>
> Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
Please reconsider such information once more.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n398
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v6.10-rc7#n5
How many source code analysis tools should be able to point out that the return value
from the call of a function like pthread_create() should get more development attention
(also for discussed test functions)?
https://elixir.bootlin.com/linux/v6.10-rc7/source/tools/testing/selftests/bpf/test_tcpnotify_user.c#L122
See also:
* https://cwe.mitre.org/data/definitions/252.html
* https://wiki.sei.cmu.edu/confluence/display/c/POS54-C.+Detect+and+handle+POSIX+library+errors
Regards,
Markus
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] selftests/bpf:fix a resource leak in main()
2024-07-12 8:05 ` Markus Elfring
@ 2024-07-13 0:59 ` Stanislav Fomichev
0 siblings, 0 replies; 6+ messages in thread
From: Stanislav Fomichev @ 2024-07-13 0:59 UTC (permalink / raw)
To: Markus Elfring
Cc: make24, bpf, linux-kselftest, kernel-janitors, LKML,
Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Eduard Zingerman, Hao Luo, Jiri Olsa, John Fastabend, KP Singh,
Martin KaFai Lau, Mykola Lysenko, Shuah Khan, Song Liu,
Sowmini Varadhan, Yonghong Song
On 07/12, Markus Elfring wrote:
> > The requested resources should be closed before return in main(), otherwise
> > resource leak will occur. Add a check of cg_fd before close().
> >
> > Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
>
> Please reconsider such information once more.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n398
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v6.10-rc7#n5
>
>
> How many source code analysis tools should be able to point out that the return value
> from the call of a function like pthread_create() should get more development attention
> (also for discussed test functions)?
> https://elixir.bootlin.com/linux/v6.10-rc7/source/tools/testing/selftests/bpf/test_tcpnotify_user.c#L122
>
> See also:
> * https://cwe.mitre.org/data/definitions/252.html
>
> * https://wiki.sei.cmu.edu/confluence/display/c/POS54-C.+Detect+and+handle+POSIX+library+errors
We are talking about testing binaries here. We don't have infinite
amount of time to polish them. If you really want to help, look at
the flakes on the bpf dashboard and help us weed them out.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] selftests/bpf:fix a resource leak in main()
@ 2024-07-11 7:18 Ma Ke
0 siblings, 0 replies; 6+ messages in thread
From: Ma Ke @ 2024-07-11 7:18 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, mykolal, shuah, rdna
Cc: bpf, linux-kselftest, linux-kernel, Ma Ke
The requested resources should be closed before return in main(), otherwise
resource leak will occur. Add a check of cg_fd before close().
Fixes: 1f5fa9ab6e2e ("selftests/bpf: Test BPF_CGROUP_SYSCTL")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
tools/testing/selftests/bpf/test_sysctl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
index bcdbd27f22f0..242246ba8d1e 100644
--- a/tools/testing/selftests/bpf/test_sysctl.c
+++ b/tools/testing/selftests/bpf/test_sysctl.c
@@ -1627,7 +1627,8 @@ int main(int argc, char **argv)
err:
err = -1;
out:
- close(cgfd);
+ if (cgfd >= 0)
+ close(cgfd);
cleanup_cgroup_environment();
return err;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] selftests/bpf:fix a resource leak in main()
@ 2024-07-11 7:34 Ma Ke
0 siblings, 0 replies; 6+ messages in thread
From: Ma Ke @ 2024-07-11 7:34 UTC (permalink / raw)
To: andrii, eddyz87, mykolal, ast, daniel, martin.lau, song,
yonghong.song, john.fastabend, kpsingh, sdf, haoluo, jolsa, shuah,
rdna
Cc: bpf, linux-kselftest, linux-kernel, Ma Ke
The requested resources should be closed before return in main(), otherwise
resource leak will occur. Add a check of cgfd before close().
Fixes: 5ecd8c22739b ("selftests/bpf: Selftest for bpf_skb_ancestor_cgroup_id")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
tools/testing/selftests/bpf/test_skb_cgroup_id_user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c b/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
index ed518d075d1d..f84faddd72e6 100644
--- a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
+++ b/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
@@ -176,7 +176,8 @@ int main(int argc, char **argv)
err:
err = -1;
out:
- close(cgfd);
+ if (cgfd >= 0)
+ close(cgfd);
cleanup_cgroup_environment();
printf("[%s]\n", err ? "FAIL" : "PASS");
return err;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-13 0:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-11 7:10 [PATCH] selftests/bpf:fix a resource leak in main() Ma Ke
2024-07-12 0:53 ` Stanislav Fomichev
2024-07-12 8:05 ` Markus Elfring
2024-07-13 0:59 ` Stanislav Fomichev
-- strict thread matches above, loose matches on Subject: below --
2024-07-11 7:18 Ma Ke
2024-07-11 7:34 Ma Ke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox