Re: [PATCH bpf-next 1/1] bpf: fix possible file descriptor leaks in verifier

From: Yonghong Song <yonghong.song@linux.dev>
To: Anton Protopopov <aspsk@isovalent.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jiri Olsa <jolsa@kernel.org>,
	Martin KaFai Lau <martin.lau@linux.dev>,
	Stanislav Fomichev <sdf@google.com>,
	bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next 1/1] bpf: fix possible file descriptor leaks in verifier
Date: Thu, 28 Mar 2024 20:23:03 -0700	[thread overview]
Message-ID: <cf8d5e75-dc75-4f86-be60-013f7ed7c688@linux.dev> (raw)
In-Reply-To: <20240328095657.1995763-1-aspsk@isovalent.com>

On 3/28/24 2:56 AM, Anton Protopopov wrote:
> The resolve_pseudo_ldimm64() function might have leaked file
> descriptors when BPF_MAP_TYPE_ARENA was used in a program (some
> error paths missed a corresponding fdput).
>
> Fix leaks and also extract code which adds maps to env->used_maps
> into a separate function. This simplifies code of resolve_pseudo_ldimm64
> and makes it possible to reuse this code later.  While at it, also add a
> verifier verbose message when the maps usage limit is reached.

Could you break this single patch into a series? For example,
for 'to reuse this code later', refactoring the code (without
funcitonality change) can be a separate patch, could you explain
what do you mean reusing the code later? Do you have some plans
to reuse the code later?

The fix for missing fdput() should be its own patch with
Fixes tag below.

You can have yet another patch to enhance verifier by adding
verbose messages.

>
> Fixes: 6082b6c328b5 ("bpf: Recognize addr_space_cast instruction in the verifier.")
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
>   kernel/bpf/verifier.c | 150 ++++++++++++++++++++++--------------------
>   1 file changed, 79 insertions(+), 71 deletions(-)
[...]