On 1/21/20 10:15 AM, John Fastabend wrote: > Alexei Starovoitov wrote: >> 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. > > Sorry mistyped names of struct is what I meant, but that is probably nice to > have per comment. > >> >> 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. > > Yes, we have various similar issue and have in the past used volatile to work > around them but volatile's inside loops tends to break loop optimizations and > cause clang warnings/errors. Another common one is verifier failing to track > when scalars move around in registers. As an example the following is valid > C for a bounded additon to array pointer but not tractable for the verifier > at the moment. (made example at some point I'll dig up a collection of > real-world examples) > > r1 = *(u64 *)(r10 - 8) > r6 = r1 > if r6 < %[const] goto %l[err] > r3 += r1 > r2 = %[copy_size] > r1 = r7 > call 4 > > compiler barriers help but not always and also breaks loop optimization > passes. But, thats a different discussion I only mention it because > either verifier has to track above logic better or new attributes in clang > could be used for these things. But the new attributes don't usually work > well when mixed with optimization passes that we would actually like to > keep. John, could you send your original C code how to reproduce this? I am working on llvm side to avoid such optimizations, i.e., moving scalars around so later on you could have two scales with different verified state holding the [slightly] same value. > >> >> 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. > > Ah ok this will be great. In that case const will be more general then > merely functions and should be applicable generally at least as an end > goal IMO. There will be a slight annoyance where old extensions may not > run on new kernels though. I will argue such extensions are broken though. > > For this patch then, > > Acked-by: John Fastabend <john.fastabend@xxxxxxxxx> >
