[bpf-next,2/3] bpf: mark bpf_cast_to_kern_ctx and bpf_rdonly_cast as KF_NOCSR

Commit Message

Eduard Zingerman Aug. 12, 2024, 11:43 p.m. UTC

do_misc_fixups() relaces bpf_cast_to_kern_ctx() and bpf_rdonly_cast()
by a single instruction "r0 = r1". This clearly follows nocsr contract.
Mark these two functions as KF_NOCSR, in order to use them in
selftests checking KF_NOCSR behaviour for kfuncs.

Signed-off-by: Eduard Zingerman <[e-mail]>
---
 kernel/bpf/helpers.c  | 4 ++--
 kernel/bpf/verifier.c | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

Comments

Andrii Nakryiko Aug. 15, 2024, 9:25 p.m. UTC | #1

On Mon, Aug 12, 2024 at 4:44 PM Eduard Zingerman <[e-mail]> wrote:
>
> do_misc_fixups() relaces bpf_cast_to_kern_ctx() and bpf_rdonly_cast()
> by a single instruction "r0 = r1". This clearly follows nocsr contract.
> Mark these two functions as KF_NOCSR, in order to use them in
> selftests checking KF_NOCSR behaviour for kfuncs.
>
> Signed-off-by: Eduard Zingerman <[e-mail]>
> ---
>  kernel/bpf/helpers.c  | 4 ++--
>  kernel/bpf/verifier.c | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)

Isn't it now "bpf fastcall" and not "nocsr"? Shouldn't the flag and
verifier code reflect this updated terminology?

