Re: [RFC bpf-next 08/13] kbuild, bpf: add module-specific pahole/resolve_btfids flags for base reference BTF

[Alan Maguire <alan.maguire@oracle.com>]

On 23/03/2024 02:50, Alexei Starovoitov wrote:
> On Fri, Mar 22, 2024 at 3:26 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> Support creation of module BTF along with base reference BTF;
>> the latter is stored in a .BTF.base_ref ELF section and supplements
>> split BTF references to base BTF with information about base types,
>> allowing for later reconciliation of split BTF with a (possibly
>> changed) base.  resolve_btfids uses the "-r" option to specify
>> that the BTF.ids section should be populated with split BTF
>> relative to the added .BTF.base_ref section rather than relative
>> to the vmlinux base.
>>
>> Modules using base reference BTF can be built via
>>
>> BTF_BASE_REF=1 make -C. -M=path2/module
>>
>> The default is still to use split BTF relative to vmlinux.
>>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  scripts/Makefile.btf      | 7 +++++++
>>  scripts/Makefile.modfinal | 4 ++--
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
>> index 9694ca3c5252..c8212b2ab7ca 100644
>> --- a/scripts/Makefile.btf
>> +++ b/scripts/Makefile.btf
>> @@ -19,4 +19,11 @@ pahole-flags-$(call test-ge, $(pahole-ver), 126)     = -j --btf_features=encode_forc
>>
>>  pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         += --lang_exclude=rust
>>
>> +ifeq ($(BTF_BASE_REF),1)
>> +module-pahole-flags-$(call test-ge, $(pahole-ver), 126)        += --btf_features=base_ref
>> +module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
>> +endif
> 
> The patch set looks great to me.
> I wonder whether we should drop this extra BTF_BASE_REF flag
> and do it unconditionally.
> Currently btf_parse_module() doesn't have any real checks
> whether module's btf matches base_btf.
> All the btf_check_*() might succeed by luck even if vmlinux btf
> is different.
> Since .BTF.base_ref is small we can always emit it and
> during module load the btf_reconcile() step will be that safety check.
> Less build options, less moving pieces and doing it all the time
> will make the whole process robust.
> Conditional BTF_BASE_REF=1 will likely mean that there will be
> bugs that only few folks will hit.

We could certainly do it unconditionally for out-of-tree module builds
(the KBUILD_EXTMOD case); so anytime a user does "make -C.
M=path/2/module", they would get base reference BTF. With this series,
that would just mean applying this change:

diff --git a/scripts/Makefile.btf b/scripts/Makefile.btf
index c8212b2ab7ca..0322f8450a89 100644
--- a/scripts/Makefile.btf
+++ b/scripts/Makefile.btf
@@ -19,7 +19,7 @@ pahole-flags-$(call test-ge, $(pahole-ver), 126)
= -j --btf_features=encode_forc

 pahole-flags-$(CONFIG_PAHOLE_HAS_LANG_EXCLUDE)         +=
--lang_exclude=rust

-ifeq ($(BTF_BASE_REF),1)
+ifneq ($(KBUILD_EXTMOD),)
 module-pahole-flags-$(call test-ge, $(pahole-ver), 126)        +=
--btf_features=base_ref
 module-resolve-btfids-flags-$(call test-ge, $(pahole-ver), 126) = -r
 endif

So with this approach, in-tree modules don't add base reference BTF
since they don't need it, while out-of-tree module builds always do.
We could arrange for bpf_testmod to be built both ways perhaps to ensure
both approaches get tested in selftests along with kfunc addition etc.

For adding base reference BTF in all cases, I ran the numbers on adding
base reference BTF across all kernel modules and for x86_64 with 2667
modules. Base reference BTF adds around 4MB in total. Not huge, but for
in-tree builds, the only cases we've seen that triggered BTF mismatches
have been broken cases where vmlinux got built twice during kernel
build. So my suggestion would be to limit base reference BTF addition to
the KBUILD_EXTMOD case, and make it unconditional for that case only.

Thanks!

Alan