Re: [PATCH v2 bpf-next 1/3] bpf: Introduce dynamic program extensions

[Yonghong Song <yhs@fb.com>]

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@gmail.com>
>