Re: [PATCH] perf bpf-filter: Fix memory leak on bpf_link

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf bpf-filter: double close of perf_event fd in bpf_link__destroy()
- [Medium] perf bpf-filter: incorrect IS_ERR() check in perf_bpf_filter__prepare()
--

commit 0bafd46f68639fd52d99226001cedebcf8838c32
Author: Namhyung Kim <namhyung@kernel.org>

perf bpf-filter: Fix memory leak on bpf_link

This commit stores the bpf_link pointers returned when attaching BPF sample
filters in an xyarray on the evsel and destroys them on cleanup and error
paths to fix a memory leak.

> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c
> index bcd81084e3420..664484703fc3d 100644
> --- a/tools/perf/util/bpf-filter.c
> +++ b/tools/perf/util/bpf-filter.c
> @@ -66,6 +66,7 @@
>  #include "bpf_skel/sample_filter.skel.h"
>  
>  #define FD(e, x, y) (*(int *)xyarray__entry(e->core.fd, x, y))
> +#define LINK(e, x, y) (*(struct bpf_link **)xyarray__entry(e->bpf_links, x, y))
>  
>  #define __PERF_SAMPLE_TYPE(tt, st, opt)	{ tt, #st, opt }
>  #define PERF_SAMPLE_TYPE(_st, opt)	__PERF_SAMPLE_TYPE(PBF_TERM_##_st, PERF_SAMPLE_##_st, opt)
> @@ -526,6 +527,15 @@ int perf_bpf_filter__prepare(struct evsel *evsel, struct target *target)
>  		goto err;
>  	}
>  
> +	evsel->bpf_links = xyarray__new(xyarray__max_x(evsel->core.fd),
> +					xyarray__max_y(evsel->core.fd),
> +					sizeof(link));
> +	if (evsel->bpf_links == NULL) {
> +		ret = -errno;
> +		pr_err("Failed to allocate bpf links\n");
> +		goto err;
> +	}
> +
>  	prog = skel->progs.perf_sample_filter;
>  	for (x = 0; x < xyarray__max_x(evsel->core.fd); x++) {
>  		for (y = 0; y < xyarray__max_y(evsel->core.fd); y++) {
> @@ -536,6 +546,8 @@ int perf_bpf_filter__prepare(struct evsel *evsel, struct target *target)
>  				ret = PTR_ERR(link);
>  				goto err;
>  			}
> +
> +			LINK(evsel, x, y) = link;
>  		}
>  	}
>  	free(entry);
> @@ -553,6 +565,15 @@ int perf_bpf_filter__prepare(struct evsel *evsel, struct target *target)
>  			free(pfi);
>  		}
>  	}
> +	if (evsel->bpf_links) {
> +		for (x = 0; x < xyarray__max_x(evsel->bpf_links); x++) {
> +			for (y = 0; y < xyarray__max_y(evsel->bpf_links); y++) {
> +				bpf_link__destroy(LINK(evsel, x, y));
> +			}
> +		}
> +		xyarray__delete(evsel->bpf_links);
> +		evsel->bpf_links = NULL;
> +	}

[Severity: Medium]
Can this error cleanup block ever be reached when attaching fails?

In perf_bpf_filter__prepare(), the attachment loop checks the return value
of bpf_program__attach_perf_event_opts() with IS_ERR(link):

tools/perf/util/bpf-filter.c:perf_bpf_filter__prepare() {
	...
	for (x = 0; x < xyarray__max_x(evsel->core.fd); x++) {
		for (y = 0; y < xyarray__max_y(evsel->core.fd); y++) {
			link = bpf_program__attach_perf_event_opts(prog, FD(evsel, x, y),
								   &pe_opts);
			if (IS_ERR(link)) {
				pr_err("Failed to attach perf sample-filter program\n");
				ret = PTR_ERR(link);
				goto err;
			}

			LINK(evsel, x, y) = link;
		}
	}
}

However, in tools/lib/bpf/libbpf.c, bpf_program__attach_perf_event_opts()
returns libbpf_err_ptr(err) on failure, which sets errno and returns NULL
rather than an error pointer:

tools/lib/bpf/libbpf.c:bpf_program__attach_perf_event_opts() {
	...
	return &link->link;
err_out:
	if (link_fd >= 0)
		close(link_fd);
	free(link);
	return libbpf_err_ptr(err);
}

Because IS_ERR(NULL) is false, an attachment failure stores NULL into
LINK(evsel, x, y) and continues instead of jumping to err, leaving the
evsel->bpf_links cleanup at err unreachable and returning 0.

>  	sample_filter_bpf__destroy(skel);
>  	return ret;
>  }
> @@ -566,6 +587,17 @@ int perf_bpf_filter__destroy(struct evsel *evsel)
>  		list_del(&expr->list);
>  		free(expr);
>  	}
> +
> +	if (evsel->bpf_links) {
> +		for (int x = 0; x < xyarray__max_x(evsel->bpf_links); x++) {
> +			for (int y = 0; y < xyarray__max_y(evsel->bpf_links); y++) {
> +				bpf_link__destroy(LINK(evsel, x, y));
> +			}
> +		}
> +		xyarray__delete(evsel->bpf_links);
> +		evsel->bpf_links = NULL;
> +	}
> +

[Severity: Medium]
Does calling bpf_link__destroy() here and in the error path of
perf_bpf_filter__prepare() cause a double close on the perf_event file
descriptors in evsel->core.fd?

In perf_bpf_filter__prepare(), FD(evsel, x, y) is passed directly to
bpf_program__attach_perf_event_opts(), which stores that descriptor in
perf_link->perf_event_fd and sets the detach callback to
bpf_link_perf_detach().

When bpf_link__destroy() is later called, bpf_link_perf_detach() disables
and closes perf_link->perf_event_fd in addition to link->fd:

tools/lib/bpf/libbpf.c:bpf_link_perf_detach() {
	...
	if (ioctl(perf_link->perf_event_fd, PERF_EVENT_IOC_DISABLE, 0) < 0)
		err = -errno;

	if (perf_link->perf_event_fd != link->fd)
		close(perf_link->perf_event_fd);
	close(link->fd);
	...
}

During normal teardown in evlist__put(), evlist__close() closes all
FD(evsel, x, y) descriptors before evlist__purge() -> evsel__put() ->
evsel__exit() -> perf_bpf_filter__destroy() runs:

tools/perf/util/evlist.c:evlist__put() {
	...
out_delete:
	evlist__free_stats(evlist);
	evlist__do_munmap(evlist);
	evlist__close(evlist);
	evlist__purge(evlist);
	evlist__exit(evlist);
	RC_CHK_FREE(evlist);
}

As a result, bpf_link__destroy() in perf_bpf_filter__destroy() performs
ioctl() and close() on FD(evsel, x, y) after it has already been closed.
Similarly, if perf_bpf_filter__prepare() fails partway through attaching
and jumps to err, bpf_link__destroy() closes the attached FD(evsel, x, y)
descriptors while leaving their positive FD numbers in evsel->core.fd, so
a subsequent evlist__close() closes them a second time.

>  	sample_filter_bpf__destroy(evsel->bpf_skel);
>  
>  	list_for_each_entry_safe(pfi, pos, &pinned_filters, list) {

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020053226.55936-1-namhyung@kernel.org?part=1