| Message ID | 20230106195130.1216841-1-void@manifault.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Annotate kfuncs with new __bpf_kfunc macro | expand |
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 > >