|
On Sat, Apr 28, 2018 at 11:19 PM, Y Song <ys114321@...> wrote: On Sat, Apr 28, 2018 at 12:00 PM, Y Song <ys114321@...> 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
<iovisor-dev@...> 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
|
|
|
On Sat, Apr 28, 2018 at 12:00 PM, Y Song <ys114321@...> 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
<iovisor-dev@...> 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. 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. Is this the right fix?
Thanks
William
_______________________________________________
iovisor-dev mailing list
iovisor-dev@...
https://lists.iovisor.org/mailman/listinfo/iovisor-dev
|
|
|
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 <iovisor-dev@...> 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)
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);
Is this the right fix?
Thanks
William
_______________________________________________
iovisor-dev mailing list
iovisor-dev@...
https://lists.iovisor.org/mailman/listinfo/iovisor-dev
|
|
|
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)
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);
Is this the right fix?
Thanks
William
|
|
|
