[bpf-next,0/3] Annotate kfuncs with new __bpf_kfunc macro

  BPF kfuncs are kernel functions that can be invoked by BPF programs.
kfuncs can be kernel functions which are also called elsewhere in the
main kernel (such as crash_kexec()), or may be functions that are only
meant to be used by BPF programs, such as bpf_task_acquire(), and which
are not called from anywhere else in the kernel.

While thus far we haven't observed any issues such as kfuncs being
elided by the compiler, at some point we could easily run into problems
such as the following:

- static kernel functions that are also used as kfuncs could be inlined
  and/or elided by the compiler.
- BPF-specific kfuncs with external linkage may at some point be elided
  by the compiler in LTO builds, when it's determined that they aren't
  called anywhere.

To address this, this patch set introduces a new __bpf_kfunc macro which
should be added to all kfuncs, and which will protect kfuncs from such
problems. Note that some kfuncs kind of try to do this already by
specifying noinline or __used. We are inconsistent in how this is
applied. __bpf_kfunc should provide a uniform and more-future-proof way
to do this.

David Vernet (3):
  bpf: Add __bpf_kfunc tag for marking kernel functions as kfuncs
  bpf: Document usage of the new __bpf_kfunc macro
  bpf: Add __bpf_kfunc tag to all kfuncs

 Documentation/bpf/kfuncs.rst                  | 18 +++++
 Documentation/conf.py                         |  3 +
 include/linux/btf.h                           |  9 +++
 kernel/bpf/helpers.c                          | 19 +++++
 kernel/cgroup/rstat.c                         |  2 +
 kernel/kexec_core.c                           |  2 +
 kernel/trace/bpf_trace.c                      |  4 +
 net/bpf/test_run.c                            | 76 ++++++++++++-------
 net/ipv4/tcp_bbr.c                            |  8 ++
 net/ipv4/tcp_cong.c                           |  5 ++
 net/ipv4/tcp_cubic.c                          |  6 ++
 net/ipv4/tcp_dctcp.c                          |  6 ++
 net/netfilter/nf_conntrack_bpf.c              | 14 +++-
 net/netfilter/nf_nat_bpf.c                    |  1 +
 net/xfrm/xfrm_interface_bpf.c                 |  4 +-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  2 +-
 16 files changed, 146 insertions(+), 33 deletions(-)
  

On Fri, Jan 6, 2023 at 11:51 AM David Vernet <void@manifault.com> wrote:
>
> BPF kfuncs are kernel functions that can be invoked by BPF programs.
> kfuncs can be kernel functions which are also called elsewhere in the
> main kernel (such as crash_kexec()), or may be functions that are only
> meant to be used by BPF programs, such as bpf_task_acquire(), and which
> are not called from anywhere else in the kernel.
>
> While thus far we haven't observed any issues such as kfuncs being
> elided by the compiler, at some point we could easily run into problems
> such as the following:
>
> - static kernel functions that are also used as kfuncs could be inlined
>   and/or elided by the compiler.
> - BPF-specific kfuncs with external linkage may at some point be elided
>   by the compiler in LTO builds, when it's determined that they aren't
>   called anywhere.
>
> To address this, this patch set introduces a new __bpf_kfunc macro which
> should be added to all kfuncs, and which will protect kfuncs from such
> problems. Note that some kfuncs kind of try to do this already by
> specifying noinline or __used. We are inconsistent in how this is
> applied. __bpf_kfunc should provide a uniform and more-future-proof way
> to do this.

The series looks reasonable to me. Would be nice if we can somehow
prevent (with a checkpatch?) adding new kfuncs without this new tag,
but I don't see an easy way.
I was waiting in case other would like to comment, but if nothing to discuss:

Acked-by: Stanislav Fomichev <sdf@google.com>




> David Vernet (3):
>   bpf: Add __bpf_kfunc tag for marking kernel functions as kfuncs
>   bpf: Document usage of the new __bpf_kfunc macro
>   bpf: Add __bpf_kfunc tag to all kfuncs
>
>  Documentation/bpf/kfuncs.rst                  | 18 +++++
>  Documentation/conf.py                         |  3 +
>  include/linux/btf.h                           |  9 +++
>  kernel/bpf/helpers.c                          | 19 +++++
>  kernel/cgroup/rstat.c                         |  2 +
>  kernel/kexec_core.c                           |  2 +
>  kernel/trace/bpf_trace.c                      |  4 +
>  net/bpf/test_run.c                            | 76 ++++++++++++-------
>  net/ipv4/tcp_bbr.c                            |  8 ++
>  net/ipv4/tcp_cong.c                           |  5 ++
>  net/ipv4/tcp_cubic.c                          |  6 ++
>  net/ipv4/tcp_dctcp.c                          |  6 ++
>  net/netfilter/nf_conntrack_bpf.c              | 14 +++-
>  net/netfilter/nf_nat_bpf.c                    |  1 +
>  net/xfrm/xfrm_interface_bpf.c                 |  4 +-
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  2 +-
>  16 files changed, 146 insertions(+), 33 deletions(-)
>
> --
> 2.39.0
>
  

