[17/39] bpf: resolve_pseudo_ldimm64(): take handling of a single ldimm64 insn into helper

Commit Message

Al Viro July 30, 2024, 5:16 a.m. UTC

From: Al Viro <[e-mail]>

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 <[e-mail]>
---
 kernel/bpf/verifier.c | 342 +++++++++++++++++++++---------------------
 1 file changed, 172 insertions(+), 170 deletions(-)

Comments

Andrii Nakryiko Aug. 6, 2024, 10:32 p.m. UTC | #1

On Mon, Jul 29, 2024 at 10:20 PM <[e-mail]> wrote:
>
> From: Al Viro <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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 <[e-mail]>
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 <[e-mail]>

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);
+    struct bpf_map *map;
+    int i;
+
+    map = __bpf_map_get(f);
+    if (IS_ERR(map)) {
+        verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
+        return PTR_ERR(map);
+    }
+
+    /* check whether we recorded this map already */
+    for (i = 0; i < env->used_map_cnt; i++) {
+        if (env->used_maps[i] == map) {
+            *reused = true;
+            fdput(f);
+            return i;
+        }
+    }
+
+    if (env->used_map_cnt >= MAX_USED_MAPS) {
+        verbose(env, "The total number of maps per program has
reached the limit of %u\n",
+            MAX_USED_MAPS);
+        fdput(f);
+        return -E2BIG;
+    }
+
+    if (env->prog->sleepable)
+        atomic64_inc(&map->sleepable_refcnt);
+
+    /* hold the map. If the program is rejected by verifier,
+     * the map will be released by release_maps() or it
+     * will be used by the valid program until it's unloaded
+     * and all maps are released in bpf_free_used_maps()
+     */
+    bpf_map_inc(map);
+
+    *reused = false;
+    env->used_maps[env->used_map_cnt++] = map;
+
+    fdput(f);
+
+    return env->used_map_cnt - 1;
+
+}
+
 /* find and rewrite pseudo imm in ld_imm64 instructions:
  *
  * 1. if it accesses map FD, replace it with actual map pointer.
@@ -18876,7 +18928,7 @@ static int resolve_pseudo_ldimm64(struct
bpf_verifier_env *env)
 {
     struct bpf_insn *insn = env->prog->insnsi;
     int insn_cnt = env->prog->len;
-    int i, j, err;
+    int i, err;

     err = bpf_prog_calc_tag(env->prog);
     if (err)
@@ -18893,9 +18945,10 @@ static int resolve_pseudo_ldimm64(struct
bpf_verifier_env *env)
         if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) {
             struct bpf_insn_aux_data *aux;
             struct bpf_map *map;
-            struct fd f;
+            int map_idx;
             u64 addr;
             u32 fd;
+            bool reused;

             if (i == insn_cnt - 1 || insn[1].code != 0 ||
                 insn[1].dst_reg != 0 || insn[1].src_reg != 0 ||
@@ -18956,20 +19009,18 @@ static int resolve_pseudo_ldimm64(struct
bpf_verifier_env *env)
                 break;
             }

-            f = fdget(fd);
-            map = __bpf_map_get(f);
-            if (IS_ERR(map)) {
-                verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
-                return PTR_ERR(map);
-            }
+            map_idx = add_used_map_from_fd(env, fd, &reused);
+            if (map_idx < 0)
+                return map_idx;
+            map = env->used_maps[map_idx];
+
+            aux = &env->insn_aux_data[i];
+            aux->map_index = map_idx;

             err = check_map_prog_compatibility(env, map, env->prog);
-            if (err) {
-                fdput(f);
+            if (err)
                 return err;
-            }

-            aux = &env->insn_aux_data[i];
             if (insn[0].src_reg == BPF_PSEUDO_MAP_FD ||
                 insn[0].src_reg == BPF_PSEUDO_MAP_IDX) {
                 addr = (unsigned long)map;
@@ -18978,13 +19029,11 @@ static int resolve_pseudo_ldimm64(struct
bpf_verifier_env *env)

                 if (off >= BPF_MAX_VAR_OFF) {
                     verbose(env, "direct value offset of %u is not
allowed\n", off);
-                    fdput(f);
                     return -EINVAL;
                 }

                 if (!map->ops->map_direct_value_addr) {
                     verbose(env, "no direct value access support for
this map type\n");
-                    fdput(f);
                     return -EINVAL;
                 }

@@ -18992,7 +19041,6 @@ static int resolve_pseudo_ldimm64(struct
bpf_verifier_env *env)
                 if (err) {
                     verbose(env, "invalid access to map value
pointer, value_size=%u off=%u\n",
                         map->value_size, off);
-                    fdput(f);
                     return err;
                 }

@@ -19003,70 +19051,39 @@ static int resolve_pseudo_ldimm64(struct
bpf_verifier_env *env)
             insn[0].imm = (u32)addr;
             insn[1].imm = addr >> 32;

-            /* check whether we recorded this map already */
-            for (j = 0; j < env->used_map_cnt; j++) {
-                if (env->used_maps[j] == map) {
-                    aux->map_index = j;
-                    fdput(f);
-                    goto next_insn;
-                }
-            }
-
-            if (env->used_map_cnt >= MAX_USED_MAPS) {
-                verbose(env, "The total number of maps per program
has reached the limit of %u\n",
-                    MAX_USED_MAPS);
-                fdput(f);
-                return -E2BIG;
-            }
-
-            if (env->prog->sleepable)
-                atomic64_inc(&map->sleepable_refcnt);
-            /* hold the map. If the program is rejected by verifier,
-             * the map will be released by release_maps() or it
-             * will be used by the valid program until it's unloaded
-             * and all maps are released in bpf_free_used_maps()
-             */
-            bpf_map_inc(map);
-
-            aux->map_index = env->used_map_cnt;
-            env->used_maps[env->used_map_cnt++] = map;
+            /* proceed with extra checks only if its newly added used map */
+            if (reused)
+                goto next_insn;

             if (bpf_map_is_cgroup_storage(map) &&
                 bpf_cgroup_storage_assign(env->prog->aux, map)) {
                 verbose(env, "only one cgroup storage of each type is
allowed\n");
-                fdput(f);
                 return -EBUSY;
             }
             if (map->map_type == BPF_MAP_TYPE_ARENA) {
                 if (env->prog->aux->arena) {
                     verbose(env, "Only one arena per program\n");
-                    fdput(f);
                     return -EBUSY;
                 }
                 if (!env->allow_ptr_leaks || !env->bpf_capable) {
                     verbose(env, "CAP_BPF and CAP_PERFMON are
required to use arena\n");
-                    fdput(f);
                     return -EPERM;
                 }
                 if (!env->prog->jit_requested) {
                     verbose(env, "JIT is required to use arena\n");
-                    fdput(f);
                     return -EOPNOTSUPP;
                 }
                 if (!bpf_jit_supports_arena()) {
                     verbose(env, "JIT doesn't support arena\n");
-                    fdput(f);
                     return -EOPNOTSUPP;
                 }
                 env->prog->aux->arena = (void *)map;
                 if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
                     verbose(env, "arena's user address must be set
via map_extra or mmap()\n");
-                    fdput(f);
                     return -EINVAL;
                 }
             }

-            fdput(f);
 next_insn:
             insn++;
             i++;



[...]

Christian Brauner Aug. 7, 2024, 10:29 a.m. UTC | #2

On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> On Mon, Jul 29, 2024 at 10:20 PM <[e-mail]> wrote:
> >
> > From: Al Viro <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>
> 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 <[e-mail]>
> 
> 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.

Andrii Nakryiko Aug. 7, 2024, 3:30 p.m. UTC | #3

On Wed, Aug 7, 2024 at 3:30 AM Christian Brauner <[e-mail]> wrote:
>
> On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> > On Mon, Jul 29, 2024 at 10:20 PM <[e-mail]> wrote:
> > >
> > > From: Al Viro <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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 <[e-mail]>
> > 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 <[e-mail]>
> >
> > 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.

That was the point of Al's next patch in the series, so I didn't want
to do it in this one that just refactored the logic of adding maps.
But I can fold that in and send it to bpf-next.

Alexei Starovoitov Aug. 8, 2024, 4:51 p.m. UTC | #4

On Wed, Aug 7, 2024 at 8:31 AM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Wed, Aug 7, 2024 at 3:30 AM Christian Brauner <[e-mail]> wrote:
> >
> > On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> > > On Mon, Jul 29, 2024 at 10:20 PM <[e-mail]> wrote:
> > > >
> > > > From: Al Viro <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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 <[e-mail]>
> > > 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 <[e-mail]>
> > >
> > > 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.
>
> That was the point of Al's next patch in the series, so I didn't want
> to do it in this one that just refactored the logic of adding maps.
> But I can fold that in and send it to bpf-next.

+1.

The bpf changes look ok and Andrii's approach is easier to grasp.
It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
so it goes through bpf CI and our other testing.

bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
and fderr, so pretty much independent from other patches.

Andrii Nakryiko Aug. 8, 2024, 8:35 p.m. UTC | #5

On Thu, Aug 8, 2024 at 9:51 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Wed, Aug 7, 2024 at 8:31 AM Andrii Nakryiko
> <[e-mail]> wrote:
> >
> > On Wed, Aug 7, 2024 at 3:30 AM Christian Brauner <[e-mail]> wrote:
> > >
> > > On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> > > > On Mon, Jul 29, 2024 at 10:20 PM <[e-mail]> wrote:
> > > > >
> > > > > From: Al Viro <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  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 <[e-mail]>
> > > > 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 <[e-mail]>
> > > >
> > > > 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.
> >
> > That was the point of Al's next patch in the series, so I didn't want
> > to do it in this one that just refactored the logic of adding maps.
> > But I can fold that in and send it to bpf-next.
>
> +1.
>
> The bpf changes look ok and Andrii's approach is easier to grasp.
> It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
> so it goes through bpf CI and our other testing.
>
> bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
> and fderr, so pretty much independent from other patches.

Ok, so CLASS(fd, f) won't work just yet because of peculiar
__bpf_map_get() contract: if it gets valid struct fd but it doesn't
contain a valid struct bpf_map, then __bpf_map_get() does fdput()
internally. In all other cases the caller has to do fdput() and
returned struct bpf_map's refcount has to be bumped by the caller
(__bpf_map_get() doesn't do that, I guess that's why it's
double-underscored).

I think the reason it was done was just a convenience to not have to
get/put bpf_map for temporary uses (and instead rely on file's
reference keeping bpf_map alive), plus we have bpf_map_inc() and
bpf_map_inc_uref() variants, so in some cases we need to bump just
refcount, and in some both user and normal refcounts.

So can't use CLASS(fd, ...) without some more clean up.

Alexei, how about changing __bpf_map_get(struct fd f) to
__bpf_map_get_from_fd(int ufd), doing fdget/fdput internally, and
always returning bpf_map with (normal) refcount bumped (if successful,
of course). We can then split bpf_map_inc_with_uref() into just
bpf_map_inc() and bpf_map_inc_uref(), and callers will be able to do
extra uref-only increment, if necessary.

I can do that as a pre-patch, there are about 15 callers, so not too
much work to clean this up. Let me know.

Alexei Starovoitov Aug. 9, 2024, 1:23 a.m. UTC | #6

On Thu, Aug 8, 2024 at 1:35 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Thu, Aug 8, 2024 at 9:51 AM Alexei Starovoitov
> <[e-mail]> wrote:
> >
> > On Wed, Aug 7, 2024 at 8:31 AM Andrii Nakryiko
> > <[e-mail]> wrote:
> > >
> > > On Wed, Aug 7, 2024 at 3:30 AM Christian Brauner <[e-mail]> wrote:
> > > >
> > > > On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> > > > > On Mon, Jul 29, 2024 at 10:20 PM <[e-mail]> wrote:
> > > > > >
> > > > > > From: Al Viro <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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 <[e-mail]>
> > > > > 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 <[e-mail]>
> > > > >
> > > > > 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.
> > >
> > > That was the point of Al's next patch in the series, so I didn't want
> > > to do it in this one that just refactored the logic of adding maps.
> > > But I can fold that in and send it to bpf-next.
> >
> > +1.
> >
> > The bpf changes look ok and Andrii's approach is easier to grasp.
> > It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
> > so it goes through bpf CI and our other testing.
> >
> > bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
> > and fderr, so pretty much independent from other patches.
>
> Ok, so CLASS(fd, f) won't work just yet because of peculiar
> __bpf_map_get() contract: if it gets valid struct fd but it doesn't
> contain a valid struct bpf_map, then __bpf_map_get() does fdput()
> internally. In all other cases the caller has to do fdput() and
> returned struct bpf_map's refcount has to be bumped by the caller
> (__bpf_map_get() doesn't do that, I guess that's why it's
> double-underscored).
>
> I think the reason it was done was just a convenience to not have to
> get/put bpf_map for temporary uses (and instead rely on file's
> reference keeping bpf_map alive), plus we have bpf_map_inc() and
> bpf_map_inc_uref() variants, so in some cases we need to bump just
> refcount, and in some both user and normal refcounts.
>
> So can't use CLASS(fd, ...) without some more clean up.
>
> Alexei, how about changing __bpf_map_get(struct fd f) to
> __bpf_map_get_from_fd(int ufd), doing fdget/fdput internally, and
> always returning bpf_map with (normal) refcount bumped (if successful,
> of course). We can then split bpf_map_inc_with_uref() into just
> bpf_map_inc() and bpf_map_inc_uref(), and callers will be able to do
> extra uref-only increment, if necessary.
>
> I can do that as a pre-patch, there are about 15 callers, so not too
> much work to clean this up. Let me know.

Yeah. Let's kill __bpf_map_get(struct fd ..) altogether.
This logic was added in 2014.
fdget() had to be first and fdput() last to make sure
the map won't disappear while sys_bpf command is running.
All of the places can use bpf_map_get(), bpf_map_put() pair
and rely on map->refcnt, but...

- it's atomic64_inc(&map->refcnt); The cost is probably
in the noise compared to all the work that map sys_bpf commands do.

- It also opens new fuzzing opportunity to do some map operation
in one thread and close(map_fd) in the other, so map->usercnt can
drop to zero and map_release_uref() cleanup can start while
the other thread is still busy doing something like map_update_elem().
It can be mitigated by doing bpf_map_get_with_uref(), but two
atomic64_inc() is kinda too much.

So let's remove __bpf_map_get() and replace all users with bpf_map_get(),
but we may need to revisit that later.

Andrii Nakryiko Aug. 9, 2024, 5:23 p.m. UTC | #7

On Thu, Aug 8, 2024 at 6:23 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Thu, Aug 8, 2024 at 1:35 PM Andrii Nakryiko
> <[e-mail]> wrote:
> >
> > On Thu, Aug 8, 2024 at 9:51 AM Alexei Starovoitov
> > <[e-mail]> wrote:
> > >
> > > On Wed, Aug 7, 2024 at 8:31 AM Andrii Nakryiko
> > > <[e-mail]> wrote:
> > > >
> > > > On Wed, Aug 7, 2024 at 3:30 AM Christian Brauner <[e-mail]> wrote:
> > > > >
> > > > > On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:
> > > > > > On Mon, Jul 29, 2024 at 10:20 PM <[e-mail]> wrote:
> > > > > > >
> > > > > > > From: Al Viro <[e-mail]>
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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 <[e-mail]>
> > > > > > 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 <[e-mail]>
> > > > > >
> > > > > > 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.
> > > >
> > > > That was the point of Al's next patch in the series, so I didn't want
> > > > to do it in this one that just refactored the logic of adding maps.
> > > > But I can fold that in and send it to bpf-next.
> > >
> > > +1.
> > >
> > > The bpf changes look ok and Andrii's approach is easier to grasp.
> > > It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
> > > so it goes through bpf CI and our other testing.
> > >
> > > bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
> > > and fderr, so pretty much independent from other patches.
> >
> > Ok, so CLASS(fd, f) won't work just yet because of peculiar
> > __bpf_map_get() contract: if it gets valid struct fd but it doesn't
> > contain a valid struct bpf_map, then __bpf_map_get() does fdput()
> > internally. In all other cases the caller has to do fdput() and
> > returned struct bpf_map's refcount has to be bumped by the caller
> > (__bpf_map_get() doesn't do that, I guess that's why it's
> > double-underscored).
> >
> > I think the reason it was done was just a convenience to not have to
> > get/put bpf_map for temporary uses (and instead rely on file's
> > reference keeping bpf_map alive), plus we have bpf_map_inc() and
> > bpf_map_inc_uref() variants, so in some cases we need to bump just
> > refcount, and in some both user and normal refcounts.
> >
> > So can't use CLASS(fd, ...) without some more clean up.
> >
> > Alexei, how about changing __bpf_map_get(struct fd f) to
> > __bpf_map_get_from_fd(int ufd), doing fdget/fdput internally, and
> > always returning bpf_map with (normal) refcount bumped (if successful,
> > of course). We can then split bpf_map_inc_with_uref() into just
> > bpf_map_inc() and bpf_map_inc_uref(), and callers will be able to do
> > extra uref-only increment, if necessary.
> >
> > I can do that as a pre-patch, there are about 15 callers, so not too
> > much work to clean this up. Let me know.
>
> Yeah. Let's kill __bpf_map_get(struct fd ..) altogether.
> This logic was added in 2014.
> fdget() had to be first and fdput() last to make sure
> the map won't disappear while sys_bpf command is running.
> All of the places can use bpf_map_get(), bpf_map_put() pair
> and rely on map->refcnt, but...
>
> - it's atomic64_inc(&map->refcnt); The cost is probably
> in the noise compared to all the work that map sys_bpf commands do.
>

agreed, not too worried about this

> - It also opens new fuzzing opportunity to do some map operation
> in one thread and close(map_fd) in the other, so map->usercnt can
> drop to zero and map_release_uref() cleanup can start while
> the other thread is still busy doing something like map_update_elem().
> It can be mitigated by doing bpf_map_get_with_uref(), but two
> atomic64_inc() is kinda too much.
>

yep, with_uref() is an overkill for most cases. I'd rather fix any
such bugs, if we have them.

> So let's remove __bpf_map_get() and replace all users with bpf_map_get(),
> but we may need to revisit that later.

Ok, I will probably send something next week.

Al Viro Aug. 10, 2024, 3:29 a.m. UTC | #8

On Thu, Aug 08, 2024 at 09:51:34AM -0700, Alexei Starovoitov wrote:

> The bpf changes look ok and Andrii's approach is easier to grasp.
> It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
> so it goes through bpf CI and our other testing.
> 
> bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
> and fderr, so pretty much independent from other patches.

Representation change and switch to accessors do matter, though.
OTOH, I can put just those into never-rebased branch (basically,
"introduce fd_file(), convert all accessors to it" +
"struct fd representation change" + possibly "add struct fd constructors,
get rid of __to_fd()", for completeness sake), so you could pull it.
Otherwise you'll get textual conflicts on all those f.file vs. fd_file(f)...

Andrii Nakryiko Aug. 12, 2024, 8:05 p.m. UTC | #9

On Fri, Aug 9, 2024 at 8:29 PM Al Viro <[e-mail]> wrote:
>
> On Thu, Aug 08, 2024 at 09:51:34AM -0700, Alexei Starovoitov wrote:
>
> > The bpf changes look ok and Andrii's approach is easier to grasp.
> > It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
> > so it goes through bpf CI and our other testing.
> >
> > bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
> > and fderr, so pretty much independent from other patches.
>
> Representation change and switch to accessors do matter, though.
> OTOH, I can put just those into never-rebased branch (basically,
> "introduce fd_file(), convert all accessors to it" +
> "struct fd representation change" + possibly "add struct fd constructors,
> get rid of __to_fd()", for completeness sake), so you could pull it.
> Otherwise you'll get textual conflicts on all those f.file vs. fd_file(f)...

Yep, makes sense. Let's do that, we can merge that branch into
bpf-next/master and I will follow up with my changes on top of that.

Let's just drop the do_one_ldimm64() extraction, and keep fdput(f)
logic, plus add fd_file() accessor changes. I'll then add a switch to
CLASS(fd) after a bit more BPF-specific clean ups. This code is pretty
sensitive, so I'd rather have all the non-trivial refactoring done
separately. Thanks!

Al Viro Aug. 13, 2024, 2:06 a.m. UTC | #10

On Mon, Aug 12, 2024 at 01:05:19PM -0700, Andrii Nakryiko wrote:
> On Fri, Aug 9, 2024 at 8:29???PM Al Viro <[e-mail]> wrote:
> >
> > On Thu, Aug 08, 2024 at 09:51:34AM -0700, Alexei Starovoitov wrote:
> >
> > > The bpf changes look ok and Andrii's approach is easier to grasp.
> > > It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
> > > so it goes through bpf CI and our other testing.
> > >
> > > bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
> > > and fderr, so pretty much independent from other patches.
> >
> > Representation change and switch to accessors do matter, though.
> > OTOH, I can put just those into never-rebased branch (basically,
> > "introduce fd_file(), convert all accessors to it" +
> > "struct fd representation change" + possibly "add struct fd constructors,
> > get rid of __to_fd()", for completeness sake), so you could pull it.
> > Otherwise you'll get textual conflicts on all those f.file vs. fd_file(f)...
> 
> Yep, makes sense. Let's do that, we can merge that branch into
> bpf-next/master and I will follow up with my changes on top of that.
> 
> Let's just drop the do_one_ldimm64() extraction, and keep fdput(f)
> logic, plus add fd_file() accessor changes. I'll then add a switch to
> CLASS(fd) after a bit more BPF-specific clean ups. This code is pretty
> sensitive, so I'd rather have all the non-trivial refactoring done
> separately. Thanks!

Done (#stable-struct_fd); BTW, which tree do you want "convert __bpf_prog_get()
to CLASS(fd)" to go through?

Andrii Nakryiko Aug. 13, 2024, 3:32 a.m. UTC | #11

On Mon, Aug 12, 2024 at 7:06 PM Al Viro <[e-mail]> wrote:
>
> On Mon, Aug 12, 2024 at 01:05:19PM -0700, Andrii Nakryiko wrote:
> > On Fri, Aug 9, 2024 at 8:29???PM Al Viro <[e-mail]> wrote:
> > >
> > > On Thu, Aug 08, 2024 at 09:51:34AM -0700, Alexei Starovoitov wrote:
> > >
> > > > The bpf changes look ok and Andrii's approach is easier to grasp.
> > > > It's better to route bpf conversion to CLASS(fd,..) via bpf-next,
> > > > so it goes through bpf CI and our other testing.
> > > >
> > > > bpf patches don't seem to depend on newly added CLASS(fd_pos, ...
> > > > and fderr, so pretty much independent from other patches.
> > >
> > > Representation change and switch to accessors do matter, though.
> > > OTOH, I can put just those into never-rebased branch (basically,
> > > "introduce fd_file(), convert all accessors to it" +
> > > "struct fd representation change" + possibly "add struct fd constructors,
> > > get rid of __to_fd()", for completeness sake), so you could pull it.
> > > Otherwise you'll get textual conflicts on all those f.file vs. fd_file(f)...
> >
> > Yep, makes sense. Let's do that, we can merge that branch into
> > bpf-next/master and I will follow up with my changes on top of that.
> >
> > Let's just drop the do_one_ldimm64() extraction, and keep fdput(f)
> > logic, plus add fd_file() accessor changes. I'll then add a switch to
> > CLASS(fd) after a bit more BPF-specific clean ups. This code is pretty
> > sensitive, so I'd rather have all the non-trivial refactoring done
> > separately. Thanks!
>
> Done (#stable-struct_fd);

great, thanks, I'll look at this tomorrow

> BTW, which tree do you want "convert __bpf_prog_get()
> to CLASS(fd)" to go through?

So we seem to have the following for BPF-related stuff:

[PATCH 16/39] convert __bpf_prog_get() to CLASS(fd, ...)

This looks to be ready to go in.

[PATCH 17/39] bpf: resolve_pseudo_ldimm64(): take handling of a single
ldimm64 insn into helper

This one I'd like to rework differently and land it through bpf-next.

[PATCH 18/39] bpf maps: switch to CLASS(fd, ...)

This one touches __bpf_map_get() which I'm going to remove or refactor
as part of the abovementioned refactoring, so there will be conflicts.

[PATCH 19/39] fdget_raw() users: switch to CLASS(fd_raw, ...)

This one touches a bunch of cases across multiple systems, including
BPF's kernel/bpf/bpf_inode_storage.c.


So how about this. We take #16 as is through bpf-next, change how #17
is done, take 18 mostly as is but adjust as necessary. As for #19, if
you could split out changes in bpf_inode_storage.c to a separate
patch, we can also apply it in bpf-next as one coherent set. I'll send
all that as one complete patch set for you to do the final review.

WDYT?

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4cb5441ad75f..d54f61e12062 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -18414,11 +18414,180 @@  static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
  *
  * NOTE: btf_vmlinux is required for converting pseudo btf_id.
  */
+static int do_one_ldimm64(struct bpf_verifier_env *env,
+			  struct bpf_insn *insn,
+			  struct bpf_insn_aux_data *aux)
+{
+	struct bpf_map *map;
+	struct fd f;
+	u64 addr;
+	u32 fd;
+	int err;
+
+	if (insn[0].src_reg == 0)
+		/* valid generic load 64-bit imm */
+		return 0;
+
+	if (insn[0].src_reg == BPF_PSEUDO_BTF_ID)
+		return check_pseudo_btf_id(env, insn, aux);
+
+	if (insn[0].src_reg == BPF_PSEUDO_FUNC) {
+		aux->ptr_type = PTR_TO_FUNC;
+		return 0;
+	}
+
+	/* In final convert_pseudo_ld_imm64() step, this is
+	 * converted into regular 64-bit imm load insn.
+	 */
+	switch (insn[0].src_reg) {
+	case BPF_PSEUDO_MAP_VALUE:
+	case BPF_PSEUDO_MAP_IDX_VALUE:
+		break;
+	case BPF_PSEUDO_MAP_FD:
+	case BPF_PSEUDO_MAP_IDX:
+		if (insn[1].imm == 0)
+			break;
+		fallthrough;
+	default:
+		verbose(env, "unrecognized bpf_ld_imm64 insn\n");
+		return -EINVAL;
+	}
+
+	switch (insn[0].src_reg) {
+	case BPF_PSEUDO_MAP_IDX_VALUE:
+	case BPF_PSEUDO_MAP_IDX:
+		if (bpfptr_is_null(env->fd_array)) {
+			verbose(env, "fd_idx without fd_array is invalid\n");
+			return -EPROTO;
+		}
+		if (copy_from_bpfptr_offset(&fd, env->fd_array,
+					    insn[0].imm * sizeof(fd),
+					    sizeof(fd)))
+			return -EFAULT;
+		break;
+	default:
+		fd = insn[0].imm;
+		break;
+	}
+
+	f = fdget(fd);
+	map = __bpf_map_get(f);
+	if (IS_ERR(map)) {
+		verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
+		return PTR_ERR(map);
+	}
+
+	err = check_map_prog_compatibility(env, map, env->prog);
+	if (err) {
+		fdput(f);
+		return err;
+	}
+
+	if (insn[0].src_reg == BPF_PSEUDO_MAP_FD ||
+	    insn[0].src_reg == BPF_PSEUDO_MAP_IDX) {
+		addr = (unsigned long)map;
+	} else {
+		u32 off = insn[1].imm;
+
+		if (off >= BPF_MAX_VAR_OFF) {
+			verbose(env, "direct value offset of %u is not allowed\n", off);
+			fdput(f);
+			return -EINVAL;
+		}
+
+		if (!map->ops->map_direct_value_addr) {
+			verbose(env, "no direct value access support for this map type\n");
+			fdput(f);
+			return -EINVAL;
+		}
+
+		err = map->ops->map_direct_value_addr(map, &addr, off);
+		if (err) {
+			verbose(env, "invalid access to map value pointer, value_size=%u off=%u\n",
+				map->value_size, off);
+			fdput(f);
+			return err;
+		}
+
+		aux->map_off = off;
+		addr += off;
+	}
+
+	insn[0].imm = (u32)addr;
+	insn[1].imm = addr >> 32;
+
+	/* check whether we recorded this map already */
+	for (int i = 0; i < env->used_map_cnt; i++) {
+		if (env->used_maps[i] == map) {
+			aux->map_index = i;
+			fdput(f);
+			return 0;
+		}
+	}
+
+	if (env->used_map_cnt >= MAX_USED_MAPS) {
+		verbose(env, "The total number of maps per program has reached the limit of %u\n",
+			MAX_USED_MAPS);
+		fdput(f);
+		return -E2BIG;
+	}
+
+	if (env->prog->sleepable)
+		atomic64_inc(&map->sleepable_refcnt);
+	/* hold the map. If the program is rejected by verifier,
+	 * the map will be released by release_maps() or it
+	 * will be used by the valid program until it's unloaded
+	 * and all maps are released in bpf_free_used_maps()
+	 */
+	bpf_map_inc(map);
+
+	aux->map_index = env->used_map_cnt;
+	env->used_maps[env->used_map_cnt++] = map;
+
+	if (bpf_map_is_cgroup_storage(map) &&
+	    bpf_cgroup_storage_assign(env->prog->aux, map)) {
+		verbose(env, "only one cgroup storage of each type is allowed\n");
+		fdput(f);
+		return -EBUSY;
+	}
+	if (map->map_type == BPF_MAP_TYPE_ARENA) {
+		if (env->prog->aux->arena) {
+			verbose(env, "Only one arena per program\n");
+			fdput(f);
+			return -EBUSY;
+		}
+		if (!env->allow_ptr_leaks || !env->bpf_capable) {
+			verbose(env, "CAP_BPF and CAP_PERFMON are required to use arena\n");
+			fdput(f);
+			return -EPERM;
+		}
+		if (!env->prog->jit_requested) {
+			verbose(env, "JIT is required to use arena\n");
+			fdput(f);
+			return -EOPNOTSUPP;
+		}
+		if (!bpf_jit_supports_arena()) {
+			verbose(env, "JIT doesn't support arena\n");
+			fdput(f);
+			return -EOPNOTSUPP;
+		}
+		env->prog->aux->arena = (void *)map;
+		if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
+			verbose(env, "arena's user address must be set via map_extra or mmap()\n");
+			fdput(f);
+			return -EINVAL;
+		}
+	}
+
+	fdput(f);
+	return 0;
+}
+
 static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 {
 	struct bpf_insn *insn = env->prog->insnsi;
 	int insn_cnt = env->prog->len;
-	int i, j, err;
+	int i, err;
 
 	err = bpf_prog_calc_tag(env->prog);
 	if (err)
@@ -18433,12 +18602,6 @@  static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 		}
 
 		if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) {
-			struct bpf_insn_aux_data *aux;
-			struct bpf_map *map;
-			struct fd f;
-			u64 addr;
-			u32 fd;
-
 			if (i == insn_cnt - 1 || insn[1].code != 0 ||
 			    insn[1].dst_reg != 0 || insn[1].src_reg != 0 ||
 			    insn[1].off != 0) {
@@ -18446,170 +18609,9 @@  static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 				return -EINVAL;
 			}
 
-			if (insn[0].src_reg == 0)
-				/* valid generic load 64-bit imm */
-				goto next_insn;
-
-			if (insn[0].src_reg == BPF_PSEUDO_BTF_ID) {
-				aux = &env->insn_aux_data[i];
-				err = check_pseudo_btf_id(env, insn, aux);
-				if (err)
-					return err;
-				goto next_insn;
-			}
-
-			if (insn[0].src_reg == BPF_PSEUDO_FUNC) {
-				aux = &env->insn_aux_data[i];
-				aux->ptr_type = PTR_TO_FUNC;
-				goto next_insn;
-			}
-
-			/* In final convert_pseudo_ld_imm64() step, this is
-			 * converted into regular 64-bit imm load insn.
-			 */
-			switch (insn[0].src_reg) {
-			case BPF_PSEUDO_MAP_VALUE:
-			case BPF_PSEUDO_MAP_IDX_VALUE:
-				break;
-			case BPF_PSEUDO_MAP_FD:
-			case BPF_PSEUDO_MAP_IDX:
-				if (insn[1].imm == 0)
-					break;
-				fallthrough;
-			default:
-				verbose(env, "unrecognized bpf_ld_imm64 insn\n");
-				return -EINVAL;
-			}
-
-			switch (insn[0].src_reg) {
-			case BPF_PSEUDO_MAP_IDX_VALUE:
-			case BPF_PSEUDO_MAP_IDX:
-				if (bpfptr_is_null(env->fd_array)) {
-					verbose(env, "fd_idx without fd_array is invalid\n");
-					return -EPROTO;
-				}
-				if (copy_from_bpfptr_offset(&fd, env->fd_array,
-							    insn[0].imm * sizeof(fd),
-							    sizeof(fd)))
-					return -EFAULT;
-				break;
-			default:
-				fd = insn[0].imm;
-				break;
-			}
-
-			f = fdget(fd);
-			map = __bpf_map_get(f);
-			if (IS_ERR(map)) {
-				verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
-				return PTR_ERR(map);
-			}
-
-			err = check_map_prog_compatibility(env, map, env->prog);
-			if (err) {
-				fdput(f);
+			err = do_one_ldimm64(env, insn, &env->insn_aux_data[i]);
+			if (err)
 				return err;
-			}
-
-			aux = &env->insn_aux_data[i];
-			if (insn[0].src_reg == BPF_PSEUDO_MAP_FD ||
-			    insn[0].src_reg == BPF_PSEUDO_MAP_IDX) {
-				addr = (unsigned long)map;
-			} else {
-				u32 off = insn[1].imm;
-
-				if (off >= BPF_MAX_VAR_OFF) {
-					verbose(env, "direct value offset of %u is not allowed\n", off);
-					fdput(f);
-					return -EINVAL;
-				}
-
-				if (!map->ops->map_direct_value_addr) {
-					verbose(env, "no direct value access support for this map type\n");
-					fdput(f);
-					return -EINVAL;
-				}
-
-				err = map->ops->map_direct_value_addr(map, &addr, off);
-				if (err) {
-					verbose(env, "invalid access to map value pointer, value_size=%u off=%u\n",
-						map->value_size, off);
-					fdput(f);
-					return err;
-				}
-
-				aux->map_off = off;
-				addr += off;
-			}
-
-			insn[0].imm = (u32)addr;
-			insn[1].imm = addr >> 32;
-
-			/* check whether we recorded this map already */
-			for (j = 0; j < env->used_map_cnt; j++) {
-				if (env->used_maps[j] == map) {
-					aux->map_index = j;
-					fdput(f);
-					goto next_insn;
-				}
-			}
-
-			if (env->used_map_cnt >= MAX_USED_MAPS) {
-				verbose(env, "The total number of maps per program has reached the limit of %u\n",
-					MAX_USED_MAPS);
-				fdput(f);
-				return -E2BIG;
-			}
-
-			if (env->prog->sleepable)
-				atomic64_inc(&map->sleepable_refcnt);
-			/* hold the map. If the program is rejected by verifier,
-			 * the map will be released by release_maps() or it
-			 * will be used by the valid program until it's unloaded
-			 * and all maps are released in bpf_free_used_maps()
-			 */
-			bpf_map_inc(map);
-
-			aux->map_index = env->used_map_cnt;
-			env->used_maps[env->used_map_cnt++] = map;
-
-			if (bpf_map_is_cgroup_storage(map) &&
-			    bpf_cgroup_storage_assign(env->prog->aux, map)) {
-				verbose(env, "only one cgroup storage of each type is allowed\n");
-				fdput(f);
-				return -EBUSY;
-			}
-			if (map->map_type == BPF_MAP_TYPE_ARENA) {
-				if (env->prog->aux->arena) {
-					verbose(env, "Only one arena per program\n");
-					fdput(f);
-					return -EBUSY;
-				}
-				if (!env->allow_ptr_leaks || !env->bpf_capable) {
-					verbose(env, "CAP_BPF and CAP_PERFMON are required to use arena\n");
-					fdput(f);
-					return -EPERM;
-				}
-				if (!env->prog->jit_requested) {
-					verbose(env, "JIT is required to use arena\n");
-					fdput(f);
-					return -EOPNOTSUPP;
-				}
-				if (!bpf_jit_supports_arena()) {
-					verbose(env, "JIT doesn't support arena\n");
-					fdput(f);
-					return -EOPNOTSUPP;
-				}
-				env->prog->aux->arena = (void *)map;
-				if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
-					verbose(env, "arena's user address must be set via map_extra or mmap()\n");
-					fdput(f);
-					return -EINVAL;
-				}
-			}
-
-			fdput(f);
-next_insn:
 			insn++;
 			i++;
 			continue;