On Wed, Aug 7, 2024 at 8:31 AM Andrii Nakryiko
[off-list ref] wrote:

On Wed, Aug 7, 2024 at 3:30 AM Christian Brauner [off-list ref] wrote:

quoted

On Tue, Aug 06, 2024 at 03:32:20PM GMT, Andrii Nakryiko wrote:

quoted

On Mon, Jul 29, 2024 at 10:20 PM [off-list ref] wrote:

quoted

From: Al Viro <viro@zeniv.linux.org.uk>

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@zeniv.linux.org.uk>
---
 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 [off-list ref]
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 [off-list ref]

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.