On Fri, Jan 06, 2023 at 04:47:35PM -0800, Stanislav Fomichev wrote:
> On Fri, Jan 6, 2023 at 11:51 AM David Vernet <void@manifault.com> wrote:
> >
> > BPF kfuncs are kernel functions that can be invoked by BPF programs.
> > kfuncs can be kernel functions which are also called elsewhere in the
> > main kernel (such as crash_kexec()), or may be functions that are only
> > meant to be used by BPF programs, such as bpf_task_acquire(), and which
> > are not called from anywhere else in the kernel.
> >
> > While thus far we haven't observed any issues such as kfuncs being
> > elided by the compiler, at some point we could easily run into problems
> > such as the following:
> >
> > - static kernel functions that are also used as kfuncs could be inlined
> >   and/or elided by the compiler.
> > - BPF-specific kfuncs with external linkage may at some point be elided
> >   by the compiler in LTO builds, when it's determined that they aren't
> >   called anywhere.
> >
> > To address this, this patch set introduces a new __bpf_kfunc macro which
> > should be added to all kfuncs, and which will protect kfuncs from such
> > problems. Note that some kfuncs kind of try to do this already by
> > specifying noinline or __used. We are inconsistent in how this is
> > applied. __bpf_kfunc should provide a uniform and more-future-proof way
> > to do this.
> 
> The series looks reasonable to me. Would be nice if we can somehow
> prevent (with a checkpatch?) adding new kfuncs without this new tag,
> but I don't see an easy way.
> I was waiting in case other would like to comment, but if nothing to discuss:

Thanks for the review, Stanislav. I agree that it would be nice to have
some automation to prevent forgetting the tag. I thought about ways to
possibly do it, including playing around with putting the kfuncs into a
separate section for post-processing which we could check against
.BTF_ids, but it felt like a lot of complexity / possibly controversial
changes that I'm hesitant to bring into the patch set which should be
pretty non-controversial otherwise.

With respect to validating the presence of kfunc "tags" (i.e. the
__diag_push() / __diag_pop() we were doing before), we're in the same
state after this patch as we were before, so my preference is to defer
improving that until a later time when we've fried some of the bigger
kfunc fish. Does that sound ok?

> Acked-by: Stanislav Fomichev <sdf@google.com>

Thanks! FYI, I'm planning on sending a v2 with Alexei's suggestion [0]

[0]: https://lore.kernel.org/all/CAADnVQLpK7WXTjF6GS1hcfPXf=8iERJmEeVFfvmG75mJj0DdaA@mail.gmail.com/

I'll go ahead and preemptively leave off your Acked-by for that, as the
patches will have changed enough that it probably warrants another
read through.

- David

> 
> 
> 
> 
> > David Vernet (3):
> >   bpf: Add __bpf_kfunc tag for marking kernel functions as kfuncs
> >   bpf: Document usage of the new __bpf_kfunc macro
> >   bpf: Add __bpf_kfunc tag to all kfuncs
> >
> >  Documentation/bpf/kfuncs.rst                  | 18 +++++
> >  Documentation/conf.py                         |  3 +
> >  include/linux/btf.h                           |  9 +++
> >  kernel/bpf/helpers.c                          | 19 +++++
> >  kernel/cgroup/rstat.c                         |  2 +
> >  kernel/kexec_core.c                           |  2 +
> >  kernel/trace/bpf_trace.c                      |  4 +
> >  net/bpf/test_run.c                            | 76 ++++++++++++-------
> >  net/ipv4/tcp_bbr.c                            |  8 ++
> >  net/ipv4/tcp_cong.c                           |  5 ++
> >  net/ipv4/tcp_cubic.c                          |  6 ++
> >  net/ipv4/tcp_dctcp.c                          |  6 ++
> >  net/netfilter/nf_conntrack_bpf.c              | 14 +++-
> >  net/netfilter/nf_nat_bpf.c                    |  1 +
> >  net/xfrm/xfrm_interface_bpf.c                 |  4 +-
> >  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  2 +-
> >  16 files changed, 146 insertions(+), 33 deletions(-)
> >
> > --
> > 2.39.0
> >