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: <4a7d22ed-e34f-4e75-a796-8d2744b6c62e@gmail.com> Date: Wed, 1 Nov 2023 21:19:38 -0700 From: Kui-Feng Lee <sinquersw@...il.com> To: Martin KaFai Lau <martin.lau@...ux.dev> 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 18:32, Martin KaFai Lau wrote: > 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. Got it! > > 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? Like this? #define REGISTER_STRUCT_OPS(st_type, st_ops) { \ BTF_STRUCT_OPS_TYPE_EMIT(st_type); \ register_bpf_struct_ops(st_ops); } while(0) static int bpf_testmod_init(void) { .... REGISTER_STRUCT_OPS(bpf_testmod_ops, &bpf_bpf_testmod_ops); .... } or you like something more aggressive #define REGISTER_STRUCT_OPS(st_type) { \ BTF_STRUCT_OPS_TYPE_EMIT(st_type); \ register_bpf_struct_ops(&bpf_##st_type); } while(0) > >> >>> >>> >>>> 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