Re: [PATCH net-next 1/4] bpf: allow bpf programs to tail-call other bpf programs

On 5/21/15 9:43 AM, Andy Lutomirski wrote:

On Thu, May 21, 2015 at 9:40 AM, Alexei Starovoitov [off-list ref] wrote:

quoted

On 5/21/15 9:20 AM, Andy Lutomirski wrote:

quoted

What I mean is: why do we need the interface to be "look up this index
in an array and just to what it references" as a single atomic
instruction?  Can't we break it down into first "look up this index in
an array" and then "do this tail call"?

I've actually considered to do this split and do first part as map lookup
and 2nd as 'tail call to this ptr' insn, but it turned out to be
painful: verifier gets more complicated, ctx pointer needs to kept
somewhere, JITs need to special case two things instead of one.
Also I couldn't see a use case for exposing program pointer to the
program itself. I've explored this path only because it felt more
traditional 'goto *ptr' like, but adding new PTR_TO_PROG type to
verifier looked wasteful.

At some point, I think that it would be worth extending the verifier
to support more general non-integral scalar types. "Pointer to
tail-call target" would be just one of them.  "Pointer to skb" might
be nice as a real first-class scalar type that lives in a register as
opposed to just being magic typed context.

well, I don't see a use case for 'pointer to tail-call target',
but more generic 'pointer to skb' indeed is a useful concept.
I was thinking more like 'pointer to structure of the type X',
then we can natively support 'pointer to task_struct',
'pointer to inode', etc which will help tracing programs to be
written in more convenient way.
Right now pointer walking has to be done via bpf_probe_read()
helper as demonstrated in tracex1_kern.c example.
With this future 'pointer to struct of type X' knowledge in verifier
we'll be able to do 'ptr->field' natively with higher performance.

We'd still need some way to stick fds into a map, but that's not
really the verifier's problem.

well, they both need to be aware of that. When it comes to safety
generalization suffers. Have to do extra checks both in map_update_elem
and in verifier. No way around that.