[PATCH bpf-next v1 0/2] Follow ups for bpf-list set

[PATCH bpf-next v1 0/2] Follow ups for bpf-list set

[Kumar Kartikeya Dwivedi]

Make a few changes
 - Remove bpf_global_ma_set check at runtime, disallow calling bpf_obj_new during verification
 - Disable spin lock failure test when JIT does not support kfunc (s390x)

Kumar Kartikeya Dwivedi (2):
  bpf: Disallow calling bpf_obj_new_impl on bpf_mem_alloc_init failure
  selftests/bpf: Skip spin lock failure test on s390x

 kernel/bpf/helpers.c                               |  2 --
 kernel/bpf/verifier.c                              | 13 ++++++++++++-
 tools/testing/selftests/bpf/prog_tests/spin_lock.c |  6 ++++++
 3 files changed, 18 insertions(+), 3 deletions(-)


base-commit: db6bf999544c8c8dcae093e91eba4570647874b1
-- 
2.38.1

[PATCH bpf-next v1 1/2] bpf: Disallow calling bpf_obj_new_impl on bpf_mem_alloc_init failure

[Kumar Kartikeya Dwivedi]

Instead of checking bpf_global_ma_set at runtime on each allocation
inside bpf_obj_new_impl, simply disallow calling the kfunc in case
bpf_global_ma initialization failed during program verification.

The error generated when bpf_global_ma initialization fails:
...
21: (18) r1 = 0x7                     ; R1_w=7
23: (b7) r2 = 0                       ; R2_w=0
24: (85) call bpf_obj_new_impl#36585
bpf_global_ma initialization failed, can't call bpf_obj_new_impl
calling kernel function bpf_obj_new_impl is not allowed

Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/helpers.c  |  2 --
 kernel/bpf/verifier.c | 13 ++++++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 212e791d7452..bc02f55adc1f 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1760,8 +1760,6 @@ void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign)
 	u64 size = local_type_id__k;
 	void *p;
 
-	if (unlikely(!bpf_global_ma_set))
-		return NULL;
 	p = bpf_mem_alloc(&bpf_global_ma, size);
 	if (!p)
 		return NULL;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 195d24316750..f04bee7934a8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8746,6 +8746,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 	return 0;
 }
 
+static bool is_kfunc_disabled(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id)
+{
+	if (btf != btf_vmlinux)
+		return false;
+	if (!bpf_global_ma_set && func_id == special_kfunc_list[KF_bpf_obj_new_impl]) {
+		verbose(env, "bpf_global_ma initialization failed, can't call bpf_obj_new_impl\n");
+		return true;
+	}
+	return false;
+}
+
 static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			    int *insn_idx_p)
 {
@@ -8773,7 +8784,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	func_proto = btf_type_by_id(desc_btf, func->type);
 
 	kfunc_flags = btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog), func_id);
