On 8/17/23 6:01 AM, David Vernet wrote:
On Wed, Aug 16, 2023 at 08:48:16PM -0700, Alexei Starovoitov wrote:
On Wed, Aug 16, 2023 at 8:38 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
On 8/16/23 8:06 AM, David Vernet wrote:
We recently got an lkp warning about missing declarations, as in e.g.
[0]. This warning is largely redundant with -Wmissing-prototypes, which
we already disable for kfuncs that have global linkage and are meant to
be exported in BTF, and called from BPF programs. Let's also disable
-Wmissing-declarations for kfuncs. For what it's worth, I wasn't able to
reproduce the warning even on W <= 3, so I can't actually be 100% sure
this fixes the issue.
[0]: https://lore.kernel.org/all/202308162115.Hn23vv3n-lkp@xxxxxxxxx/
Okay, I just got a similar email to [0] which complains
bpf_obj_new_impl, ..., bpf_cast_to_kern_ctx
missing declarations.
In the email, the used compiler is
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
Unfortunately, I did not have gcc-7 to verify this.
Also, what is the minimum gcc version kernel supports? 5.1?
pahole and BTF might be broken in such old GCC too.
Maybe we should add:
config BPF_SYSCALL
depends on GCC_VERSION >= 90000 || CLANG_VERSION >= 130000
It seems prudent to formally declare minimum compiler versions. Though
modern gcc and clang also support -Wmissing-declarations, so maybe we
should merge this patch regardless? Just unfortunate to have to add even
more boilerplate just to get the compiler off our backs.
Urgh, to restrict BPF syscall with such `depends on` would be super ugly. Why
can't we just move this boilerplate behind a macro instead of copying this
everywhere? For example the below on top of your patch builds just fine on my
side:
diff --git a/include/linux/btf.h b/include/linux/btf.h
index df64cc642074..6a873a652001 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -83,6 +83,16 @@
*/
#define __bpf_kfunc __used noinline
+#define __bpf_kfunc_start \
+ __diag_push(); \
+ __diag_ignore_all("-Wmissing-prototypes", \
+ "Global functions as their definitions will be in vmlinux BTF"); \
+ __diag_ignore_all("-Wmissing-declarations", \
+ "Global functions as their definitions will be in vmlinux BTF");
+
+#define __bpf_kfunc_end \
+ __diag_pop();
+
/*
* Return the name of the passed struct, if exists, or halt the build if for
* example the structure gets renamed. In this way, developers have to revisit
diff --git a/net/core/filter.c b/net/core/filter.c
index c2b32b94c6bd..08dd0dd710dd 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11724,11 +11724,7 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id)
return func;
}
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
- "Global functions as their definitions will be in vmlinux BTF");
-__diag_ignore_all("-Wmissing-declarations",
- "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start
__bpf_kfunc int bpf_dynptr_from_skb(struct sk_buff *skb, u64 flags,
struct bpf_dynptr_kern *ptr__uninit)
{
@@ -11754,7 +11750,7 @@ __bpf_kfunc int bpf_dynptr_from_xdp(struct xdp_buff *xdp, u64 flags,
return 0;
}
-__diag_pop();
+__bpf_kfunc_end
int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
struct bpf_dynptr_kern *ptr__uninit)
Thanks,
Daniel
