Re: [oss-drivers] Re: [PATCH bpf] bpf: verifier: make sure callees don't prune with caller differences

[Jiong Wang <jiong.wang@netronome.com>]

Jakub Kicinski writes:

<snip>

>> Is that just an optimisation that we can do because we know caller's
>> r1-r5 were scratched by the call?

(resent, got a bounce back from netdev mailing list for previous email)

I had been pondering whether it is really necessary to mark caller saved
registers as scratched. I kind of feel the following code inside
check_func_call might cause valid program rejected:

  /* after the call registers r0 - r5 were scratched */
    for (i = 0; i < CALLER_SAVED_REGS; i++) {
        mark_reg_not_init(env, caller->regs, caller_saved[i]);
        check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK);
    }

Because there is inter-procedure register allocation support in LLVM
(-enable-ipra), which could effectively eliminate register save/restore for
one caller-saved register across function call if the compiler can prove
callee or any other childs on the callgraph doesn't use/clobber this
particular caller-saved register. Then the later sequence in caller after
the call site could just safely read the caller-saved without restoring it
from stack etc. But we are marking all caller-saved as NOT_INIT, such read
will be treated as reading from uninitialized value, so the program will be
rejected.

-enable-ipra is disabled at default at the moment.

Regards,
Jiong

> r0 - r5 of the caller will not be pushed to the stack by JITs and
> therefore those are really dead.  It is an optimization.  We could've
> connected them but they are already marked as WRITTEN in
> check_func_call().
>
>> It looks like, without that, the change is "every reg in all frames
>> needs to be parented to the new-state", which is somewhat easier to
>> understand (though I did have to think for quite a long time before
>> it made sense to me why even that was necessary ;-)
>
> Ack, that was my initial patch actually, but Jiong pointed out it's
> unnecessary and I didn't argue :)  r1 - r5 are marked as WRITTEN and
> UNINIT anyway, and r0 gets its parent from the callees frame
> (prepare_func_exit() links callers r0 parentage chain to previous
> states in the callee).