* Re: more on FP operations
2017-04-20 21:52 ` Daniel Borkmann
@ 2017-04-20 23:13 ` Alexei Starovoitov
0 siblings, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2017-04-20 23:13 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David Miller, ast, borkmann, netdev
On Thu, Apr 20, 2017 at 11:52:44PM +0200, Daniel Borkmann wrote:
> On 04/20/2017 08:06 PM, David Miller wrote:
> >
> >I'm running test_verifier for testing, and I notice in my JIT that a
> >32-bit move from the frame pointer (BPF_REG_10) ends up in the JIT.
> >
> >It is from this test:
> >
> > "unpriv: partial copy of pointer",
> > .insns = {
> > BPF_MOV32_REG(BPF_REG_1, BPF_REG_10),
> > BPF_MOV64_IMM(BPF_REG_0, 0),
> > BPF_EXIT_INSN(),
> > },
> > .errstr_unpriv = "R10 partial copy",
> > .result_unpriv = REJECT,
> > .result = ACCEPT,
> >
> >It seems to suggest that privileged code is allowed to do this, but I
> >can't think of a legitimate usage.
>
> One thing I could think of right now would be for use in 32 bit
> archs, but that would still need to be taught to the verifier
> first. Other patterns f.e. like ...
>
> {
> "unpriv: adding of fp",
> .insns = {
> BPF_MOV64_IMM(BPF_REG_1, 0),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_10),
> BPF_MOV64_IMM(BPF_REG_0, 0),
> BPF_EXIT_INSN(),
> },
> .errstr_unpriv = "pointer arithmetic prohibited",
> .result_unpriv = REJECT,
> .result = ACCEPT,
> },
>
> ... are currently also possible, but in the above and the partial
> copy r1 is always considered as UNKNOWN_VALUE from that point onward
> and there's not really much we could do with it anymore, except
> perhaps passing to bpf_probe_read() for inspection in tracing for
> some reason. Since there are also various other pointers, it is
> really only the FP that needs to be special cased for sparc JIT,
> right?
Just remembered another issue with frame pointers.
In is_spillable_regtype() we have:
case PTR_TO_STACK:
...
case FRAME_PTR:
return true;
so both FP and FP-10 are spillable, since LLVM sometimes
thinks that it's cheaper to load stack pointer from stack memory
instead of recomputing it. It's doing it rarely. Unfortunately
I couldn't figure out how to teach llvm not to do that.
> >I really want to be able to JIT anything the verifier accepts, but I
> >have a hard time justifying adding 32-bit FP register move support,
> >adjusting for the stack bias, etc.
32-bit FP reg move is indeed useless.
The only reason I used R10 when i did that commit bf5088773faff
is because on the program entry it only has two valid pointers r1 and r10.
r1 was used in the other test and this test used r10.
If we only disallow 32-bit move from reg_type=FRAME_PTR
it's not going to be enough. We'd need to disallow spilling of
reg_type=FRAME_PTR, which I'm afraid may break some programs.
Like:
r2 = r10
*(u64*)stack = r2;
..stuff...
r3 = *(u64*)stack;
r3 += -10;
is currently recognized by the verifier, since it tracks pointer types
through spill/fill.
I made llvm never generate 'r3 -= 10' in the above example, but
I couldn't make it avoid spill/fill of pointers to stack.
It was trivial to support it in the verifier, since it has to track
spill/fill of other pointer types anyway. I didn't expect it to
cause JIT headaches.
Such spill/fill is rare, so if it's too ugly to support in sparc JIT
we can add a boolean flag where verifier will say that FP spill/fill
is used by the prog and JIT can ignore such progs.
I don't think JIT can detect that by itself and verifier help is needed.
^ permalink raw reply [flat|nested] 3+ messages in thread