On Sat, Apr 28, 2018 at 11:19 PM, Y Song <[email protected]> wrote:
> On Sat, Apr 28, 2018 at 12:00 PM, Y Song <[email protected]> wrote:
>> Hi, William,
>>
>> Do you have a test (just compile and load) to demonstrate the problem?
>> I would like to understand why the compiler generates r1+0 and whether
>> we could possibly avoid it.
>>
>> Thanks,
>> Yonghong
>>
>>
>> On Sat, Apr 28, 2018 at 7:57 AM, William Tu via iovisor-dev
>> <[email protected]> wrote:
>>> Hi,
>>>
>>> We're hitting a BPF verifier error saying
>>> "dereference of modified ctx ptr R1 off=36+0, ctx+const is allowed,
>>> ctx+const+const is not
>>> "
>>> but actually the 2nd const is 0. I don't know why compiler generate (r1+0)
>>> in this case:
>>> 602: (61) r4 = *(u32 *)(r1 +0)
>>>
>>> verifier log
>>> ========
>>> 594: (15) if r6 == 0x0 goto pc+8
>>> R0=inv(id=0) R1=inv7161128523638600565
>>> R3=inv(id=0,umax_value=65535,var_off=(0x0; 0xffff)) R4=inv0
>>> R6=ctx(id=0,off=0,imm=0) R7=inv2
>>> R8=map_value(id=0,off=0,ks=232,vs=4352,imm=0) R9=inv(id=0)
>>> R10=fp0,call_-1 fp-352=map_value fp-360=ctx
>>> 595: (bf) r1 = r6
>>> 596: (07) r1 += 40
>>> 597: (61) r2 = *(u32 *)(r6 +52)
>>> 598: (b7) r4 = 0
>>> 599: (1d) if r2 == r4 goto pc+2
>>> R0=inv(id=0) R1=ctx(id=0,off=40,imm=0)
>>> R2=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff))
>>> R3=inv(id=0,umax_value=65535,var_off=(0x0; 0xffff)) R4=inv0
>>> R6=ctx(id=0,off=0,imm=0) R7=inv2
>>> R8=map_value(id=0,off=0,ks=232,vs=4352,imm=0) R9=inv(id=0)
>>> R10=fp0,call_-1 fp-352=map_value fp-360=ctx
>>> 600: (bf) r1 = r6
>>> 601: (07) r1 += 36 ---->
>>> r1 has offset 36
>>> 602: (61) r4 = *(u32 *)(r1 +0) ----->
>>> then r1 + 0
>>> dereference of modified ctx ptr R1 off=36+0, ctx+const is allowed,
>>> ctx+const+const is not
>>>
>>>
>>> llvm-objdump
>>> ==========
>>> ; if (!skb)
>>> 594: if r6 == 0 goto 8
>>> ; return skb->ifindex;
>>> 595: r1 = r6
>>> 596: r1 += 40
>>> ; if (skb->cb[OVS_CB_INGRESS]) {
>>> 597: r2 = *(u32 *)(r6 + 52)
>>> 598: r4 = 0
>>> 599: if r2 == r4 goto 2
>>> ; return skb->ingress_ifindex;
>>> 600: r1 = r6
>>> 601: r1 += 36
>>>
>>> LBB1_28:
>>> 602: r4 = *(u32 *)(r1 + 0)
>
>
> Now I remembered that we had this issue before in bcc.
> it is a compiler optimization likes this:
> if (...)
> *(ctx + 60)
> else
> *(ctx + 56)
>
> The compiler translates it to
> if (...)
> ptr = ctx + 60
> else
> ptr = ctx + 56
> *(ptr + 0)
>
> Yes, this *(ptr + 0) will cause problem in verifier
> as verifier only allows direct "ctx + offset" offset access.
>
Interesting... thanks for explanation.
> Since in bcc this happens in auto generated code, we put the barrier
> after the original
> ctx memory assess
>
> if (...) {
> *(ctx + 60)
> __asm__ __volatile__(\"\": : :\"memory\");
> } else {
> *(ctx + 56)
> __asm__ __volatile__(\"\": : :\"memory\");
> }
>
> Should we fix this in verifier? Probably as adding these compiler
> barriers indeed very annoying.
>
>
>>>
>>> With the below patch fix the issue
>>> ==========================
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -1619,7 +1619,7 @@ static int check_mem_access(struct
>>> bpf_verifier_env *env, int insn_idx, u32 regn
>>> /* ctx accesses must be at a fixed offset, so that we can
>>> * determine what type of data were returned.
>>> */
>>> - if (reg->off) {
>>> + if (reg->off && reg->off != off) {
>>> verbose(env,
>>> "dereference of modified ctx ptr R%d
>>> off=%d+%d, ctx+const is allowed, ctx+const+const is not\n",
>>> regno, reg->off, off - reg->off);
>>>
>
> You change looks okay. When you send upstream patch,
> you may want to add some context about
> when this case can happen, what workaround you could have
> and why this change is preferable.
>
OK, I will submit the patch.
Thanks
William
_______________________________________________
iovisor-dev mailing list
[email protected]
https://lists.iovisor.org/mailman/listinfo/iovisor-dev
