diff mbox series

[v1,bpf-next] bpf: Add __bpf_kfunc_{start,end}_defs macros

Message ID 20231030210638.2415306-1-davemarchevsky@fb.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [v1,bpf-next] bpf: Add __bpf_kfunc_{start,end}_defs macros | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 fail Logs for x86_64-llvm-16 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-llvm-16 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-16 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-llvm-16 / build / build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-9 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-16 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-16 / veristat
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2868 this patch: 2868
netdev/cc_maintainers warning 25 maintainers not CCed: hawk@kernel.org herbert@gondor.apana.org.au pabeni@redhat.com pablo@netfilter.org kadlec@netfilter.org sdf@google.com kuba@kernel.org kpsingh@kernel.org song@kernel.org haoluo@google.com netfilter-devel@vger.kernel.org coreteam@netfilter.org netdev@vger.kernel.org rostedt@goodmis.org martin.lau@linux.dev fw@strlen.de jolsa@kernel.org linux-trace-kernel@vger.kernel.org john.fastabend@gmail.com mhiramat@kernel.org davem@davemloft.net dsahern@kernel.org yonghong.song@linux.dev edumazet@google.com steffen.klassert@secunet.com
netdev/build_clang success Errors and warnings before: 1541 this patch: 1541
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2945 this patch: 2945
netdev/checkpatch fail CHECK: Please use a blank line after function/struct/union/enum declarations ERROR: Macros with multiple statements should be enclosed in a do - while loop
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-10 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Dave Marchevsky Oct. 30, 2023, 9:06 p.m. UTC
BPF kfuncs are meant to be called from BPF programs. Accordingly, most
kfuncs are not called from anywhere in the kernel, which the
-Wmissing-prototypes warning is unhappy about. We've peppered
__diag_ignore_all("-Wmissing-prototypes", ... everywhere kfuncs are
defined in the codebase to suppress this warning.

This patch adds two macros meant to bound one or many kfunc definitions.
All existing kfunc definitions which use these __diag calls to suppress
-Wmissing-prototypes are migrated to use the newly-introduced macros.
A new __diag_ignore_all - for "-Wmissing-declarations" - is added to the
__bpf_kfunc_start_defs macro based on feedback from Andrii on an earlier
version of this patch [0] and another recent mailing list thread [1].

In the future we might need to ignore different warnings or do other
kfunc-specific things. This change will make it easier to make such
modifications for all kfunc defs.

  [0]: https://lore.kernel.org/bpf/CAEf4BzaE5dRWtK6RPLnjTW-MW9sx9K3Fn6uwqCTChK2Dcb1Xig@mail.gmail.com/
  [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Cc: Jiri Olsa <olsajiri@gmail.com>
---

This patch was submitted earlier as part of task_vma
iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@fb.com/

This separate submission addresses Andrii's comments from
that thread.

 include/linux/btf.h              |  9 +++++++++
 kernel/bpf/bpf_iter.c            |  6 ++----
 kernel/bpf/cpumask.c             |  6 ++----
 kernel/bpf/helpers.c             |  6 ++----
 kernel/bpf/map_iter.c            |  6 ++----
 kernel/bpf/task_iter.c           |  6 ++----
 kernel/trace/bpf_trace.c         |  6 ++----
 net/bpf/test_run.c               |  7 +++----
 net/core/filter.c                | 13 ++++---------
 net/core/xdp.c                   |  6 ++----
 net/ipv4/fou_bpf.c               |  6 ++----
 net/netfilter/nf_conntrack_bpf.c |  6 ++----
 net/netfilter/nf_nat_bpf.c       |  6 ++----
 net/xfrm/xfrm_interface_bpf.c    |  6 ++----
 14 files changed, 38 insertions(+), 57 deletions(-)

Comments

Yafang Shao Oct. 31, 2023, 5:55 a.m. UTC | #1
On Tue, Oct 31, 2023 at 5:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> BPF kfuncs are meant to be called from BPF programs. Accordingly, most
> kfuncs are not called from anywhere in the kernel, which the
> -Wmissing-prototypes warning is unhappy about. We've peppered
> __diag_ignore_all("-Wmissing-prototypes", ... everywhere kfuncs are
> defined in the codebase to suppress this warning.
>
> This patch adds two macros meant to bound one or many kfunc definitions.
> All existing kfunc definitions which use these __diag calls to suppress
> -Wmissing-prototypes are migrated to use the newly-introduced macros.
> A new __diag_ignore_all - for "-Wmissing-declarations" - is added to the
> __bpf_kfunc_start_defs macro based on feedback from Andrii on an earlier
> version of this patch [0] and another recent mailing list thread [1].
>
> In the future we might need to ignore different warnings or do other
> kfunc-specific things. This change will make it easier to make such
> modifications for all kfunc defs.
>
>   [0]: https://lore.kernel.org/bpf/CAEf4BzaE5dRWtK6RPLnjTW-MW9sx9K3Fn6uwqCTChK2Dcb1Xig@mail.gmail.com/
>   [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Cc: Jiri Olsa <olsajiri@gmail.com>
> ---
>
> This patch was submitted earlier as part of task_vma
> iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@fb.com/
>
> This separate submission addresses Andrii's comments from
> that thread.
>
>  include/linux/btf.h              |  9 +++++++++
>  kernel/bpf/bpf_iter.c            |  6 ++----
>  kernel/bpf/cpumask.c             |  6 ++----
>  kernel/bpf/helpers.c             |  6 ++----
>  kernel/bpf/map_iter.c            |  6 ++----
>  kernel/bpf/task_iter.c           |  6 ++----
>  kernel/trace/bpf_trace.c         |  6 ++----
>  net/bpf/test_run.c               |  7 +++----
>  net/core/filter.c                | 13 ++++---------
>  net/core/xdp.c                   |  6 ++----
>  net/ipv4/fou_bpf.c               |  6 ++----
>  net/netfilter/nf_conntrack_bpf.c |  6 ++----
>  net/netfilter/nf_nat_bpf.c       |  6 ++----
>  net/xfrm/xfrm_interface_bpf.c    |  6 ++----
>  14 files changed, 38 insertions(+), 57 deletions(-)
>

Thanks for your work.

By using a simple grep for "__diag_ignore_all(\"-Wmissing-prototypes",
it appears that the files net/socket.c,
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c,
kernel/cgroup/rstat.c and Documentation/bpf/kfuncs.rst are missing. It
seems that we should also update them.
Andrii Nakryiko Oct. 31, 2023, 6:23 a.m. UTC | #2
On Mon, Oct 30, 2023 at 10:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Tue, Oct 31, 2023 at 5:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> >
> > BPF kfuncs are meant to be called from BPF programs. Accordingly, most
> > kfuncs are not called from anywhere in the kernel, which the
> > -Wmissing-prototypes warning is unhappy about. We've peppered
> > __diag_ignore_all("-Wmissing-prototypes", ... everywhere kfuncs are
> > defined in the codebase to suppress this warning.
> >
> > This patch adds two macros meant to bound one or many kfunc definitions.
> > All existing kfunc definitions which use these __diag calls to suppress
> > -Wmissing-prototypes are migrated to use the newly-introduced macros.
> > A new __diag_ignore_all - for "-Wmissing-declarations" - is added to the
> > __bpf_kfunc_start_defs macro based on feedback from Andrii on an earlier
> > version of this patch [0] and another recent mailing list thread [1].
> >
> > In the future we might need to ignore different warnings or do other
> > kfunc-specific things. This change will make it easier to make such
> > modifications for all kfunc defs.
> >
> >   [0]: https://lore.kernel.org/bpf/CAEf4BzaE5dRWtK6RPLnjTW-MW9sx9K3Fn6uwqCTChK2Dcb1Xig@mail.gmail.com/
> >   [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/
> >
> > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Jiri Olsa <olsajiri@gmail.com>
> > ---
> >
> > This patch was submitted earlier as part of task_vma
> > iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@fb.com/
> >
> > This separate submission addresses Andrii's comments from
> > that thread.
> >
> >  include/linux/btf.h              |  9 +++++++++
> >  kernel/bpf/bpf_iter.c            |  6 ++----
> >  kernel/bpf/cpumask.c             |  6 ++----
> >  kernel/bpf/helpers.c             |  6 ++----
> >  kernel/bpf/map_iter.c            |  6 ++----
> >  kernel/bpf/task_iter.c           |  6 ++----
> >  kernel/trace/bpf_trace.c         |  6 ++----
> >  net/bpf/test_run.c               |  7 +++----
> >  net/core/filter.c                | 13 ++++---------
> >  net/core/xdp.c                   |  6 ++----
> >  net/ipv4/fou_bpf.c               |  6 ++----
> >  net/netfilter/nf_conntrack_bpf.c |  6 ++----
> >  net/netfilter/nf_nat_bpf.c       |  6 ++----
> >  net/xfrm/xfrm_interface_bpf.c    |  6 ++----
> >  14 files changed, 38 insertions(+), 57 deletions(-)
> >
>
> Thanks for your work.
>
> By using a simple grep for "__diag_ignore_all(\"-Wmissing-prototypes",
> it appears that the files net/socket.c,
> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c,
> kernel/cgroup/rstat.c and Documentation/bpf/kfuncs.rst are missing. It
> seems that we should also update them.
>

rstat.c and net/socket.c don't have kfuncs, so those are not relevant
here. But we are missing changes also in kernel/bpf/task_iter.c and
kernel/bpf/cgroup_iter.c

And let's update Documentation/bpf/kfuncs.rst to use your new set of macros?

With the above addressed, please add my ack. Thanks!

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> --
> Regards
> Yafang
Yafang Shao Oct. 31, 2023, 6:51 a.m. UTC | #3
On Tue, Oct 31, 2023 at 2:23 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Oct 30, 2023 at 10:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Tue, Oct 31, 2023 at 5:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> > >
> > > BPF kfuncs are meant to be called from BPF programs. Accordingly, most
> > > kfuncs are not called from anywhere in the kernel, which the
> > > -Wmissing-prototypes warning is unhappy about. We've peppered
> > > __diag_ignore_all("-Wmissing-prototypes", ... everywhere kfuncs are
> > > defined in the codebase to suppress this warning.
> > >
> > > This patch adds two macros meant to bound one or many kfunc definitions.
> > > All existing kfunc definitions which use these __diag calls to suppress
> > > -Wmissing-prototypes are migrated to use the newly-introduced macros.
> > > A new __diag_ignore_all - for "-Wmissing-declarations" - is added to the
> > > __bpf_kfunc_start_defs macro based on feedback from Andrii on an earlier
> > > version of this patch [0] and another recent mailing list thread [1].
> > >
> > > In the future we might need to ignore different warnings or do other
> > > kfunc-specific things. This change will make it easier to make such
> > > modifications for all kfunc defs.
> > >
> > >   [0]: https://lore.kernel.org/bpf/CAEf4BzaE5dRWtK6RPLnjTW-MW9sx9K3Fn6uwqCTChK2Dcb1Xig@mail.gmail.com/
> > >   [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/
> > >
> > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > Cc: Jiri Olsa <olsajiri@gmail.com>
> > > ---
> > >
> > > This patch was submitted earlier as part of task_vma
> > > iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@fb.com/
> > >
> > > This separate submission addresses Andrii's comments from
> > > that thread.
> > >
> > >  include/linux/btf.h              |  9 +++++++++
> > >  kernel/bpf/bpf_iter.c            |  6 ++----
> > >  kernel/bpf/cpumask.c             |  6 ++----
> > >  kernel/bpf/helpers.c             |  6 ++----
> > >  kernel/bpf/map_iter.c            |  6 ++----
> > >  kernel/bpf/task_iter.c           |  6 ++----
> > >  kernel/trace/bpf_trace.c         |  6 ++----
> > >  net/bpf/test_run.c               |  7 +++----
> > >  net/core/filter.c                | 13 ++++---------
> > >  net/core/xdp.c                   |  6 ++----
> > >  net/ipv4/fou_bpf.c               |  6 ++----
> > >  net/netfilter/nf_conntrack_bpf.c |  6 ++----
> > >  net/netfilter/nf_nat_bpf.c       |  6 ++----
> > >  net/xfrm/xfrm_interface_bpf.c    |  6 ++----
> > >  14 files changed, 38 insertions(+), 57 deletions(-)
> > >
> >
> > Thanks for your work.
> >
> > By using a simple grep for "__diag_ignore_all(\"-Wmissing-prototypes",
> > it appears that the files net/socket.c,
> > tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c,
> > kernel/cgroup/rstat.c and Documentation/bpf/kfuncs.rst are missing. It
> > seems that we should also update them.
> >
>
> rstat.c and net/socket.c don't have kfuncs, so those are not relevant
> here.

The bpf_rstat_flush() and update_socket_protocol() can also trigger
the -Wmissing-declarations.
These two functions are for BPF only. Shouldn't we better include them as well ?

> But we are missing changes also in kernel/bpf/task_iter.c and
> kernel/bpf/cgroup_iter.c
>
> And let's update Documentation/bpf/kfuncs.rst to use your new set of macros?
>
> With the above addressed, please add my ack. Thanks!
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> > --
> > Regards
> > Yafang
David Marchevsky Oct. 31, 2023, 5:03 p.m. UTC | #4
On 10/31/23 2:51 AM, Yafang Shao wrote:
> On Tue, Oct 31, 2023 at 2:23 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Mon, Oct 30, 2023 at 10:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>>>
>>> On Tue, Oct 31, 2023 at 5:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>>>>
>>>> BPF kfuncs are meant to be called from BPF programs. Accordingly, most
>>>> kfuncs are not called from anywhere in the kernel, which the
>>>> -Wmissing-prototypes warning is unhappy about. We've peppered
>>>> __diag_ignore_all("-Wmissing-prototypes", ... everywhere kfuncs are
>>>> defined in the codebase to suppress this warning.
>>>>
>>>> This patch adds two macros meant to bound one or many kfunc definitions.
>>>> All existing kfunc definitions which use these __diag calls to suppress
>>>> -Wmissing-prototypes are migrated to use the newly-introduced macros.
>>>> A new __diag_ignore_all - for "-Wmissing-declarations" - is added to the
>>>> __bpf_kfunc_start_defs macro based on feedback from Andrii on an earlier
>>>> version of this patch [0] and another recent mailing list thread [1].
>>>>
>>>> In the future we might need to ignore different warnings or do other
>>>> kfunc-specific things. This change will make it easier to make such
>>>> modifications for all kfunc defs.
>>>>
>>>>   [0]: https://lore.kernel.org/bpf/CAEf4BzaE5dRWtK6RPLnjTW-MW9sx9K3Fn6uwqCTChK2Dcb1Xig@mail.gmail.com/
>>>>   [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/
>>>>
>>>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>>>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>>>> Cc: Jiri Olsa <olsajiri@gmail.com>
>>>> ---
>>>>
>>>> This patch was submitted earlier as part of task_vma
>>>> iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@fb.com/
>>>>
>>>> This separate submission addresses Andrii's comments from
>>>> that thread.
>>>>
>>>>  include/linux/btf.h              |  9 +++++++++
>>>>  kernel/bpf/bpf_iter.c            |  6 ++----
>>>>  kernel/bpf/cpumask.c             |  6 ++----
>>>>  kernel/bpf/helpers.c             |  6 ++----
>>>>  kernel/bpf/map_iter.c            |  6 ++----
>>>>  kernel/bpf/task_iter.c           |  6 ++----
>>>>  kernel/trace/bpf_trace.c         |  6 ++----
>>>>  net/bpf/test_run.c               |  7 +++----
>>>>  net/core/filter.c                | 13 ++++---------
>>>>  net/core/xdp.c                   |  6 ++----
>>>>  net/ipv4/fou_bpf.c               |  6 ++----
>>>>  net/netfilter/nf_conntrack_bpf.c |  6 ++----
>>>>  net/netfilter/nf_nat_bpf.c       |  6 ++----
>>>>  net/xfrm/xfrm_interface_bpf.c    |  6 ++----
>>>>  14 files changed, 38 insertions(+), 57 deletions(-)
>>>>
>>>
>>> Thanks for your work.
>>>
>>> By using a simple grep for "__diag_ignore_all(\"-Wmissing-prototypes",
>>> it appears that the files net/socket.c,
>>> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c,
>>> kernel/cgroup/rstat.c and Documentation/bpf/kfuncs.rst are missing. It
>>> seems that we should also update them.
>>>
>>
>> rstat.c and net/socket.c don't have kfuncs, so those are not relevant
>> here.
> 
> The bpf_rstat_flush() and update_socket_protocol() can also trigger
> the -Wmissing-declarations.
> These two functions are for BPF only. Shouldn't we better include them as well ?
> 

I had this conundrum when writing the patch as well. Since they're not kfuncs
and the macros are meant to wrap kfunc definitions, I felt that it would be
confusing to someone unfamiliar with BPF internals. But I agree that the current
state isn't ideal either.

How about either:
  * I use the __bpf_kfunc_{start,end}_defs macros in those two places,
    with comment describing that they're not wrapping kfunc def, but rather
    BPF hook point that throws the same warnings.
  * Two additional macros, __bpf_hook_{start,end} are added, just
    pointing to __bpf_kfunc_{start,end} for now. They're used for
    these two functions

WDYT? 

>> But we are missing changes also in kernel/bpf/task_iter.c and
>> kernel/bpf/cgroup_iter.c
>>
>> And let's update Documentation/bpf/kfuncs.rst to use your new set of macros?
>>
>> With the above addressed, please add my ack. Thanks!
>>
>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>>
>>> --
>>> Regards
>>> Yafang
> 
> 
>
Andrii Nakryiko Oct. 31, 2023, 6:23 p.m. UTC | #5
On Tue, Oct 31, 2023 at 10:04 AM David Marchevsky
<david.marchevsky@linux.dev> wrote:
>
> On 10/31/23 2:51 AM, Yafang Shao wrote:
> > On Tue, Oct 31, 2023 at 2:23 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Mon, Oct 30, 2023 at 10:56 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >>>
> >>> On Tue, Oct 31, 2023 at 5:07 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> >>>>
> >>>> BPF kfuncs are meant to be called from BPF programs. Accordingly, most
> >>>> kfuncs are not called from anywhere in the kernel, which the
> >>>> -Wmissing-prototypes warning is unhappy about. We've peppered
> >>>> __diag_ignore_all("-Wmissing-prototypes", ... everywhere kfuncs are
> >>>> defined in the codebase to suppress this warning.
> >>>>
> >>>> This patch adds two macros meant to bound one or many kfunc definitions.
> >>>> All existing kfunc definitions which use these __diag calls to suppress
> >>>> -Wmissing-prototypes are migrated to use the newly-introduced macros.
> >>>> A new __diag_ignore_all - for "-Wmissing-declarations" - is added to the
> >>>> __bpf_kfunc_start_defs macro based on feedback from Andrii on an earlier
> >>>> version of this patch [0] and another recent mailing list thread [1].
> >>>>
> >>>> In the future we might need to ignore different warnings or do other
> >>>> kfunc-specific things. This change will make it easier to make such
> >>>> modifications for all kfunc defs.
> >>>>
> >>>>   [0]: https://lore.kernel.org/bpf/CAEf4BzaE5dRWtK6RPLnjTW-MW9sx9K3Fn6uwqCTChK2Dcb1Xig@mail.gmail.com/
> >>>>   [1]: https://lore.kernel.org/bpf/ZT+2qCc%2FaXep0%2FLf@krava/
> >>>>
> >>>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> >>>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> >>>> Cc: Jiri Olsa <olsajiri@gmail.com>
> >>>> ---
> >>>>
> >>>> This patch was submitted earlier as part of task_vma
> >>>> iter series: https://lore.kernel.org/bpf/20231013204426.1074286-6-davemarchevsky@fb.com/
> >>>>
> >>>> This separate submission addresses Andrii's comments from
> >>>> that thread.
> >>>>
> >>>>  include/linux/btf.h              |  9 +++++++++
> >>>>  kernel/bpf/bpf_iter.c            |  6 ++----
> >>>>  kernel/bpf/cpumask.c             |  6 ++----
> >>>>  kernel/bpf/helpers.c             |  6 ++----
> >>>>  kernel/bpf/map_iter.c            |  6 ++----
> >>>>  kernel/bpf/task_iter.c           |  6 ++----
> >>>>  kernel/trace/bpf_trace.c         |  6 ++----
> >>>>  net/bpf/test_run.c               |  7 +++----
> >>>>  net/core/filter.c                | 13 ++++---------
> >>>>  net/core/xdp.c                   |  6 ++----
> >>>>  net/ipv4/fou_bpf.c               |  6 ++----
> >>>>  net/netfilter/nf_conntrack_bpf.c |  6 ++----
> >>>>  net/netfilter/nf_nat_bpf.c       |  6 ++----
> >>>>  net/xfrm/xfrm_interface_bpf.c    |  6 ++----
> >>>>  14 files changed, 38 insertions(+), 57 deletions(-)
> >>>>
> >>>
> >>> Thanks for your work.
> >>>
> >>> By using a simple grep for "__diag_ignore_all(\"-Wmissing-prototypes",
> >>> it appears that the files net/socket.c,
> >>> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c,
> >>> kernel/cgroup/rstat.c and Documentation/bpf/kfuncs.rst are missing. It
> >>> seems that we should also update them.
> >>>
> >>
> >> rstat.c and net/socket.c don't have kfuncs, so those are not relevant
> >> here.
> >
> > The bpf_rstat_flush() and update_socket_protocol() can also trigger
> > the -Wmissing-declarations.
> > These two functions are for BPF only. Shouldn't we better include them as well ?
> >
>
> I had this conundrum when writing the patch as well. Since they're not kfuncs
> and the macros are meant to wrap kfunc definitions, I felt that it would be

yeah, I was going to say the same, it's misleading

> confusing to someone unfamiliar with BPF internals. But I agree that the current
> state isn't ideal either.
>
> How about either:
>   * I use the __bpf_kfunc_{start,end}_defs macros in those two places,
>     with comment describing that they're not wrapping kfunc def, but rather
>     BPF hook point that throws the same warnings.
>   * Two additional macros, __bpf_hook_{start,end} are added, just
>     pointing to __bpf_kfunc_{start,end} for now. They're used for
>     these two functions
>
> WDYT?

I like the option #2 best

>
> >> But we are missing changes also in kernel/bpf/task_iter.c and
> >> kernel/bpf/cgroup_iter.c
> >>
> >> And let's update Documentation/bpf/kfuncs.rst to use your new set of macros?
> >>
> >> With the above addressed, please add my ack. Thanks!
> >>
> >> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >>
> >>> --
> >>> Regards
> >>> Yafang
> >
> >
> >
diff mbox series

Patch

diff --git a/include/linux/btf.h b/include/linux/btf.h
index c2231c64d60b..dc5ce962f600 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -84,6 +84,15 @@ 
  */
 #define __bpf_kfunc __used noinline
 
+#define __bpf_kfunc_start_defs()					       \
+	__diag_push();							       \
+	__diag_ignore_all("-Wmissing-declarations",			       \
+			  "Global kfuncs as their definitions will be in BTF");\
+	__diag_ignore_all("-Wmissing-prototypes",			       \
+			  "Global kfuncs as their definitions will be in BTF")
+
+#define __bpf_kfunc_end_defs() __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/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 833faa04461b..0fae79164187 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -782,9 +782,7 @@  struct bpf_iter_num_kern {
 	int end; /* final value, exclusive */
 } __aligned(8);
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start_defs();
 
 __bpf_kfunc int bpf_iter_num_new(struct bpf_iter_num *it, int start, int end)
 {
@@ -843,4 +841,4 @@  __bpf_kfunc void bpf_iter_num_destroy(struct bpf_iter_num *it)
 	s->cur = s->end = 0;
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
index 6983af8e093c..e01c741e54e7 100644
--- a/kernel/bpf/cpumask.c
+++ b/kernel/bpf/cpumask.c
@@ -34,9 +34,7 @@  static bool cpu_valid(u32 cpu)
 	return cpu < nr_cpu_ids;
 }
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global kfuncs as their definitions will be in BTF");
+__bpf_kfunc_start_defs();
 
 /**
  * bpf_cpumask_create() - Create a mutable BPF cpumask.
@@ -407,7 +405,7 @@  __bpf_kfunc u32 bpf_cpumask_any_and_distribute(const struct cpumask *src1,
 	return cpumask_any_and_distribute(src1, src2);
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(cpumask_kfunc_btf_ids)
 BTF_ID_FLAGS(func, bpf_cpumask_create, KF_ACQUIRE | KF_RET_NULL)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index e46ac288a108..0e4657606eaa 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1886,9 +1886,7 @@  void bpf_rb_root_free(const struct btf_field *field, void *rb_root,
 	}
 }
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start_defs();
 
 __bpf_kfunc void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign)
 {
@@ -2505,7 +2503,7 @@  __bpf_kfunc void bpf_throw(u64 cookie)
 	WARN(1, "A call to BPF exception callback should never return\n");
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(generic_btf_ids)
 #ifdef CONFIG_KEXEC_CORE
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 6fc9dae9edc8..6abd7c5df4b3 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -193,9 +193,7 @@  static int __init bpf_map_iter_init(void)
 
 late_initcall(bpf_map_iter_init);
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start_defs();
 
 __bpf_kfunc s64 bpf_map_sum_elem_count(const struct bpf_map *map)
 {
@@ -213,7 +211,7 @@  __bpf_kfunc s64 bpf_map_sum_elem_count(const struct bpf_map *map)
 	return ret;
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(bpf_map_iter_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_map_sum_elem_count, KF_TRUSTED_ARGS)
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 59e747938bdb..19518f5ba7a3 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -824,9 +824,7 @@  struct bpf_iter_task_vma_kern {
 	struct bpf_iter_task_vma_kern_data *data;
 } __attribute__((aligned(8)));
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start_defs();
 
 __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
 				      struct task_struct *task, u64 addr)
@@ -892,7 +890,7 @@  __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
 	}
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 struct bpf_iter_css_task {
 	__u64 __opaque[1];
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index df697c74d519..84e8a0f6e4e0 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1252,9 +1252,7 @@  static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
 };
 
 #ifdef CONFIG_KEYS
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "kfuncs which will be used in BPF programs");
+__bpf_kfunc_start_defs();
 
 /**
  * bpf_lookup_user_key - lookup a key by its serial
@@ -1404,7 +1402,7 @@  __bpf_kfunc int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
 }
 #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(key_sig_kfunc_set)
 BTF_ID_FLAGS(func, bpf_lookup_user_key, KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 0841f8d82419..c9fdcc5cdce1 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -503,9 +503,8 @@  static int bpf_test_finish(const union bpf_attr *kattr,
  * architecture dependent calling conventions. 7+ can be supported in the
  * future.
  */
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start_defs();
+
 __bpf_kfunc int bpf_fentry_test1(int a)
 {
 	return a + 1;
@@ -605,7 +604,7 @@  __bpf_kfunc void bpf_kfunc_call_memb_release(struct prog_test_member *p)
 {
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(bpf_test_modify_return_ids)
 BTF_ID_FLAGS(func, bpf_modify_return_test)
diff --git a/net/core/filter.c b/net/core/filter.c
index 21d75108c2e9..383f96b0a1c7 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11767,9 +11767,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");
+__bpf_kfunc_start_defs();
 __bpf_kfunc int bpf_dynptr_from_skb(struct sk_buff *skb, u64 flags,
 				    struct bpf_dynptr_kern *ptr__uninit)
 {
@@ -11816,7 +11814,7 @@  __bpf_kfunc int bpf_sock_addr_set_sun_path(struct bpf_sock_addr_kern *sa_kern,
 
 	return 0;
 }
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
 			       struct bpf_dynptr_kern *ptr__uninit)
@@ -11879,10 +11877,7 @@  static int __init bpf_kfunc_init(void)
 }
 late_initcall(bpf_kfunc_init);
 
-/* Disables missing prototype warnings */
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start_defs();
 
 /* bpf_sock_destroy: Destroy the given socket with ECONNABORTED error code.
  *
@@ -11916,7 +11911,7 @@  __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
 	return sk->sk_prot->diag_destroy(sk, ECONNABORTED);
 }
 
-__diag_pop()
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(bpf_sk_iter_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_sock_destroy, KF_TRUSTED_ARGS)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index df4789ab512d..b6f1d6dab3f2 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -696,9 +696,7 @@  struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf)
 	return nxdpf;
 }
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in vmlinux BTF");
+__bpf_kfunc_start_defs();
 
 /**
  * bpf_xdp_metadata_rx_timestamp - Read XDP frame RX timestamp.
@@ -738,7 +736,7 @@  __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash,
 	return -EOPNOTSUPP;
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(xdp_metadata_kfunc_ids)
 #define XDP_METADATA_KFUNC(_, __, name, ___) BTF_ID_FLAGS(func, name, KF_TRUSTED_ARGS)
diff --git a/net/ipv4/fou_bpf.c b/net/ipv4/fou_bpf.c
index 3760a14b6b57..4da03bf45c9b 100644
--- a/net/ipv4/fou_bpf.c
+++ b/net/ipv4/fou_bpf.c
@@ -22,9 +22,7 @@  enum bpf_fou_encap_type {
 	FOU_BPF_ENCAP_GUE,
 };
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in BTF");
+__bpf_kfunc_start_defs();
 
 /* bpf_skb_set_fou_encap - Set FOU encap parameters
  *
@@ -100,7 +98,7 @@  __bpf_kfunc int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
 	return 0;
 }
 
-__diag_pop()
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(fou_kfunc_set)
 BTF_ID_FLAGS(func, bpf_skb_set_fou_encap)
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index b21799d468d2..475358ec8212 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -230,9 +230,7 @@  static int _nf_conntrack_btf_struct_access(struct bpf_verifier_log *log,
 	return 0;
 }
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in nf_conntrack BTF");
+__bpf_kfunc_start_defs();
 
 /* bpf_xdp_ct_alloc - Allocate a new CT entry
  *
@@ -467,7 +465,7 @@  __bpf_kfunc int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
 	return nf_ct_change_status_common(nfct, status);
 }
 
-__diag_pop()
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(nf_ct_kfunc_set)
 BTF_ID_FLAGS(func, bpf_xdp_ct_alloc, KF_ACQUIRE | KF_RET_NULL)
diff --git a/net/netfilter/nf_nat_bpf.c b/net/netfilter/nf_nat_bpf.c
index 141ee7783223..6e3b2f58855f 100644
--- a/net/netfilter/nf_nat_bpf.c
+++ b/net/netfilter/nf_nat_bpf.c
@@ -12,9 +12,7 @@ 
 #include <net/netfilter/nf_conntrack_core.h>
 #include <net/netfilter/nf_nat.h>
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in nf_nat BTF");
+__bpf_kfunc_start_defs();
 
 /* bpf_ct_set_nat_info - Set source or destination nat address
  *
@@ -54,7 +52,7 @@  __bpf_kfunc int bpf_ct_set_nat_info(struct nf_conn___init *nfct,
 	return nf_nat_setup_info(ct, &range, manip) == NF_DROP ? -ENOMEM : 0;
 }
 
-__diag_pop()
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(nf_nat_kfunc_set)
 BTF_ID_FLAGS(func, bpf_ct_set_nat_info, KF_TRUSTED_ARGS)
diff --git a/net/xfrm/xfrm_interface_bpf.c b/net/xfrm/xfrm_interface_bpf.c
index d74f3fd20f2b..7d5e920141e9 100644
--- a/net/xfrm/xfrm_interface_bpf.c
+++ b/net/xfrm/xfrm_interface_bpf.c
@@ -27,9 +27,7 @@  struct bpf_xfrm_info {
 	int link;
 };
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "Global functions as their definitions will be in xfrm_interface BTF");
+__bpf_kfunc_start_defs();
 
 /* bpf_skb_get_xfrm_info - Get XFRM metadata
  *
@@ -93,7 +91,7 @@  __bpf_kfunc int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx, const struct bp
 	return 0;
 }
 
-__diag_pop()
+__bpf_kfunc_end_defs();
 
 BTF_SET8_START(xfrm_ifc_kfunc_set)
 BTF_ID_FLAGS(func, bpf_skb_get_xfrm_info)