[v2,bpf-next,2/2] bpf: Add __bpf_hook_{start,end} macros

Commit Message

Dave Marchevsky Oct. 31, 2023, 9:56 p.m. UTC

Not all uses of __diag_ignore_all(...) in BPF-related code in order to
suppress warnings are wrapping kfunc definitions. Some "hook point"
definitions - small functions meant to be used as attach points for
fentry and similar BPF progs - need to suppress -Wmissing-declarations.

We could use __bpf_kfunc_{start,end}_defs added in the previous patch in
such cases, but this might be confusing to someone unfamiliar with BPF
internals. Instead, this patch adds __bpf_hook_{start,end} macros,
currently having the same effect as __bpf_kfunc_{start,end}_defs, then
uses them to suppress warnings for two hook points in the kernel itself
and some bpf_testmod hook points as well.

Signed-off-by: Dave Marchevsky <[e-mail]>
Cc: Yafang Shao <[e-mail]>
---

This patch was added in v2 in response to convo on v1's thread.

 include/linux/btf.h                                   | 2 ++
 kernel/cgroup/rstat.c                                 | 9 +++------
 net/socket.c                                          | 8 ++------
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 6 ++----
 4 files changed, 9 insertions(+), 16 deletions(-)

Comments

Jiri Olsa Oct. 31, 2023, 11:03 p.m. UTC | #1

On Tue, Oct 31, 2023 at 02:56:25PM -0700, Dave Marchevsky wrote:
> Not all uses of __diag_ignore_all(...) in BPF-related code in order to
> suppress warnings are wrapping kfunc definitions. Some "hook point"
> definitions - small functions meant to be used as attach points for
> fentry and similar BPF progs - need to suppress -Wmissing-declarations.
> 
> We could use __bpf_kfunc_{start,end}_defs added in the previous patch in
> such cases, but this might be confusing to someone unfamiliar with BPF
> internals. Instead, this patch adds __bpf_hook_{start,end} macros,
> currently having the same effect as __bpf_kfunc_{start,end}_defs, then
> uses them to suppress warnings for two hook points in the kernel itself
> and some bpf_testmod hook points as well.
> 
> Signed-off-by: Dave Marchevsky <[e-mail]>
> Cc: Yafang Shao <[e-mail]>

Acked-by: Jiri Olsa <[e-mail]>

jirka

> ---
> 
> This patch was added in v2 in response to convo on v1's thread.
> 
>  include/linux/btf.h                                   | 2 ++
>  kernel/cgroup/rstat.c                                 | 9 +++------
>  net/socket.c                                          | 8 ++------
>  tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 6 ++----
>  4 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index dc5ce962f600..59d404e22814 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -92,6 +92,8 @@
>  			  "Global kfuncs as their definitions will be in BTF")
>  
>  #define __bpf_kfunc_end_defs() __diag_pop()
> +#define __bpf_hook_start() __bpf_kfunc_start_defs()
> +#define __bpf_hook_end() __bpf_kfunc_end_defs()
>  
>  /*
>   * Return the name of the passed struct, if exists, or halt the build if for
> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> index d80d7a608141..c0adb7254b45 100644
> --- a/kernel/cgroup/rstat.c
> +++ b/kernel/cgroup/rstat.c
> @@ -156,19 +156,16 @@ static struct cgroup *cgroup_rstat_cpu_pop_updated(struct cgroup *pos,
>   * optimize away the callsite. Therefore, __weak is needed to ensure that the
>   * call is still emitted, by telling the compiler that we don't know what the
>   * function might eventually be.
> - *
> - * __diag_* below are needed to dismiss the missing prototype warning.
>   */
> -__diag_push();
> -__diag_ignore_all("-Wmissing-prototypes",
> -		  "kfuncs which will be used in BPF programs");
> +
> +__bpf_hook_start();
>  
>  __weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
>  				     struct cgroup *parent, int cpu)
>  {
>  }
>  
> -__diag_pop();
> +__bpf_hook_end();
>  
>  /* see cgroup_rstat_flush() */
>  static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
> diff --git a/net/socket.c b/net/socket.c
> index c4a6f5532955..cd4d9ae2144f 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1685,20 +1685,16 @@ struct file *__sys_socket_file(int family, int type, int protocol)
>   *	Therefore, __weak is needed to ensure that the call is still
>   *	emitted, by telling the compiler that we don't know what the
>   *	function might eventually be.
> - *
> - *	__diag_* below are needed to dismiss the missing prototype warning.
>   */
>  
> -__diag_push();
> -__diag_ignore_all("-Wmissing-prototypes",
> -		  "A fmod_ret entry point for BPF programs");
> +__bpf_hook_start();
>  
>  __weak noinline int update_socket_protocol(int family, int type, int protocol)
>  {
>  	return protocol;
>  }
>  
> -__diag_pop();
> +__bpf_hook_end();
>  
>  int __sys_socket(int family, int type, int protocol)
>  {
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index a5e246f7b202..91907b321f91 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -39,9 +39,7 @@ struct bpf_testmod_struct_arg_4 {
>  	int b;
>  };
>  
> -__diag_push();
> -__diag_ignore_all("-Wmissing-prototypes",
> -		  "Global functions as their definitions will be in bpf_testmod.ko BTF");
> +__bpf_hook_start();
>  
>  noinline int
>  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int b, int c) {
> @@ -335,7 +333,7 @@ noinline int bpf_fentry_shadow_test(int a)
>  }
>  EXPORT_SYMBOL_GPL(bpf_fentry_shadow_test);
>  
> -__diag_pop();
> +__bpf_hook_end();
>  
>  static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
>  	.attr = { .name = "bpf_testmod", .mode = 0666, },
> -- 
> 2.34.1
> 
>

