Re: [PATCH bpf 1/2] bpf: Add dummy type reference to nf_conn___init to fix type deduplication

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Wed, Nov 30, 2022 at 6:42 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> The bpf_ct_set_nat_info() kfunc is defined in the nf_nat.ko module, and
>> takes as a parameter the nf_conn___init struct, which is allocated through
>> the bpf_xdp_ct_alloc() helper defined in the nf_conntrack.ko module.
>> However, because kernel modules can't deduplicate BTF types between each
>> other, and the nf_conn___init struct is not referenced anywhere in vmlinux
>> BTF, this leads to two distinct BTF IDs for the same type (one in each
>> module). This confuses the verifier, as described here:
>>
>
> Argh, shouldn't have wasted writing [1], but oh well.
>
>   [1] https://lore.kernel.org/bpf/CAEf4Bza2xDZ45kxxa3dg1C_RWE=UB5UFYEuFp6rbXgX=LRHv-A@mail.gmail.com/

Ah, yeah, crossed streams; as you can see I came to the same conclusion
wrt types being conceptually independent.

>> https://lore.kernel.org/all/87leoh372s.fsf@toke.dk/
>>
>> As a workaround, add a dummy pointer to the type in net/filter.c, so the
>> type definition gets included in vmlinux BTF. This way, both modules can
>> refer to the same type ID (as they both build on top of vmlinux BTF), and
>> the verifier is no longer confused.
>>
>> Fixes: 820dc0523e05 ("net: netfilter: move bpf_ct_set_nat_info kfunc in nf_nat_bpf.c")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>  net/core/filter.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index bb0136e7a8e4..1bdf9efe8593 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -80,6 +80,7 @@
>>  #include <net/tls.h>
>>  #include <net/xdp.h>
>>  #include <net/mptcp.h>
>> +#include <net/netfilter/nf_conntrack_bpf.h>
>>
>>  static const struct bpf_func_proto *
>>  bpf_sk_base_func_proto(enum bpf_func_id func_id);
>> @@ -11531,3 +11532,17 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id)
>>
>>         return func;
>>  }
>> +
>> +#if IS_MODULE(CONFIG_NF_CONNTRACK) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)
>> +/* The nf_conn___init type is used in the NF_CONNTRACK kfuncs. The kfuncs are
>> + * defined in two different modules, and we want to be able to use them
>> + * interchangably with the same BTF type ID. Because modules can't de-duplicate
>> + * BTF IDs between each other, we need the type to be referenced in the vmlinux
>> + * BTF or the verifier will get confused about the different types. So we add
>> + * this dummy pointer to serve as a type reference which will be included in
>> + * vmlinux BTF, allowing both modules to refer to the same type ID.
>> + *
>> + * We use a pointer as that is smaller than an instance of the struct.
>> + */
>> +const struct nf_conn___init *ctinit;
>> +#endif
>
> Use BTF_TYPE_EMIT() instead maybe?

Ah, TIL about that macro; thanks, will fix!

-Toke