[PATCH bpf-next 1/1]

[PATCH bpf-next 1/1]

[David CARLIER]

[-- Attachment #1.1: Type: text/plain, Size: 38 bytes --]

libbpf.c memory leaks fixes proposal.

[-- Attachment #1.2: Type: text/html, Size: 63 bytes --]

[-- Attachment #2: 0001-libbpf.c-bpf_program__attach_uprobe_opts-fix-possibl.patch --]
[-- Type: text/x-patch, Size: 1918 bytes --]

From 381ae513671ea214f0f6a04ca9da75dfe4411683 Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen@gmail.com>
Date: Sat, 18 Jan 2025 10:30:39 +0000
Subject: [PATCH] libbpf.c: bpf_program__attach_uprobe_opts fix possible memory
 leaks.

bpf_program__attach_perf_event_opts() might be not enough to close
the file descriptor, bpt_link__destroy() does a more thorough
clean up including its inner file descriptor.
Applying to
bpf_program__attach_kprobe_opts/bpf_program__attach_tracepoints_opts
too.

Signed-off-by: David Carlier <devnexen@gmail.com>
---
 src/libbpf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/libbpf.c b/src/libbpf.c
index 194809d..e571675 100644
--- a/src/libbpf.c
+++ b/src/libbpf.c
@@ -11271,7 +11271,7 @@ bpf_program__attach_kprobe_opts(const struct bpf_program *prog,
 	link = bpf_program__attach_perf_event_opts(prog, pfd, &pe_opts);
 	err = libbpf_get_error(link);
 	if (err) {
-		close(pfd);
+		bpf_link__destroy(link);
 		pr_warn("prog '%s': failed to attach to %s '%s+0x%zx': %s\n",
 			prog->name, retprobe ? "kretprobe" : "kprobe",
 			func_name, offset,
@@ -12259,7 +12259,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
 	link = bpf_program__attach_perf_event_opts(prog, pfd, &pe_opts);
 	err = libbpf_get_error(link);
 	if (err) {
-		close(pfd);
+		bpf_link__destroy(link);
 		pr_warn("prog '%s': failed to attach to %s '%s:0x%zx': %s\n",
 			prog->name, retprobe ? "uretprobe" : "uprobe",
 			binary_path, func_offset,
@@ -12514,7 +12514,7 @@ struct bpf_link *bpf_program__attach_tracepoint_opts(const struct bpf_program *p
 	link = bpf_program__attach_perf_event_opts(prog, pfd, &pe_opts);
 	err = libbpf_get_error(link);
 	if (err) {
-		close(pfd);
+		bpf_link__destroy(link);
 		pr_warn("prog '%s': failed to attach to tracepoint '%s/%s': %s\n",
 			prog->name, tp_category, tp_name,
 			errstr(err));
-- 
2.47.2

Re: [PATCH bpf-next 1/1]

[Ihor Solodrai]

Hi David,

Thanks for the patch.

Please take a look at "submitting patches" page on kernel docs and resubmit.
https://docs.kernel.org/process/submitting-patches.html


On Tuesday, January 21st, 2025 at 8:50 AM, David CARLIER <devnexen@gmail.com> wrote:

> libbpf.c memory leaks fixes proposal.

Re: [PATCH bpf-next 1/1]

[Eduard Zingerman]

On Tue, 2025-01-21 at 16:50 +0000, David CARLIER wrote:
> libbpf.c memory leaks fixes proposal.

Hi David,

please take a look at the documentation regarding sending kernel patches:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
In particular:
- the email should be in plain text
- subject should be present
- the patch itself is a part of the email, not an attachment.

About the change itself, why do you think there is a resource leak?
Here is a fragment of bpf_program__attach_kprobe_opts:

	link = bpf_program__attach_perf_event_opts(prog, pfd, &pe_opts);
	err = libbpf_get_error(link);
	if (err) {
-		close(pfd);
+		bpf_link__destroy(link);
		pr_warn("prog '%s': failed to attach to %s '%s+0x%zx': %s\n",
			prog->name, retprobe ? "kretprobe" : "kprobe",
			func_name, offset,
			errstr(err));
		goto err_clean_legacy;
	}

When libbpf_get_error returns a non-zero value the `link`
is either an error value or null, so bpf_link__destroy
has nothing to work with.

Re: [PATCH bpf-next 1/1]

[David CARLIER]

Hi sorry for not responding earlier I realised by myself going through
the reading submission process and rereading my patch, it was not
correct/useful.

Cheers.


On Fri, 24 Jan 2025 at 02:17, Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2025-01-21 at 16:50 +0000, David CARLIER wrote:
> > libbpf.c memory leaks fixes proposal.
>
> Hi David,
>
> please take a look at the documentation regarding sending kernel patches:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> In particular:
> - the email should be in plain text
> - subject should be present
> - the patch itself is a part of the email, not an attachment.
>
> About the change itself, why do you think there is a resource leak?
> Here is a fragment of bpf_program__attach_kprobe_opts:
>
>         link = bpf_program__attach_perf_event_opts(prog, pfd, &pe_opts);
>         err = libbpf_get_error(link);
>         if (err) {
> -               close(pfd);
> +               bpf_link__destroy(link);
>                 pr_warn("prog '%s': failed to attach to %s '%s+0x%zx': %s\n",
>                         prog->name, retprobe ? "kretprobe" : "kprobe",
>                         func_name, offset,
>                         errstr(err));
>                 goto err_clean_legacy;
>         }
>
> When libbpf_get_error returns a non-zero value the `link`
> is either an error value or null, so bpf_link__destroy
> has nothing to work with.