Yafang Shao Nov. 1, 2023, 2:17 a.m. UTC | #2

On Wed, Nov 1, 2023 at 7:03 AM Jiri Olsa <[e-mail]> wrote:
>
> On Tue, Oct 31, 2023 at 02:56:25PM -0700, Dave Marchevsky wrote:
> > Not all uses of __diag_ignore_all(...) in BPF-related code in order to
> > suppress warnings are wrapping kfunc definitions. Some "hook point"
> > definitions - small functions meant to be used as attach points for
> > fentry and similar BPF progs - need to suppress -Wmissing-declarations.
> >
> > We could use __bpf_kfunc_{start,end}_defs added in the previous patch in
> > such cases, but this might be confusing to someone unfamiliar with BPF
> > internals. Instead, this patch adds __bpf_hook_{start,end} macros,
> > currently having the same effect as __bpf_kfunc_{start,end}_defs, then
> > uses them to suppress warnings for two hook points in the kernel itself
> > and some bpf_testmod hook points as well.
> >
> > Signed-off-by: Dave Marchevsky <[e-mail]>
> > Cc: Yafang Shao <[e-mail]>
>
> Acked-by: Jiri Olsa <[e-mail]>

Acked-by: Yafang Shao <[e-mail]>

>
> jirka
>
> > ---
> >
> > This patch was added in v2 in response to convo on v1's thread.
> >
> >  include/linux/btf.h                                   | 2 ++
> >  kernel/cgroup/rstat.c                                 | 9 +++------
> >  net/socket.c                                          | 8 ++------
> >  tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 6 ++----
> >  4 files changed, 9 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index dc5ce962f600..59d404e22814 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -92,6 +92,8 @@
> >                         "Global kfuncs as their definitions will be in BTF")
> >
> >  #define __bpf_kfunc_end_defs() __diag_pop()
> > +#define __bpf_hook_start() __bpf_kfunc_start_defs()
> > +#define __bpf_hook_end() __bpf_kfunc_end_defs()
> >
> >  /*
> >   * Return the name of the passed struct, if exists, or halt the build if for
> > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> > index d80d7a608141..c0adb7254b45 100644
> > --- a/kernel/cgroup/rstat.c
> > +++ b/kernel/cgroup/rstat.c
> > @@ -156,19 +156,16 @@ static struct cgroup *cgroup_rstat_cpu_pop_updated(struct cgroup *pos,
> >   * optimize away the callsite. Therefore, __weak is needed to ensure that the
> >   * call is still emitted, by telling the compiler that we don't know what the
> >   * function might eventually be.
> > - *
> > - * __diag_* below are needed to dismiss the missing prototype warning.
> >   */
> > -__diag_push();
> > -__diag_ignore_all("-Wmissing-prototypes",
> > -               "kfuncs which will be used in BPF programs");
> > +
> > +__bpf_hook_start();
> >
> >  __weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
> >                                    struct cgroup *parent, int cpu)
> >  {
> >  }
> >
> > -__diag_pop();
> > +__bpf_hook_end();
> >
> >  /* see cgroup_rstat_flush() */
> >  static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
> > diff --git a/net/socket.c b/net/socket.c
> > index c4a6f5532955..cd4d9ae2144f 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1685,20 +1685,16 @@ struct file *__sys_socket_file(int family, int type, int protocol)
> >   *   Therefore, __weak is needed to ensure that the call is still
> >   *   emitted, by telling the compiler that we don't know what the
> >   *   function might eventually be.
> > - *
> > - *   __diag_* below are needed to dismiss the missing prototype warning.
> >   */
> >
> > -__diag_push();
> > -__diag_ignore_all("-Wmissing-prototypes",
> > -               "A fmod_ret entry point for BPF programs");
> > +__bpf_hook_start();
> >
> >  __weak noinline int update_socket_protocol(int family, int type, int protocol)
> >  {
> >       return protocol;
> >  }
> >
> > -__diag_pop();
> > +__bpf_hook_end();
> >
> >  int __sys_socket(int family, int type, int protocol)
> >  {
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index a5e246f7b202..91907b321f91 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -39,9 +39,7 @@ struct bpf_testmod_struct_arg_4 {
> >       int b;
> >  };
> >
> > -__diag_push();
> > -__diag_ignore_all("-Wmissing-prototypes",
> > -               "Global functions as their definitions will be in bpf_testmod.ko BTF");
> > +__bpf_hook_start();
> >
> >  noinline int
> >  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int b, int c) {
> > @@ -335,7 +333,7 @@ noinline int bpf_fentry_shadow_test(int a)
> >  }
> >  EXPORT_SYMBOL_GPL(bpf_fentry_shadow_test);
> >
> > -__diag_pop();
> > +__bpf_hook_end();
> >
> >  static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
> >       .attr = { .name = "bpf_testmod", .mode = 0666, },
> > --
> > 2.34.1
> >
> >

