Re: [PATCH bpf-next v2] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

From: Daniel Borkmann <daniel@iogearbox.net>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Yonghong Song <yhs@fb.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Kernel Team <kernel-team@fb.com>,
	Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v2] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly
Date: Wed, 17 Mar 2021 17:59:50 +0100	[thread overview]
Message-ID: <c768692d-5a61-df68-a89a-b7ba64b5c188@iogearbox.net> (raw)
In-Reply-To: <CAADnVQ+hUjX-Hk9=9X+=ii1SusfsZJrsxXUn4krH1bUvNjuVRg@mail.gmail.com>

On 3/17/21 5:45 PM, Alexei Starovoitov wrote:
> On Tue, Mar 16, 2021 at 10:58 PM Yonghong Song <yhs@fb.com> wrote:
>> On 3/16/21 10:44 PM, Alexei Starovoitov wrote:
>>> On Tue, Mar 16, 2021 at 9:29 PM Yonghong Song <yhs@fb.com> wrote:
>>>> +       BTF_TYPE_EMIT_ENUM(BPF_TCP_ESTABLISHED);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +late_initcall(bpf_emit_btf_type);
>>>
>>> I think if we burn a dummy function on this it would be a wrong
>>> pattern to follow.
>>
>> Maybe we can pick another initcall to piggyback?
>>
>>> This is just a nop C statement.
>>> Typically we add BUILD_BUG_ON near the places that rely on that constraint.
>>> There is such a function already. It's tcp_set_state() as you pointed out.
>>> It's not using BTF of course, but I would move above BTF_TYPE_EMIT_ENUM there.
>>> I'm not sure why you're calling it "pollute net/ipv4/tcp.c".
>>
>> This is the minor reason. I first coded in that place and feel awkward
>> where we have macro referenced above and we still emit a BTF_TYPE_EMIT
>> below although with some comments.
>>
>> The major reason is I think we may have some uapi type/enum's (e.g., in
>> uapi/linux/bpf.h) which will be used in bpf program but not in kernel
>> itself. So we cannot generate types in vmlinux btf because of this. So I
>> used this case to find a place to generate these btf types.
>> BPF_TCP_CLOSE is actually such an example, it happens we have a
>> BUILD_BUG_ON in kernel to access it.
>> Maybe I am too forward looking?
> 
> It's great to be forward looking :)
> I'm just having a hard time justifying an empty function with single 'ret' insn
> that actually will be called at init time and it will stay empty like this for
> foreseeable future. Static analysis tools and whatnot will start sending
> patches to remove that empty function.

+1, even empty function exported as symbol for modules (to avoid compiler
optimization) might be better than initcall, imho.