Alexei Starovoitov wrote:

On Mon, Jan 20, 2020 at 11:36:43PM -0800, John Fastabend wrote:

quoted

quoted

+
+	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.

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>