On Mon, Jan 20, 2020 at 11:36:43PM -0800, John Fastabend wrote: > > > + > > + t1 = btf_type_skip_modifiers(btf1, t1->type, NULL); > > + t2 = btf_type_skip_modifiers(btf2, t2->type, NULL); > > Is it really best to skip modifiers? I would expect that if the > signature is different including modifiers then we should just reject it. > OTOH its not really C code here either so modifiers may not have the same > meaning. With just integers and struct it may be ok but if we add pointers > to ints then what would we expect from a const int*? > > So whats the reasoning for skipping modifiers? Is it purely an argument > that its not required for safety so solve it elsewhere? In that case then > checking names of functions is also equally not required. Function names are not checked by the kernel. It's purely libbpf and bpf_prog.c convention. The kernel operates on prog_fd+btf_id only. The names of function arguments are not compared either. The code has to skip modifiers. Otherwise the type comparison algorithm will be quite complex, since typedef is such modifier. Like 'u32' in original program and 'u32' in extension program would have to be recursively checked. Another reason to skip modifiers is 'volatile' modifier. I suspect we would have to use it from time to time in original placeholder functions. Yet new replacement function will be written without volatile. The placeholder may need volatile to make sure compiler doesn't optimize things away. I found cases where 'noinline' in placeholder was not enough. clang would still inline the body of the function and remove call instruction. So far I've been using volatile as a workaround. May be we will introduce new function attribute to clang. Having said that I share your concern regarding skipping 'const'. For 'const int arg' it's totally ok to skip it, since it's meaningless from safety pov, but for 'const int *arg' and 'const struct foo *arg' I'm planning to preserve it. It will be preserved at the verifier bpf_reg_state level though. Just checking that 'const' is present in extension prog's BTF doesn't help safety. I'm planing to make the verifier enforce that bpf prog cannot write into argument which type is pointer to const struct. That part is still wip. It will be implemented for global functions first and then for extension programs. Currently the verifier rejects any pointer to struct (other than context), so no backward compatibility issues.