Patch

diff --git a/include/linux/btf.h b/include/linux/btf.h
index dc5ce962f600..59d404e22814 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -92,6 +92,8 @@ 
 			  "Global kfuncs as their definitions will be in BTF")
 
 #define __bpf_kfunc_end_defs() __diag_pop()
+#define __bpf_hook_start() __bpf_kfunc_start_defs()
+#define __bpf_hook_end() __bpf_kfunc_end_defs()
 
 /*
  * Return the name of the passed struct, if exists, or halt the build if for
diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index d80d7a608141..c0adb7254b45 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -156,19 +156,16 @@  static struct cgroup *cgroup_rstat_cpu_pop_updated(struct cgroup *pos,
  * optimize away the callsite. Therefore, __weak is needed to ensure that the
  * call is still emitted, by telling the compiler that we don't know what the
  * function might eventually be.
- *
- * __diag_* below are needed to dismiss the missing prototype warning.
  */
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "kfuncs which will be used in BPF programs");
+
+__bpf_hook_start();
 
 __weak noinline void bpf_rstat_flush(struct cgroup *cgrp,
 				     struct cgroup *parent, int cpu)
 {
 }
 
-__diag_pop();
+__bpf_hook_end();
 
 /* see cgroup_rstat_flush() */
 static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
diff --git a/net/socket.c b/net/socket.c
index c4a6f5532955..cd4d9ae2144f 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1685,20 +1685,16 @@  struct file *__sys_socket_file(int family, int type, int protocol)
  *	Therefore, __weak is needed to ensure that the call is still
  *	emitted, by telling the compiler that we don't know what the
  *	function might eventually be.
- *
- *	__diag_* below are needed to dismiss the missing prototype warning.
  */
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "A fmod_ret entry point for BPF programs");
+__bpf_hook_start();
 
 __weak noinline int update_socket_protocol(int family, int type, int protocol)
 {
 	return protocol;
 }
 
-__diag_pop();
+__bpf_hook_end();
 
 int __sys_socket(int family, int type, int protocol)
 {
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index a5e246f7b202..91907b321f91 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -39,9 +39,7 @@  struct bpf_testmod_struct_arg_4 {
 	int b;
 };
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in bpf_testmod.ko BTF");
+__bpf_hook_start();
 
 noinline int
 bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int b, int c) {
@@ -335,7 +333,7 @@  noinline int bpf_fentry_shadow_test(int a)
 }
 EXPORT_SYMBOL_GPL(bpf_fentry_shadow_test);
 
-__diag_pop();
+__bpf_hook_end();
 
 static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
 	.attr = { .name = "bpf_testmod", .mode = 0666, },