On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> On Mon, Jul 29, 2024 at 10:20 PM <viro@xxxxxxxxxx> wrote:
> >
> > From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> >
> > Equivalent transformation. For one thing, it's easier to follow that way.
> > For another, that simplifies the control flow in the vicinity of struct fd
> > handling in there, which will allow a switch to CLASS(fd) and make the
> > thing much easier to verify wrt leaks.
> >
> > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > ---
> > kernel/bpf/verifier.c | 342 +++++++++++++++++++++---------------------
> > 1 file changed, 172 insertions(+), 170 deletions(-)
> >
>
> This looks unnecessarily intrusive. I think it's best to extract the
> logic of fetching and adding bpf_map by fd into a helper and that way
> contain fdget + fdput logic nicely. Something like below, which I can
> send to bpf-next.
>
> commit b5eec08241cc0263e560551de91eda73ccc5987d
> Author: Andrii Nakryiko <andrii@xxxxxxxxxx>
> Date: Tue Aug 6 14:31:34 2024 -0700
>
> bpf: factor out fetching bpf_map from FD and adding it to used_maps list
>
> Factor out the logic to extract bpf_map instances from FD embedded in
> bpf_insns, adding it to the list of used_maps (unless it's already
> there, in which case we just reuse map's index). This simplifies the
> logic in resolve_pseudo_ldimm64(), especially around `struct fd`
> handling, as all that is now neatly contained in the helper and doesn't
> leak into a dozen error handling paths.
>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index df3be12096cf..14e4ef687a59 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18865,6 +18865,58 @@ static bool bpf_map_is_cgroup_storage(struct
> bpf_map *map)
> map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
> }
>
> +/* Add map behind fd to used maps list, if it's not already there, and return
> + * its index. Also set *reused to true if this map was already in the list of
> + * used maps.
> + * Returns <0 on error, or >= 0 index, on success.
> + */
> +static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd,
> bool *reused)
> +{
> + struct fd f = fdget(fd);
Use CLASS(fd, f)(fd) and you can avoid all that fdput() stuff.