-	if (!kfunc_flags) {
+	if (!kfunc_flags || is_kfunc_disabled(env, desc_btf, func_id)) {
 		verbose(env, "calling kernel function %s is not allowed\n",
 			func_name);
 		return -EACCES;
-- 
2.38.1

[PATCH bpf-next v1 2/2] selftests/bpf: Skip spin lock failure test on s390x

[Kumar Kartikeya Dwivedi]

Instead of adding the whole test to DENYLIST.s390x, which also has
success test cases that should be run, just skip over failure test
cases in case the JIT does not support kfuncs.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/spin_lock.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/spin_lock.c b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
index 72282e92a78a..d9270bd3d920 100644
--- a/tools/testing/selftests/bpf/prog_tests/spin_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spin_lock.c
@@ -68,6 +68,12 @@ static void test_spin_lock_fail_prog(const char *prog_name, const char *err_msg)
 	if (!ASSERT_ERR(ret, "test_spin_lock_fail__load must fail"))
 		goto end;
 
+	/* Skip check if JIT does not support kfuncs */
+	if (strstr(log_buf, "JIT does not support calling kernel function")) {
+		test__skip();
+		goto end;
+	}
+
 	if (!ASSERT_OK_PTR(strstr(log_buf, err_msg), "expected error message")) {
 		fprintf(stderr, "Expected: %s\n", err_msg);
 		fprintf(stderr, "Verifier: %s\n", log_buf);
-- 
2.38.1

Re: [PATCH bpf-next v1 1/2] bpf: Disallow calling bpf_obj_new_impl on bpf_mem_alloc_init failure

[Alexei Starovoitov]

On Fri, Nov 18, 2022 at 11:00 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Instead of checking bpf_global_ma_set at runtime on each allocation
> inside bpf_obj_new_impl, simply disallow calling the kfunc in case
> bpf_global_ma initialization failed during program verification.
>
> The error generated when bpf_global_ma initialization fails:
> ...
> 21: (18) r1 = 0x7                     ; R1_w=7
> 23: (b7) r2 = 0                       ; R2_w=0
> 24: (85) call bpf_obj_new_impl#36585
> bpf_global_ma initialization failed, can't call bpf_obj_new_impl
> calling kernel function bpf_obj_new_impl is not allowed
>
> Suggested-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  kernel/bpf/helpers.c  |  2 --
>  kernel/bpf/verifier.c | 13 ++++++++++++-
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 212e791d7452..bc02f55adc1f 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1760,8 +1760,6 @@ void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign)
>         u64 size = local_type_id__k;
>         void *p;
>
> -       if (unlikely(!bpf_global_ma_set))
> -               return NULL;
>         p = bpf_mem_alloc(&bpf_global_ma, size);
>         if (!p)
>                 return NULL;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 195d24316750..f04bee7934a8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8746,6 +8746,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>         return 0;
>  }
>
> +static bool is_kfunc_disabled(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id)
> +{
> +       if (btf != btf_vmlinux)
> +               return false;
> +       if (!bpf_global_ma_set && func_id == special_kfunc_list[KF_bpf_obj_new_impl]) {
> +               verbose(env, "bpf_global_ma initialization failed, can't call bpf_obj_new_impl\n");
> +               return true;
> +       }
> +       return false;
> +}
> +

This is all just unnecessary code bloat for the case
that cannot happen.

When you do:
meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl]
just add
if (!bpf_global_ma_set)
 return -ENOMEM;

No need for verbose(). The users will never hit it.

Also please get rid of special_kfunc_set and
and btf_id_set_contains(&special_kfunc_set, meta.func_id)
That additional check is unnecessary as well.
special_kfunc_list is enough.
I'm going to apply patch 2 to make CI green.

Re: [PATCH bpf-next v1 0/2] Follow ups for bpf-list set

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Sat, 19 Nov 2022 00:29:36 +0530 you wrote:
> Make a few changes
>  - Remove bpf_global_ma_set check at runtime, disallow calling bpf_obj_new during verification
>  - Disable spin lock failure test when JIT does not support kfunc (s390x)
> 
> Kumar Kartikeya Dwivedi (2):
>   bpf: Disallow calling bpf_obj_new_impl on bpf_mem_alloc_init failure
>   selftests/bpf: Skip spin lock failure test on s390x
> 
> [...]

Here is the summary with links:
  - [bpf-next,v1,1/2] bpf: Disallow calling bpf_obj_new_impl on bpf_mem_alloc_init failure
    (no matching commit)
  - [bpf-next,v1,2/2] selftests/bpf: Skip spin lock failure test on s390x
    https://git.kernel.org/bpf/bpf-next/c/97c11d6e3154

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next v1 1/2] bpf: Disallow calling bpf_obj_new_impl on bpf_mem_alloc_init failure

[Kumar Kartikeya Dwivedi]

