On Fri, Feb 03, 2017 at 11:29:19PM +0100, Daniel Borkmann wrote: > On 02/03/2017 10:10 PM, William Tu wrote: > >Hi Alexei, > > > >why it is bogus? on my system, it fails without the patch applied. > > > >--William > > > >On Fri, Feb 3, 2017 at 12:55 PM, Alexei Starovoitov > ><alexei.starovoi...@gmail.com> wrote: > >>On Fri, Feb 03, 2017 at 09:22:45AM -0800, William Tu wrote: > >>>The patch fixes the case when adding a zero value to the packet > >>>pointer. The verifer reports the following error: > >>> [...] > >>> R0=imm0,min_value=0,max_value=0 > >>> R1=pkt(id=0,off=0,r=4) > >>> R2=pkt_end R3=fp-12 > >>> R4=imm4,min_value=4,max_value=4 > >>> R5=pkt(id=0,off=4,r=4) > >>> 269: (bf) r2 = r0 // r2 becomes imm0 > >>> 270: (77) r2 >>= 3 > >>> 271: (bf) r4 = r1 // r4 becomes pkt ptr > >>> 272: (0f) r4 += r2 // r4 += 0 > >>> addition of negative constant to packet pointer is not allowed > >>> > >>>Signed-off-by: William Tu <u9012...@gmail.com> > >>>Signed-off-by: Mihai Budiu <mbu...@vmware.com> > [...] > >>> { > >>>+ "direct packet access: test14 (pkt_ptr += 0, good access)", > >>>+ .insns = { > >>>+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, > >>>+ offsetof(struct __sk_buff, data)), > >>>+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1, > >>>+ offsetof(struct __sk_buff, data_end)), > >>>+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2), > >>>+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 0), > >> > >>wait. the test is bogus. > >>please write the proper test for the feature > >>and check that it fails before the patch and passes afterwards. > > But still same code path that is executed in verifier as BPF_K and > CONST_IMM tracked reg both share the same path under add_imm label > in check_packet_ptr_add(), no? So it becomes r2=pkt(id=0,off=0,r=0); > r0 = r2; r0 += 0 here in this test. Probably okay as well, though > there could be risk that in future both don't share the same path > for some reason. I guess you were referring to either adding tests > for BPF_K /and/ CONST_IMM reg or just the latter, right?
yes. Sorry I wasn't clear. imo the 'r0 += 0' is not something that verifier should recognize, since such nop insns shouldn't be generated by the compiler. It happened that the code path in verifier covers that case as well, but I think we really need to test 'rX += rY' case where rY is recognized as imm0, since that what the original use case was about.