On 04-Mär 10:47, Andrii Nakryiko wrote:

On Wed, Mar 4, 2020 at 10:44 AM KP Singh [off-list ref] wrote:

quoted

On 04-Mär 19:37, Daniel Borkmann wrote:

quoted

On 3/4/20 4:47 PM, KP Singh wrote:

quoted

From: KP Singh <redacted>

As we need to introduce a third type of attachment for trampolines, the
flattened signature of arch_prepare_bpf_trampoline gets even more
complicated.

Refactor the prog and count argument to arch_prepare_bpf_trampoline to
use bpf_tramp_progs to simplify the addition and accounting for new
attachment types.

Signed-off-by: KP Singh <redacted>
Acked-by: Andrii Nakryiko <redacted>

[...]

quoted

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index c498f0fffb40..9f7e0328a644 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c

@@ -320,6 +320,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
    struct bpf_struct_ops_value *uvalue, *kvalue;
    const struct btf_member *member;
    const struct btf_type *t = st_ops->type;
+   struct bpf_tramp_progs *tprogs = NULL;
    void *udata, *kdata;
    int prog_fd, err = 0;
    void *image;

@@ -425,10 +426,18 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
                    goto reset_unlock;
            }
+           tprogs = kcalloc(BPF_TRAMP_MAX, sizeof(*tprogs), GFP_KERNEL);
+           if (!tprogs) {
+                   err = -ENOMEM;
+                   goto reset_unlock;
+           }
+

Looking over the code again, I'm quite certain that here's a memleak
since the kcalloc() is done in the for_each_member() loop in the ops
update but then going out of scope and in the exit path we only kfree
the last tprogs.

You're right, nice catch. Fixing it.

There is probably no need to do many allocations as well, just one
outside of the loop and reuse?

Yeah moved it out of the loop and before we grab the mutex, returning
an -ENOMEM directly.

Thanks for noticing this. Sending v4 now.

- KP

quoted

- KP

quoted

quoted

+           tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;
+           tprogs[BPF_TRAMP_FENTRY].nr_progs = 1;
            err = arch_prepare_bpf_trampoline(image,
                                              st_map->image + PAGE_SIZE,
                                              &st_ops->func_models[i], 0,
-                                             &prog, 1, NULL, 0, NULL);
+                                             tprogs, NULL);
            if (err < 0)
                    goto reset_unlock;

@@ -469,6 +478,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
    memset(uvalue, 0, map->value_size);
    memset(kvalue, 0, map->value_size);
  unlock:
+   kfree(tprogs);
    mutex_unlock(&st_map->lock);
    return err;
  }