Message-ID: <5e273fce74a7f_20522ac6ec2c45c457@john-XPS-13-9370.notmuch>
Date:   Tue, 21 Jan 2020 10:15:42 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net,
        daniel@...earbox.net, netdev@...r.kernel.org, bpf@...r.kernel.org,
        kernel-team@...com
Subject: Re: [PATCH v2 bpf-next 1/3] bpf: Introduce dynamic program extensions

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.

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

Powered by blists - more mailing lists