lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <22051390-2331-ad11-406b-1e5c6dbcd6a2@linux.dev> Date: Wed, 1 Nov 2023 18:32:57 -0700 From: Martin KaFai Lau <martin.lau@...ux.dev> To: Kui-Feng Lee <sinquersw@...il.com> Cc: kuifeng@...a.com, netdev@...r.kernel.org, bpf@...r.kernel.org, ast@...nel.org, song@...nel.org, kernel-team@...a.com, andrii@...nel.org, thinker.li@...il.com, drosen@...gle.com Subject: Re: [PATCH bpf-next v8 07/10] bpf, net: switch to dynamic registration On 11/1/23 5:59 PM, Kui-Feng Lee wrote: > > > On 11/1/23 17:17, Martin KaFai Lau wrote: >> On 10/31/23 5:19 PM, Kui-Feng Lee wrote: >>> >>> >>> On 10/31/23 17:02, Martin KaFai Lau wrote: >>>> On 10/31/23 4:34 PM, Kui-Feng Lee wrote: >>>>>>> diff --git a/include/linux/btf.h b/include/linux/btf.h >>>>>>> index a8813605f2f6..954536431e0b 100644 >>>>>>> --- a/include/linux/btf.h >>>>>>> +++ b/include/linux/btf.h >>>>>>> @@ -12,6 +12,8 @@ >>>>>>> #include <uapi/linux/bpf.h> >>>>>>> #define BTF_TYPE_EMIT(type) ((void)(type *)0) >>>>>>> +#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0); \ >>>>>> >>>>>> ((void)(struct type *)0); is new. Why is it needed? >>>>> >>>>> This is a trick of BTF to force compiler generate type info for >>>>> the given type. Without trick, compiler may skip these types if these >>>>> type are not used at all in the module. For example, modules usually >>>>> don't use value types of struct_ops directly. >>>> It is not the value type and value type emit is understood. It is the >>>> struct_ops type itself and it is new addition in this patchset afaict. The >>>> value type emit is in the next line which was cut out from the context here. >>>> >>> I mean both of them are required. >>> In the case of a dummy implementation, struct_ops type itself properly never >>> being used, only being declared by the module. Without this line, >> >> Other than bpf_dummy_ops, after reg(), the struct_ops->func() must be used >> somewhere in the kernel or module. Like tcp must be using the >> tcp_congestion_ops after reg(). bpf_dummy_ops is very special and probably >> should be moved out to bpf_testmod somehow but this is for later. Even >> bpf_dummy_ops does not have an issue now. Why it is needed after the kmod >> support change? >> >> or it is a preemptive addition to be future proof only? >> >> Addition is fine if it is required to work. I am trying to understand why this >> new addition is needed after the kmod support change. The reason why this is >> needed after the kmod support change is not obvious from looking at the code. >> The commit message didn't mention why and what broke after this kmod change. >> If someone wants to clean it up a few months later, we will need to figure out >> why it was added in the first place. > > > It is a future proof. > What do you think if I add a comment in the code? If it is not required to work, I prefer not adding it to avoid confusion and avoid future cleanup temptation. Even the artificial bpf_dummy_ops does not need it, so not enough reason to introduce this code redundancy. Switch topic. While we are on a new macro topic, I think a new macro will be useful to emit the value type and register_bpf_struct_ops together. wdyt? > >> >> >>> the module developer will fail to load a struct_ops map of the dummy >>> type. This line is added to avoid this awful situation. >>> >>
Powered by blists - more mailing lists