On Sat, Nov 19, 2022 at 01:47:25AM IST, Alexei Starovoitov wrote:
> On Fri, Nov 18, 2022 at 11:00 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Instead of checking bpf_global_ma_set at runtime on each allocation
> > inside bpf_obj_new_impl, simply disallow calling the kfunc in case
> > bpf_global_ma initialization failed during program verification.
> >
> > The error generated when bpf_global_ma initialization fails:
> > ...
> > 21: (18) r1 = 0x7                     ; R1_w=7
> > 23: (b7) r2 = 0                       ; R2_w=0
> > 24: (85) call bpf_obj_new_impl#36585
> > bpf_global_ma initialization failed, can't call bpf_obj_new_impl
> > calling kernel function bpf_obj_new_impl is not allowed
> >
> > Suggested-by: Alexei Starovoitov <ast@kernel.org>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  kernel/bpf/helpers.c  |  2 --
> >  kernel/bpf/verifier.c | 13 ++++++++++++-
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 212e791d7452..bc02f55adc1f 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1760,8 +1760,6 @@ void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign)
> >         u64 size = local_type_id__k;
> >         void *p;
> >
> > -       if (unlikely(!bpf_global_ma_set))
> > -               return NULL;
> >         p = bpf_mem_alloc(&bpf_global_ma, size);
> >         if (!p)
> >                 return NULL;
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 195d24316750..f04bee7934a8 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -8746,6 +8746,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> >         return 0;
> >  }
> >
> > +static bool is_kfunc_disabled(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id)
> > +{
> > +       if (btf != btf_vmlinux)
> > +               return false;
> > +       if (!bpf_global_ma_set && func_id == special_kfunc_list[KF_bpf_obj_new_impl]) {
> > +               verbose(env, "bpf_global_ma initialization failed, can't call bpf_obj_new_impl\n");
> > +               return true;
> > +       }
> > +       return false;
> > +}
> > +
>
> This is all just unnecessary code bloat for the case
> that cannot happen.
>
> When you do:
> meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl]
> just add
> if (!bpf_global_ma_set)
>  return -ENOMEM;
>
> No need for verbose(). The users will never hit it.
>

Ok, makes sense, will fix and resend.

> Also please get rid of special_kfunc_set and
> and btf_id_set_contains(&special_kfunc_set, meta.func_id)
> That additional check is unnecessary as well.
> special_kfunc_list is enough.

It provides an easy way to do 'btf == vmlinux && is_special_kfunc'.
Otherwise if I drop it, every check matching func_id == special_kfunc_list will
also have to do btf == vmlinux check (because eventually we want to drop to the
other branches that should work for non-vmlinux BTF as well).

What I mean is:
	if (btf == vmlinux && btf_id_set_contains(special_kfunc_set)) {
		if (func_id == special_kfunc_list) {
		} else if (func_id == special_kfunc_list) {
		} else {
		}
	} else if (!__btf_type_is_struct) {
	} else /* struct */ {
	}

will become:
	if (btf == vmlinux && func_id == special_kfunc_list) {
	} else if (btf == vmlinux && func_id == special_kfunc_list) {
	} else if (!__btf_type_is_struct) {
	} else /* struct */ {
	}

I think it's better to keep it. It also fails the kfunc call if the return type
is not properly handled (either custom implementation or just falling through to
default cases, w/e applies).

But up to you.

Re: [PATCH bpf-next v1 1/2] bpf: Disallow calling bpf_obj_new_impl on bpf_mem_alloc_init failure

[Alexei Starovoitov]

On Mon, Nov 21, 2022 at 02:16:25AM +0530, Kumar Kartikeya Dwivedi wrote:
> 
> > Also please get rid of special_kfunc_set and
> > and btf_id_set_contains(&special_kfunc_set, meta.func_id)
> > That additional check is unnecessary as well.
> > special_kfunc_list is enough.
> 
> It provides an easy way to do 'btf == vmlinux && is_special_kfunc'.
> Otherwise if I drop it, every check matching func_id == special_kfunc_list will
> also have to do btf == vmlinux check (because eventually we want to drop to the
> other branches that should work for non-vmlinux BTF as well).
> 
> What I mean is:
> 	if (btf == vmlinux && btf_id_set_contains(special_kfunc_set)) {
> 		if (func_id == special_kfunc_list) {
> 		} else if (func_id == special_kfunc_list) {
> 		} else {
> 		}
> 	} else if (!__btf_type_is_struct) {
> 	} else /* struct */ {
> 	}
> 
> will become:
> 	if (btf == vmlinux && func_id == special_kfunc_list) {
> 	} else if (btf == vmlinux && func_id == special_kfunc_list) {
> 	} else if (!__btf_type_is_struct) {
> 	} else /* struct */ {
> 	}

One less indent looks better.
Repeated btf == vmlinux doesn't bother me.
There is no use for this special_kfunc_set. It just wastes memory.
So I still think it's better to remove it.

While at it please swap branches !__btf_type_is_struct to __btf_type_is_struct:
  else if (__btf_type_is_struct) {
    regs[BPF_REG_0].type = PTR_TO_BTF_ID
  } else { // handle 'void *' return aka r0_size
would read better.