>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index d02ae323996b..cda3c326eeb1 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2987,8 +2987,8 @@ BTF_ID(func, bpf_cgroup_release_dtor)
>  #endif
>
>  BTF_KFUNCS_START(common_btf_ids)
> -BTF_ID_FLAGS(func, bpf_cast_to_kern_ctx)
> -BTF_ID_FLAGS(func, bpf_rdonly_cast)
> +BTF_ID_FLAGS(func, bpf_cast_to_kern_ctx, KF_NOCSR)
> +BTF_ID_FLAGS(func, bpf_rdonly_cast, KF_NOCSR)
>  BTF_ID_FLAGS(func, bpf_rcu_read_lock)
>  BTF_ID_FLAGS(func, bpf_rcu_read_unlock)
>  BTF_ID_FLAGS(func, bpf_dynptr_slice, KF_RET_NULL)
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c579f74be3f9..88e583a37296 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16159,7 +16159,8 @@ static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta)
>  /* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */
>  static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta)
>  {
> -       return false;
> +       return meta->func_id == special_kfunc_list[KF_bpf_cast_to_kern_ctx] ||
> +              meta->func_id == special_kfunc_list[KF_bpf_rdonly_cast];
>  }
>
>  /* GCC and LLVM define a no_caller_saved_registers function attribute.
> --
> 2.45.2
>

Eduard Zingerman Aug. 15, 2024, 9:59 p.m. UTC | #2

On Thu, 2024-08-15 at 14:25 -0700, Andrii Nakryiko wrote:
> On Mon, Aug 12, 2024 at 4:44 PM Eduard Zingerman <[e-mail]> wrote:
> > 
> > do_misc_fixups() relaces bpf_cast_to_kern_ctx() and bpf_rdonly_cast()
> > by a single instruction "r0 = r1". This clearly follows nocsr contract.
> > Mark these two functions as KF_NOCSR, in order to use them in
> > selftests checking KF_NOCSR behaviour for kfuncs.
> > 
> > Signed-off-by: Eduard Zingerman <[e-mail]>
> > ---
> >  kernel/bpf/helpers.c  | 4 ++--
> >  kernel/bpf/verifier.c | 3 ++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> Isn't it now "bpf fastcall" and not "nocsr"? Shouldn't the flag and
> verifier code reflect this updated terminology?

Here is a pull request for LLVM that lands the feature under
the new bpf_fastcall name: https://github.com/llvm/llvm-project/pull/101228
I hope that it would be approved today or tomorrow (more like tomorrow).

Kernel side uses NOCSR in all places.
I can add a first patch to the series, renaming all NOCSR to bpf_fastcall,
now that it looks like llvm upstream won't object the name.

[...]

Andrii Nakryiko Aug. 15, 2024, 10:12 p.m. UTC | #3

On Thu, Aug 15, 2024 at 2:59 PM Eduard Zingerman <[e-mail]> wrote:
>
> On Thu, 2024-08-15 at 14:25 -0700, Andrii Nakryiko wrote:
> > On Mon, Aug 12, 2024 at 4:44 PM Eduard Zingerman <[e-mail]> wrote:
> > >
> > > do_misc_fixups() relaces bpf_cast_to_kern_ctx() and bpf_rdonly_cast()
> > > by a single instruction "r0 = r1". This clearly follows nocsr contract.
> > > Mark these two functions as KF_NOCSR, in order to use them in
> > > selftests checking KF_NOCSR behaviour for kfuncs.
> > >
> > > Signed-off-by: Eduard Zingerman <[e-mail]>
> > > ---
> > >  kernel/bpf/helpers.c  | 4 ++--
> > >  kernel/bpf/verifier.c | 3 ++-
> > >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > Isn't it now "bpf fastcall" and not "nocsr"? Shouldn't the flag and
> > verifier code reflect this updated terminology?
>
> Here is a pull request for LLVM that lands the feature under
> the new bpf_fastcall name: https://github.com/llvm/llvm-project/pull/101228
> I hope that it would be approved today or tomorrow (more like tomorrow).
>
> Kernel side uses NOCSR in all places.
> I can add a first patch to the series, renaming all NOCSR to bpf_fastcall,
> now that it looks like llvm upstream won't object the name.

Yep, I'd do that. Let's keep terminology consistent throughout.

I assume you'll also eventually follow up with bpf_helpers_defs.h
(there is a script that generates it) change to add that bpf_fastcall
attribute for select helpers, right?

>
> [...]
>

Eduard Zingerman Aug. 15, 2024, 10:14 p.m. UTC | #4

On Thu, 2024-08-15 at 15:12 -0700, Andrii Nakryiko wrote:

[...]

> > Here is a pull request for LLVM that lands the feature under
> > the new bpf_fastcall name: https://github.com/llvm/llvm-project/pull/101228
> > I hope that it would be approved today or tomorrow (more like tomorrow).
> > 
> > Kernel side uses NOCSR in all places.
> > I can add a first patch to the series, renaming all NOCSR to bpf_fastcall,
> > now that it looks like llvm upstream won't object the name.
> 
> Yep, I'd do that. Let's keep terminology consistent throughout.

Ok, will do, thank you.

> I assume you'll also eventually follow up with bpf_helpers_defs.h
> (there is a script that generates it) change to add that bpf_fastcall
> attribute for select helpers, right?

Good point.

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index d02ae323996b..cda3c326eeb1 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2987,8 +2987,8 @@  BTF_ID(func, bpf_cgroup_release_dtor)
 #endif
 
 BTF_KFUNCS_START(common_btf_ids)
-BTF_ID_FLAGS(func, bpf_cast_to_kern_ctx)
-BTF_ID_FLAGS(func, bpf_rdonly_cast)
+BTF_ID_FLAGS(func, bpf_cast_to_kern_ctx, KF_NOCSR)
+BTF_ID_FLAGS(func, bpf_rdonly_cast, KF_NOCSR)
 BTF_ID_FLAGS(func, bpf_rcu_read_lock)
 BTF_ID_FLAGS(func, bpf_rcu_read_unlock)
 BTF_ID_FLAGS(func, bpf_dynptr_slice, KF_RET_NULL)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c579f74be3f9..88e583a37296 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -16159,7 +16159,8 @@  static u32 kfunc_nocsr_clobber_mask(struct bpf_kfunc_call_arg_meta *meta)
 /* Same as verifier_inlines_helper_call() but for kfuncs, see comment above */
 static bool verifier_inlines_kfunc_call(struct bpf_kfunc_call_arg_meta *meta)
 {
-	return false;
+	return meta->func_id == special_kfunc_list[KF_bpf_cast_to_kern_ctx] ||
+	       meta->func_id == special_kfunc_list[KF_bpf_rdonly_cast];
 }
 
 /* GCC and LLVM define a no_caller_saved_registers function attribute.