* [REGRESSION][PATCH V4 0/3] bpf jit drops the ball on negative memory references @ 2012-03-30 15:00 Jan Seiffert 2012-03-30 15:08 ` [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits Jan Seiffert 2012-03-30 15:35 ` [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets Jan Seiffert 0 siblings, 2 replies; 22+ messages in thread From: Jan Seiffert @ 2012-03-30 15:00 UTC (permalink / raw) To: netdev Cc: Eric Dumazet, linuxppc-dev, linux-kernel, Matt Evans, David S. Miller Consider the following test program: #include <stdio.h> #include <sys/types.h> #include <sys/socket.h> #include <netinet/in.h> #include <pcap-bpf.h> #define die(x) do {perror(x); return 1;} while (0) struct bpf_insn udp_filter[] = { /* 0 */ BPF_STMT(BPF_LDX|BPF_W|BPF_IMM, -1048576+(0)), /* leax net[0] */ /* 1 */ BPF_STMT(BPF_LD|BPF_B|BPF_IND, 0), /* ldb [x+0] */ /* 2 */ BPF_STMT(BPF_RET|BPF_A, 0), /* ret a */ }; int main(int argc, char *argv[]) { char buf[512]; struct sockaddr_in addr; struct bpf_program prg; socklen_t addr_s; ssize_t res; int fd; addr.sin_family = AF_INET; addr.sin_port = htons(5000); addr.sin_addr.s_addr = 0; addr_s = sizeof(addr); prg.bf_len = sizeof(udp_filter)/sizeof(udp_filter[0]); prg.bf_insns = udp_filter; if(-1 == (fd = socket(AF_INET, SOCK_DGRAM, 0))) die("socket"); if(-1 == bind(fd, (struct sockaddr *)&addr, sizeof(addr))) die("bind"); if(-1 == setsockopt(fd, SOL_SOCKET, SO_ATTACH_FILTER, &prg, sizeof(prg))) die("setsockopt"); res = recvfrom(fd, buf, sizeof(buf), 0, (struct sockaddr *)&addr, &addr_s); if(res != -1) printf("packet received: %zi bytes\n", res); else die("recvfrom"); return 0; } when used with the bpf jit disabled works: console 1 $ ./bpf console 2 $ echo "hello" | nc -u localhost 5000 console 1: packet received: 6 bytes When the bpf jit gets enabled (echo 100 > /proc/sys/net/core/bpf_jit_enable) the same program stops working: console 1 $ ./bpf console 2 $ echo "hello" | nc -u localhost 5000 console 1: The reason is that both jits (x86 and powerpc) do not handle negative memory references like SKF_NET_OFF or SKF_LL_OFF, only the simple ancillary data references are supported (by mapping to special instructions). In the case of an absolute reference, the jit aborts the translation if a negative reference is seen, also a negative k on the indirect load aborts the translation, but if X is negative to begin with, only the error handler is reached at runtime which drops the whole packet. Such a setup is useful to say filter bogus source addresses on an UDP socket. I propose the following patch series to fix this situation. Patch 1 exports the helper function the interpreter uses. Patch 2 incorporates the helper into the x86 jit (so it depends on patch 1). Patch 3 incorporates the helper into the powerpc jit (so it depends on patch 1). Lightly tested on x86, but the powerpc asm part is prop. wrong, could need assistance. Signed-of-by: Jan Seiffert <kaffeemonster@googlemail.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits 2012-03-30 15:00 [REGRESSION][PATCH V4 0/3] bpf jit drops the ball on negative memory references Jan Seiffert @ 2012-03-30 15:08 ` Jan Seiffert 2012-03-30 18:56 ` Eric Dumazet ` (2 more replies) 2012-03-30 15:35 ` [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets Jan Seiffert 1 sibling, 3 replies; 22+ messages in thread From: Jan Seiffert @ 2012-03-30 15:08 UTC (permalink / raw) To: netdev Cc: Eric Dumazet, linuxppc-dev, linux-kernel, Matt Evans, David S. Miller The function is renamed to make it a little more clear what it does. It is not added to any .h because it is not for general consumption, only for bpf internal use (and so by the jits). Signed-of-by: Jan Seiffert <kaffeemonster@googlemail.com> --- a/net/core/filter.c +++ b/net/core/filter.c @@ -40,8 +40,12 @@ #include <linux/reciprocal_div.h> #include <linux/ratelimit.h> -/* No hurry in this branch */ -static void *__load_pointer(const struct sk_buff *skb, int k, unsigned int size) +/* + * No hurry in this branch + * + * Exported for the bpf jit load helper. + */ +void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size) { u8 *ptr = NULL; @@ -60,7 +64,7 @@ static inline void *load_pointer(const struct sk_buff *skb, int k, { if (k >= 0) return skb_header_pointer(skb, k, size, buffer); - return __load_pointer(skb, k, size); + return bpf_internal_load_pointer_neg_helper(skb, k, size); } /** ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits 2012-03-30 15:08 ` [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits Jan Seiffert @ 2012-03-30 18:56 ` Eric Dumazet 2012-04-02 9:18 ` David Laight 2012-04-03 22:02 ` David Miller 2 siblings, 0 replies; 22+ messages in thread From: Eric Dumazet @ 2012-03-30 18:56 UTC (permalink / raw) To: Jan Seiffert Cc: netdev, linuxppc-dev, linux-kernel, Matt Evans, David S. Miller On Fri, 2012-03-30 at 17:08 +0200, Jan Seiffert wrote: > The function is renamed to make it a little more clear what it does. > It is not added to any .h because it is not for general consumption, only for > bpf internal use (and so by the jits). > > Signed-of-by: Jan Seiffert <kaffeemonster@googlemail.com> > Missing "---" line separator (check Documentation/SubmittingPatches line 490) You can check http://patchwork.ozlabs.org/patch/149683/ and see there is a problem, compared to http://patchwork.ozlabs.org/patch/149441/ for example > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -40,8 +40,12 @@ > #include <linux/reciprocal_div.h> > #include <linux/ratelimit.h> > > -/* No hurry in this branch */ > -static void *__load_pointer(const struct sk_buff *skb, int k, unsigned int size) > +/* > + * No hurry in this branch > + * > + * Exported for the bpf jit load helper. > + */ Seems good to me, maybe add a strong warning in the comment to say that function prototype can NOT change without major surgery in ASM files, since assembler wont catch the prototype change for us. Acked-by: Eric Dumazet <eric.dumazet@gmail.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits 2012-03-30 15:08 ` [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits Jan Seiffert 2012-03-30 18:56 ` Eric Dumazet @ 2012-04-02 9:18 ` David Laight 2012-04-02 13:02 ` Jan Seiffert 2012-04-03 22:02 ` David Miller 2 siblings, 1 reply; 22+ messages in thread From: David Laight @ 2012-04-02 9:18 UTC (permalink / raw) To: Jan Seiffert, netdev Cc: Eric Dumazet, linuxppc-dev, linux-kernel, Matt Evans, David S. Miller =20 > The function is renamed to make it a little more clear what it does. > It is not added to any .h because it is not for general=20 > consumption, only for bpf internal use (and so by the jits). I'd have thought it better to put in into a bfp_internal.h (or similar) with a big warning there about the asm users. Possibly even worth adding some other defs that the asm files will need (if there are any). David ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits 2012-04-02 9:18 ` David Laight @ 2012-04-02 13:02 ` Jan Seiffert 0 siblings, 0 replies; 22+ messages in thread From: Jan Seiffert @ 2012-04-02 13:02 UTC (permalink / raw) To: David Laight Cc: Matt Evans, Eric Dumazet, netdev, linux-kernel, linuxppc-dev, David S. Miller David Laight schrieb: > >> The function is renamed to make it a little more clear what it does. >> It is not added to any .h because it is not for general >> consumption, only for bpf internal use (and so by the jits). > > I'd have thought it better to put in into a bfp_internal.h > (or similar) with a big warning there about the asm users. > Hmmm, OK, where would i put the .h? Right there under ./include/linux/? > Possibly even worth adding some other defs that the asm > files will need (if there are any). > There is at least one other define, the lowest negative address range, but it would be a copy of the same define in filter.h, or i would have to massage filter.h to make it fit for inclusion by assembly. So I'm unsure how to proceed. > David > Greetings Jan -- An IPv4 address space walks into a bar: "A strong CIDR please. I'm exhausted." ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits 2012-03-30 15:08 ` [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits Jan Seiffert 2012-03-30 18:56 ` Eric Dumazet 2012-04-02 9:18 ` David Laight @ 2012-04-03 22:02 ` David Miller 2 siblings, 0 replies; 22+ messages in thread From: David Miller @ 2012-04-03 22:02 UTC (permalink / raw) To: kaffeemonster; +Cc: netdev, eric.dumazet, linuxppc-dev, linux-kernel, matt From: Jan Seiffert <kaffeemonster@googlemail.com> Date: Fri, 30 Mar 2012 17:08:19 +0200 > The function is renamed to make it a little more clear what it does. > It is not added to any .h because it is not for general consumption, only for > bpf internal use (and so by the jits). > > Signed-of-by: Jan Seiffert <kaffeemonster@googlemail.com> Applied but with comment formatting fixed up: > +/* > + * No hurry in this branch > + * > + * Exported for the bpf jit load helper. > + */ Like this: > +/* No hurry in this branch > + * > + * Exported for the bpf jit load helper. > + */ ^ permalink raw reply [flat|nested] 22+ messages in thread
* [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-03-30 15:00 [REGRESSION][PATCH V4 0/3] bpf jit drops the ball on negative memory references Jan Seiffert 2012-03-30 15:08 ` [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits Jan Seiffert @ 2012-03-30 15:35 ` Jan Seiffert 2012-04-03 22:03 ` David Miller 1 sibling, 1 reply; 22+ messages in thread From: Jan Seiffert @ 2012-03-30 15:35 UTC (permalink / raw) To: netdev Cc: Eric Dumazet, linuxppc-dev, linux-kernel, Matt Evans, David S. Miller Now the helper function from filter.c for negative offsets is exported, it can be used it in the jit to handle negative offsets. First modify the asm load helper functions to handle: - know positive offsets - know negative offsets - any offset then the compiler can be modified to explicitly use these helper when appropriate. This fixes the case of a negative X register and allows to lift the restriction that bpf programs with negative offsets can't be jited. Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com> I have only compile tested this, -ENOHARDWARE. Can someone with more powerpc kung-fu review and maybe test this? Esp. powerpc asm is not my strong point. I think i botched the stack frame in the call setup. Help? diff --git a/arch/powerpc/net/bpf_jit_64.S b/arch/powerpc/net/bpf_jit_64.S index ff4506e..e590aa5 100644 --- a/arch/powerpc/net/bpf_jit_64.S +++ b/arch/powerpc/net/bpf_jit_64.S @@ -31,14 +31,13 @@ * then branch directly to slow_path_XXX if required. (In fact, could * load a spare GPR with the address of slow_path_generic and pass size * as an argument, making the call site a mtlr, li and bllr.) - * - * Technically, the "is addr < 0" check is unnecessary & slowing down - * the ABS path, as it's statically checked on generation. */ .globl sk_load_word sk_load_word: cmpdi r_addr, 0 - blt bpf_error + blt bpf_slow_path_word_neg + .globl sk_load_word_positive_offset +sk_load_word_positive_offset: /* Are we accessing past headlen? */ subi r_scratch1, r_HL, 4 cmpd r_scratch1, r_addr @@ -51,7 +50,9 @@ sk_load_word: .globl sk_load_half sk_load_half: cmpdi r_addr, 0 - blt bpf_error + blt bpf_slow_path_half_neg + .globl sk_load_half_positive_offset +sk_load_half_positive_offset: subi r_scratch1, r_HL, 2 cmpd r_scratch1, r_addr blt bpf_slow_path_half @@ -61,7 +62,9 @@ sk_load_half: .globl sk_load_byte sk_load_byte: cmpdi r_addr, 0 - blt bpf_error + blt bpf_slow_path_byte_neg + .globl sk_load_byte_positive_offset +sk_load_byte_positive_offset: cmpd r_HL, r_addr ble bpf_slow_path_byte lbzx r_A, r_D, r_addr @@ -69,22 +72,20 @@ sk_load_byte: /* * BPF_S_LDX_B_MSH: ldxb 4*([offset]&0xf) - * r_addr is the offset value, already known positive + * r_addr is the offset value */ .globl sk_load_byte_msh sk_load_byte_msh: + cmpdi r_addr, 0 + blt bpf_slow_path_byte_msh_neg + .globl sk_load_byte_msh_positive_offset +sk_load_byte_msh_positive_offset: cmpd r_HL, r_addr ble bpf_slow_path_byte_msh lbzx r_X, r_D, r_addr rlwinm r_X, r_X, 2, 32-4-2, 31-2 blr -bpf_error: - /* Entered with cr0 = lt */ - li r3, 0 - /* Generated code will 'blt epilogue', returning 0. */ - blr - /* Call out to skb_copy_bits: * We'll need to back up our volatile regs first; we have * local variable space at r1+(BPF_PPC_STACK_BASIC). @@ -136,3 +137,85 @@ bpf_slow_path_byte_msh: lbz r_X, BPF_PPC_STACK_BASIC+(2*8)(r1) rlwinm r_X, r_X, 2, 32-4-2, 31-2 blr + +/* Call out to bpf_internal_load_pointer_neg_helper: + * We'll need to back up our volatile regs first; we have + * local variable space at r1+(BPF_PPC_STACK_BASIC). + * Allocate a new stack frame here to remain ABI-compliant in + * stashing LR. + */ +#define sk_negative_common(SIZE) \ + mflr r0; \ + std r0, 16(r1); \ + /* R3 goes in parameter space of caller's frame */ \ + std r_skb, (BPF_PPC_STACKFRAME+48)(r1); \ + std r_A, (BPF_PPC_STACK_BASIC+(0*8))(r1); \ + std r_X, (BPF_PPC_STACK_BASIC+(1*8))(r1); \ + stdu r1, -BPF_PPC_SLOWPATH_FRAME(r1); \ + /* R3 = r_skb, as passed */ \ + mr r4, r_addr; \ + li r5, SIZE; \ + bl bpf_internal_load_pointer_neg_helper; \ + /* R3 != 0 on success */ \ + addi r1, r1, BPF_PPC_SLOWPATH_FRAME; \ + ld r0, 16(r1); \ + ld r_A, (BPF_PPC_STACK_BASIC+(0*8))(r1); \ + ld r_X, (BPF_PPC_STACK_BASIC+(1*8))(r1); \ + mtlr r0; \ + cmpldi r3, 0; \ + beq bpf_error_slow; /* cr0 = EQ */ \ + mr r_addr, r3; \ + ld r_skb, (BPF_PPC_STACKFRAME+48)(r1); \ + /* Great success! */ + +bpf_slow_path_word_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_word_negative_offset +sk_load_word_negative_offset: + sk_negative_common(4) + lwz r_A, 0(r_addr) + blr + +bpf_slow_path_half_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_half_negative_offset +sk_load_half_negative_offset: + sk_negative_common(2) + lhz r_A, 0(r_addr) + blr + +bpf_slow_path_byte_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_byte_negative_offset +sk_load_byte_negative_offset: + sk_negative_common(1) + lbz r_A, 0(r_addr) + blr + +bpf_slow_path_byte_msh_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_byte_msh_negative_offset +sk_load_byte_msh_negative_offset: + sk_negative_common(1) + lbz r_X, 0(r_addr) + rlwinm r_X, r_X, 2, 32-4-2, 31-2 + blr + +bpf_error_slow: + /* fabricate a cr0 = lt */ + li r_scratch1, -1 + cmpdi r_scratch1, 0 +bpf_error: + /* Entered with cr0 = lt */ + li r3, 0 + /* Generated code will 'blt epilogue', returning 0. */ + blr + diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c index 73619d3..2dc8b14 100644 --- a/arch/powerpc/net/bpf_jit_comp.c +++ b/arch/powerpc/net/bpf_jit_comp.c @@ -127,6 +127,9 @@ static void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx) PPC_BLR(); } +#define CHOOSE_LOAD_FUNC(K, func) \ + ((int)K < 0 ? ((int)K >= SKF_LL_OFF ? func##_negative_offset : func) : func##_positive_offset) + /* Assemble the body code between the prologue & epilogue. */ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, struct codegen_context *ctx, @@ -391,21 +394,16 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, /*** Absolute loads from packet header/data ***/ case BPF_S_LD_W_ABS: - func = sk_load_word; + func = CHOOSE_LOAD_FUNC(K, sk_load_word); goto common_load; case BPF_S_LD_H_ABS: - func = sk_load_half; + func = CHOOSE_LOAD_FUNC(K, sk_load_half); goto common_load; case BPF_S_LD_B_ABS: - func = sk_load_byte; + func = CHOOSE_LOAD_FUNC(K, sk_load_byte); common_load: - /* - * Load from [K]. Reference with the (negative) - * SKF_NET_OFF/SKF_LL_OFF offsets is unsupported. - */ + /* Load from [K]. */ ctx->seen |= SEEN_DATAREF; - if ((int)K < 0) - return -ENOTSUPP; PPC_LI64(r_scratch1, func); PPC_MTLR(r_scratch1); PPC_LI32(r_addr, K); @@ -429,7 +427,7 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, common_load_ind: /* * Load from [X + K]. Negative offsets are tested for - * in the helper functions, and result in a 'ret 0'. + * in the helper functions. */ ctx->seen |= SEEN_DATAREF | SEEN_XREG; PPC_LI64(r_scratch1, func); @@ -443,13 +441,7 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, break; case BPF_S_LDX_B_MSH: - /* - * x86 version drops packet (RET 0) when K<0, whereas - * interpreter does allow K<0 (__load_pointer, special - * ancillary data). common_load returns ENOTSUPP if K<0, - * so we fall back to interpreter & filter works. - */ - func = sk_load_byte_msh; + func = CHOOSE_LOAD_FUNC(K, sk_load_byte_msh); goto common_load; break; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-03-30 15:35 ` [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets Jan Seiffert @ 2012-04-03 22:03 ` David Miller 2012-04-03 22:11 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 22+ messages in thread From: David Miller @ 2012-04-03 22:03 UTC (permalink / raw) To: kaffeemonster; +Cc: netdev, eric.dumazet, linuxppc-dev, linux-kernel, matt From: Jan Seiffert <kaffeemonster@googlemail.com> Date: Fri, 30 Mar 2012 17:35:01 +0200 > Now the helper function from filter.c for negative offsets is exported, > it can be used it in the jit to handle negative offsets. > > First modify the asm load helper functions to handle: > - know positive offsets > - know negative offsets > - any offset > > then the compiler can be modified to explicitly use these helper > when appropriate. > > This fixes the case of a negative X register and allows to lift > the restriction that bpf programs with negative offsets can't > be jited. > > Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com> > > I have only compile tested this, -ENOHARDWARE. > Can someone with more powerpc kung-fu review and maybe test this? > Esp. powerpc asm is not my strong point. I think i botched the > stack frame in the call setup. Help? I'm not applying this until a powerpc person tests it. Also, we have an ARM JIT in the tree which probably needs to be fixed similarly. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-03 22:03 ` David Miller @ 2012-04-03 22:11 ` Benjamin Herrenschmidt 2012-04-30 2:43 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-03 22:11 UTC (permalink / raw) To: David Miller Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev On Tue, 2012-04-03 at 18:03 -0400, David Miller wrote: > > Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com> > > > > I have only compile tested this, -ENOHARDWARE. > > Can someone with more powerpc kung-fu review and maybe test this? > > Esp. powerpc asm is not my strong point. I think i botched the > > stack frame in the call setup. Help? > > I'm not applying this until a powerpc person tests it. > > Also, we have an ARM JIT in the tree which probably needs to > be fixed similarly. Matt's having a look at powerpc Cheers, Ben. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-03 22:11 ` Benjamin Herrenschmidt @ 2012-04-30 2:43 ` Benjamin Herrenschmidt 2012-04-30 3:40 ` Benjamin Herrenschmidt ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-30 2:43 UTC (permalink / raw) To: David Miller Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev On Wed, 2012-04-04 at 08:11 +1000, Benjamin Herrenschmidt wrote: > On Tue, 2012-04-03 at 18:03 -0400, David Miller wrote: > > > > Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com> > > > > > > I have only compile tested this, -ENOHARDWARE. > > > Can someone with more powerpc kung-fu review and maybe test this? > > > Esp. powerpc asm is not my strong point. I think i botched the > > > stack frame in the call setup. Help? > > > > I'm not applying this until a powerpc person tests it. > > > > Also, we have an ARM JIT in the tree which probably needs to > > be fixed similarly. > > Matt's having a look at powerpc Ok, he hasn't so I'll dig a bit. No obvious wrongness (but I'm not very familiar with bpf), though I do have a comment: sk_negative_common() and bpf_slow_path_common() should be made one and single macro which takes the fallback function as an argument. I'll mess around & try to test using Jan test case & will come back with an updated patch. Cheers, Ben. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 2:43 ` Benjamin Herrenschmidt @ 2012-04-30 3:40 ` Benjamin Herrenschmidt 2012-04-30 3:43 ` Jan Seiffert 2012-04-30 4:11 ` Benjamin Herrenschmidt 2 siblings, 0 replies; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-30 3:40 UTC (permalink / raw) To: David Miller Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: > Ok, he hasn't so I'll dig a bit. > > No obvious wrongness (but I'm not very familiar with bpf), though I do > have a comment: sk_negative_common() and bpf_slow_path_common() should > be made one and single macro which takes the fallback function as an > argument. > > I'll mess around & try to test using Jan test case & will come back > with an updated patch. Wow, hit that nasty along the way: The test program will not work on big endian machines because of a nasty difference between the kernel struct sock_fprog and libpcap struct bpf_program: Kernel expects: struct sock_fprog { /* Required for SO_ATTACH_FILTER. */ unsigned short len; /* Number of filter blocks */ struct sock_filter __user *filter; }; libpcap provides: struct bpf_program { u_int bf_len; struct bpf_insn *bf_insns; }; Note the unsigned short vs. unsigned int there ? This totally breaks it here. Is it expected that one can pass a struct bpf_program directly to the kernel or should it be "converted" by the library in which case it's just a bug in Jan's test program ? Cheers, Ben. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 2:43 ` Benjamin Herrenschmidt 2012-04-30 3:40 ` Benjamin Herrenschmidt @ 2012-04-30 3:43 ` Jan Seiffert 2012-04-30 4:11 ` Benjamin Herrenschmidt 2 siblings, 0 replies; 22+ messages in thread From: Jan Seiffert @ 2012-04-30 3:43 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev, David Miller Benjamin Herrenschmidt schrieb: > On Wed, 2012-04-04 at 08:11 +1000, Benjamin Herrenschmidt wrote: >> On Tue, 2012-04-03 at 18:03 -0400, David Miller wrote: >> >>>> Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com> >>>> >>>> I have only compile tested this, -ENOHARDWARE. Can someone with >>>> more powerpc kung-fu review and maybe test this? Esp. powerpc >>>> asm is not my strong point. I think i botched the stack frame >>>> in the call setup. Help? >>> >>> I'm not applying this until a powerpc person tests it. >>> >>> Also, we have an ARM JIT in the tree which probably needs to be >>> fixed similarly. >> >> Matt's having a look at powerpc > > Ok, he hasn't so I'll dig a bit. > That would be great Benjamin! > No obvious wrongness (but I'm not very familiar with bpf), As long as you know PPC ASM you are my man ;-) > though I do have a comment: sk_negative_common() and > bpf_slow_path_common() should be made one and single macro which > takes the fallback function as an argument. > I don't know if this is possible. The return value is different (one returns 0 on success, the other != 0, the return value of != is needed). I didn't wanted to change to much, because i'm not fluent in ppc. > I'll mess around & try to test using Jan test case & will come back > with an updated patch. > Would be great! > Cheers, Ben. > Greetings Jan -- A UDP packet walks into a ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 2:43 ` Benjamin Herrenschmidt 2012-04-30 3:40 ` Benjamin Herrenschmidt 2012-04-30 3:43 ` Jan Seiffert @ 2012-04-30 4:11 ` Benjamin Herrenschmidt 2012-04-30 4:27 ` Jan Seiffert 2012-04-30 5:02 ` [REGRESSION][PATCH V5 " Jan Seiffert 2 siblings, 2 replies; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-30 4:11 UTC (permalink / raw) To: David Miller Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: > > Matt's having a look at powerpc > > Ok, he hasn't so I'll dig a bit. > > No obvious wrongness (but I'm not very familiar with bpf), though I do > have a comment: sk_negative_common() and bpf_slow_path_common() should > be made one and single macro which takes the fallback function as an > argument. Ok, with the compile fix below it seems to work for me: (Feel free to fold that into the original patch) diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h index af1ab5e..5c3cf2d 100644 --- a/arch/powerpc/net/bpf_jit.h +++ b/arch/powerpc/net/bpf_jit.h @@ -48,7 +48,13 @@ /* * Assembly helpers from arch/powerpc/net/bpf_jit.S: */ -extern u8 sk_load_word[], sk_load_half[], sk_load_byte[], sk_load_byte_msh[]; +#define DECLARE_LOAD_FUNC(func) \ + extern u8 func[], func##_negative_offset[], func##_positive_offset[] + +DECLARE_LOAD_FUNC(sk_load_word); +DECLARE_LOAD_FUNC(sk_load_half); +DECLARE_LOAD_FUNC(sk_load_byte); +DECLARE_LOAD_FUNC(sk_load_byte_msh); #define FUNCTION_DESCR_SIZE 24 Cheers, Ben. ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 4:11 ` Benjamin Herrenschmidt @ 2012-04-30 4:27 ` Jan Seiffert 2012-04-30 4:29 ` Benjamin Herrenschmidt 2012-04-30 5:02 ` [REGRESSION][PATCH V5 " Jan Seiffert 1 sibling, 1 reply; 22+ messages in thread From: Jan Seiffert @ 2012-04-30 4:27 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev, David Miller Benjamin Herrenschmidt schrieb: > On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: > >>> Matt's having a look at powerpc >> >> Ok, he hasn't so I'll dig a bit. >> >> No obvious wrongness (but I'm not very familiar with bpf), though I do >> have a comment: sk_negative_common() and bpf_slow_path_common() should >> be made one and single macro which takes the fallback function as an >> argument. > > Ok, with the compile fix below it seems to work for me: > > (Feel free to fold that into the original patch) > Should i resend the complete patch with the compile fix? [snip] > > Cheers, > Ben. > Greetings Jan PS: I am sure i compile tested the orig. patch here, hmmm, must have lost that part when moving trees, #GitIsNotMyFriend -- en.gin.eer en-ji-nir n 1: a mechanism for converting caffeine into designs. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 4:27 ` Jan Seiffert @ 2012-04-30 4:29 ` Benjamin Herrenschmidt 2012-04-30 4:43 ` Jan Seiffert 0 siblings, 1 reply; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-30 4:29 UTC (permalink / raw) To: kaffeemonster Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev, David Miller On Mon, 2012-04-30 at 06:27 +0200, Jan Seiffert wrote: > Benjamin Herrenschmidt schrieb: > > On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: > > > >>> Matt's having a look at powerpc > >> > >> Ok, he hasn't so I'll dig a bit. > >> > >> No obvious wrongness (but I'm not very familiar with bpf), though I do > >> have a comment: sk_negative_common() and bpf_slow_path_common() should > >> be made one and single macro which takes the fallback function as an > >> argument. > > > > Ok, with the compile fix below it seems to work for me: > > > > (Feel free to fold that into the original patch) > > > > Should i resend the complete patch with the compile fix? Won't hurt... BTW. Any idea about that bpf_program vs. sock_fprog issue I mentioned earlier ? Cheers, Ben. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 4:29 ` Benjamin Herrenschmidt @ 2012-04-30 4:43 ` Jan Seiffert 2012-04-30 5:26 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 22+ messages in thread From: Jan Seiffert @ 2012-04-30 4:43 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev, David Miller Benjamin Herrenschmidt schrieb: > On Mon, 2012-04-30 at 06:27 +0200, Jan Seiffert wrote: >> Benjamin Herrenschmidt schrieb: >>> On Mon, 2012-04-30 at 12:43 +1000, Benjamin Herrenschmidt wrote: >>> >>>>> Matt's having a look at powerpc >>>> >>>> Ok, he hasn't so I'll dig a bit. >>>> >>>> No obvious wrongness (but I'm not very familiar with bpf), though I do >>>> have a comment: sk_negative_common() and bpf_slow_path_common() should >>>> be made one and single macro which takes the fallback function as an >>>> argument. >>> >>> Ok, with the compile fix below it seems to work for me: >>> >>> (Feel free to fold that into the original patch) >>> >> >> Should i resend the complete patch with the compile fix? > > Won't hurt... > Ok > BTW. Any idea about that bpf_program vs. sock_fprog issue I mentioned > earlier ? > No idea, i was going by the old saying: "Thou shall not include kernel header, or you will feel the wrath of angry kernel gurus." > Cheers, > Ben. > Greetings Jan -- The OO-Hype keeps on spinning, C stays. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 4:43 ` Jan Seiffert @ 2012-04-30 5:26 ` Benjamin Herrenschmidt 2012-04-30 17:41 ` David Miller 0 siblings, 1 reply; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-30 5:26 UTC (permalink / raw) To: kaffeemonster Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev, David Miller > No idea, i was going by the old saying: > "Thou shall not include kernel header, or you will feel the wrath of angry > kernel gurus." Heh :-) Well, googling around, it looks like there's a mix of both type of programs out there. Those using a struct bpf_program and those using a struct soft_fprog. Only the latter will work on BE machines as far as I can tell. David, what's the right way to fix that ? Cheers, Ben. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 5:26 ` Benjamin Herrenschmidt @ 2012-04-30 17:41 ` David Miller 2012-04-30 21:55 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 22+ messages in thread From: David Miller @ 2012-04-30 17:41 UTC (permalink / raw) To: benh; +Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: Mon, 30 Apr 2012 15:26:08 +1000 > David, what's the right way to fix that ? There is no doubt that sock_fprog is the correct datastructure to use. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 17:41 ` David Miller @ 2012-04-30 21:55 ` Benjamin Herrenschmidt 2012-04-30 21:57 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-30 21:55 UTC (permalink / raw) To: David Miller Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev On Mon, 2012-04-30 at 13:41 -0400, David Miller wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Date: Mon, 30 Apr 2012 15:26:08 +1000 > > > David, what's the right way to fix that ? > > There is no doubt that sock_fprog is the correct datastructure to use. Ok, so the right fix is to email anybody who posted code using struct bpf_program to fix their code ? :-) My question was more along the lines of should we attempt changing one of the two variants to make them match on BE (since they are in effect compatible on LE), tho of course this could have the usual annoying consequence of breaking the mangled c++ name of the symbol). >From your reply I assume the answer is no... so that leaves us to chase up users and fix them. Great.... Cheers, Ben. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 21:55 ` Benjamin Herrenschmidt @ 2012-04-30 21:57 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 22+ messages in thread From: Benjamin Herrenschmidt @ 2012-04-30 21:57 UTC (permalink / raw) To: David Miller Cc: kaffeemonster, eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev On Tue, 2012-05-01 at 07:55 +1000, Benjamin Herrenschmidt wrote: > On Mon, 2012-04-30 at 13:41 -0400, David Miller wrote: > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Date: Mon, 30 Apr 2012 15:26:08 +1000 > > > > > David, what's the right way to fix that ? > > > > There is no doubt that sock_fprog is the correct datastructure to use. > > Ok, so the right fix is to email anybody who posted code using struct > bpf_program to fix their code ? :-) Actually, the right fix is for anybody using pcap-bpf.h to not use SO_ATTACH_FILTER directly but to use pcap_setfilter() which handles the compatibility. I'll start spamming web sites who tell people to do the wrong thing. Cheers, Ben. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [REGRESSION][PATCH V5 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 4:11 ` Benjamin Herrenschmidt 2012-04-30 4:27 ` Jan Seiffert @ 2012-04-30 5:02 ` Jan Seiffert 2012-04-30 17:41 ` David Miller 1 sibling, 1 reply; 22+ messages in thread From: Jan Seiffert @ 2012-04-30 5:02 UTC (permalink / raw) To: netdev; +Cc: eric.dumazet, matt, linux-kernel, linuxppc-dev, David Miller Now the helper function from filter.c for negative offsets is exported, it can be used it in the jit to handle negative offsets. First modify the asm load helper functions to handle: - know positive offsets - know negative offsets - any offset then the compiler can be modified to explicitly use these helper when appropriate. This fixes the case of a negative X register and allows to lift the restriction that bpf programs with negative offsets can't be jited. Tested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com> --- arch/powerpc/net/bpf_jit.h | 8 +++- arch/powerpc/net/bpf_jit_64.S | 108 ++++++++++++++++++++++++++++++++++----- arch/powerpc/net/bpf_jit_comp.c | 26 +++------ 3 files changed, 111 insertions(+), 31 deletions(-) diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h index af1ab5e..5c3cf2d 100644 --- a/arch/powerpc/net/bpf_jit.h +++ b/arch/powerpc/net/bpf_jit.h @@ -48,7 +48,13 @@ /* * Assembly helpers from arch/powerpc/net/bpf_jit.S: */ -extern u8 sk_load_word[], sk_load_half[], sk_load_byte[], sk_load_byte_msh[]; +#define DECLARE_LOAD_FUNC(func) \ + extern u8 func[], func##_negative_offset[], func##_positive_offset[] + +DECLARE_LOAD_FUNC(sk_load_word); +DECLARE_LOAD_FUNC(sk_load_half); +DECLARE_LOAD_FUNC(sk_load_byte); +DECLARE_LOAD_FUNC(sk_load_byte_msh); #define FUNCTION_DESCR_SIZE 24 diff --git a/arch/powerpc/net/bpf_jit_64.S b/arch/powerpc/net/bpf_jit_64.S index ff4506e..55ba385 100644 --- a/arch/powerpc/net/bpf_jit_64.S +++ b/arch/powerpc/net/bpf_jit_64.S @@ -31,14 +31,13 @@ * then branch directly to slow_path_XXX if required. (In fact, could * load a spare GPR with the address of slow_path_generic and pass size * as an argument, making the call site a mtlr, li and bllr.) - * - * Technically, the "is addr < 0" check is unnecessary & slowing down - * the ABS path, as it's statically checked on generation. */ .globl sk_load_word sk_load_word: cmpdi r_addr, 0 - blt bpf_error + blt bpf_slow_path_word_neg + .globl sk_load_word_positive_offset +sk_load_word_positive_offset: /* Are we accessing past headlen? */ subi r_scratch1, r_HL, 4 cmpd r_scratch1, r_addr @@ -51,7 +50,9 @@ sk_load_word: .globl sk_load_half sk_load_half: cmpdi r_addr, 0 - blt bpf_error + blt bpf_slow_path_half_neg + .globl sk_load_half_positive_offset +sk_load_half_positive_offset: subi r_scratch1, r_HL, 2 cmpd r_scratch1, r_addr blt bpf_slow_path_half @@ -61,7 +62,9 @@ sk_load_half: .globl sk_load_byte sk_load_byte: cmpdi r_addr, 0 - blt bpf_error + blt bpf_slow_path_byte_neg + .globl sk_load_byte_positive_offset +sk_load_byte_positive_offset: cmpd r_HL, r_addr ble bpf_slow_path_byte lbzx r_A, r_D, r_addr @@ -69,22 +72,20 @@ sk_load_byte: /* * BPF_S_LDX_B_MSH: ldxb 4*([offset]&0xf) - * r_addr is the offset value, already known positive + * r_addr is the offset value */ .globl sk_load_byte_msh sk_load_byte_msh: + cmpdi r_addr, 0 + blt bpf_slow_path_byte_msh_neg + .globl sk_load_byte_msh_positive_offset +sk_load_byte_msh_positive_offset: cmpd r_HL, r_addr ble bpf_slow_path_byte_msh lbzx r_X, r_D, r_addr rlwinm r_X, r_X, 2, 32-4-2, 31-2 blr -bpf_error: - /* Entered with cr0 = lt */ - li r3, 0 - /* Generated code will 'blt epilogue', returning 0. */ - blr - /* Call out to skb_copy_bits: * We'll need to back up our volatile regs first; we have * local variable space at r1+(BPF_PPC_STACK_BASIC). @@ -136,3 +137,84 @@ bpf_slow_path_byte_msh: lbz r_X, BPF_PPC_STACK_BASIC+(2*8)(r1) rlwinm r_X, r_X, 2, 32-4-2, 31-2 blr + +/* Call out to bpf_internal_load_pointer_neg_helper: + * We'll need to back up our volatile regs first; we have + * local variable space at r1+(BPF_PPC_STACK_BASIC). + * Allocate a new stack frame here to remain ABI-compliant in + * stashing LR. + */ +#define sk_negative_common(SIZE) \ + mflr r0; \ + std r0, 16(r1); \ + /* R3 goes in parameter space of caller's frame */ \ + std r_skb, (BPF_PPC_STACKFRAME+48)(r1); \ + std r_A, (BPF_PPC_STACK_BASIC+(0*8))(r1); \ + std r_X, (BPF_PPC_STACK_BASIC+(1*8))(r1); \ + stdu r1, -BPF_PPC_SLOWPATH_FRAME(r1); \ + /* R3 = r_skb, as passed */ \ + mr r4, r_addr; \ + li r5, SIZE; \ + bl bpf_internal_load_pointer_neg_helper; \ + /* R3 != 0 on success */ \ + addi r1, r1, BPF_PPC_SLOWPATH_FRAME; \ + ld r0, 16(r1); \ + ld r_A, (BPF_PPC_STACK_BASIC+(0*8))(r1); \ + ld r_X, (BPF_PPC_STACK_BASIC+(1*8))(r1); \ + mtlr r0; \ + cmpldi r3, 0; \ + beq bpf_error_slow; /* cr0 = EQ */ \ + mr r_addr, r3; \ + ld r_skb, (BPF_PPC_STACKFRAME+48)(r1); \ + /* Great success! */ + +bpf_slow_path_word_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_word_negative_offset +sk_load_word_negative_offset: + sk_negative_common(4) + lwz r_A, 0(r_addr) + blr + +bpf_slow_path_half_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_half_negative_offset +sk_load_half_negative_offset: + sk_negative_common(2) + lhz r_A, 0(r_addr) + blr + +bpf_slow_path_byte_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_byte_negative_offset +sk_load_byte_negative_offset: + sk_negative_common(1) + lbz r_A, 0(r_addr) + blr + +bpf_slow_path_byte_msh_neg: + lis r_scratch1,-32 /* SKF_LL_OFF */ + cmpd r_addr, r_scratch1 /* addr < SKF_* */ + blt bpf_error /* cr0 = LT */ + .globl sk_load_byte_msh_negative_offset +sk_load_byte_msh_negative_offset: + sk_negative_common(1) + lbz r_X, 0(r_addr) + rlwinm r_X, r_X, 2, 32-4-2, 31-2 + blr + +bpf_error_slow: + /* fabricate a cr0 = lt */ + li r_scratch1, -1 + cmpdi r_scratch1, 0 +bpf_error: + /* Entered with cr0 = lt */ + li r3, 0 + /* Generated code will 'blt epilogue', returning 0. */ + blr diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c index 73619d3..2dc8b14 100644 --- a/arch/powerpc/net/bpf_jit_comp.c +++ b/arch/powerpc/net/bpf_jit_comp.c @@ -127,6 +127,9 @@ static void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx) PPC_BLR(); } +#define CHOOSE_LOAD_FUNC(K, func) \ + ((int)K < 0 ? ((int)K >= SKF_LL_OFF ? func##_negative_offset : func) : func##_positive_offset) + /* Assemble the body code between the prologue & epilogue. */ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, struct codegen_context *ctx, @@ -391,21 +394,16 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, /*** Absolute loads from packet header/data ***/ case BPF_S_LD_W_ABS: - func = sk_load_word; + func = CHOOSE_LOAD_FUNC(K, sk_load_word); goto common_load; case BPF_S_LD_H_ABS: - func = sk_load_half; + func = CHOOSE_LOAD_FUNC(K, sk_load_half); goto common_load; case BPF_S_LD_B_ABS: - func = sk_load_byte; + func = CHOOSE_LOAD_FUNC(K, sk_load_byte); common_load: - /* - * Load from [K]. Reference with the (negative) - * SKF_NET_OFF/SKF_LL_OFF offsets is unsupported. - */ + /* Load from [K]. */ ctx->seen |= SEEN_DATAREF; - if ((int)K < 0) - return -ENOTSUPP; PPC_LI64(r_scratch1, func); PPC_MTLR(r_scratch1); PPC_LI32(r_addr, K); @@ -429,7 +427,7 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, common_load_ind: /* * Load from [X + K]. Negative offsets are tested for - * in the helper functions, and result in a 'ret 0'. + * in the helper functions. */ ctx->seen |= SEEN_DATAREF | SEEN_XREG; PPC_LI64(r_scratch1, func); @@ -443,13 +441,7 @@ static int bpf_jit_build_body(struct sk_filter *fp, u32 *image, break; case BPF_S_LDX_B_MSH: - /* - * x86 version drops packet (RET 0) when K<0, whereas - * interpreter does allow K<0 (__load_pointer, special - * ancillary data). common_load returns ENOTSUPP if K<0, - * so we fall back to interpreter & filter works. - */ - func = sk_load_byte_msh; + func = CHOOSE_LOAD_FUNC(K, sk_load_byte_msh); goto common_load; break; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [REGRESSION][PATCH V5 3/3] bpf jit: Let the powerpc jit handle negative offsets 2012-04-30 5:02 ` [REGRESSION][PATCH V5 " Jan Seiffert @ 2012-04-30 17:41 ` David Miller 0 siblings, 0 replies; 22+ messages in thread From: David Miller @ 2012-04-30 17:41 UTC (permalink / raw) To: kaffeemonster; +Cc: eric.dumazet, matt, netdev, linux-kernel, linuxppc-dev From: Jan Seiffert <kaffeemonster@googlemail.com> Date: Mon, 30 Apr 2012 07:02:19 +0200 > Now the helper function from filter.c for negative offsets is exported, > it can be used it in the jit to handle negative offsets. > > First modify the asm load helper functions to handle: > - know positive offsets > - know negative offsets > - any offset > > then the compiler can be modified to explicitly use these helper > when appropriate. > > This fixes the case of a negative X register and allows to lift > the restriction that bpf programs with negative offsets can't > be jited. > > Tested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Jan Seiffert <kaffeemonster@googlemail.com> Applied, thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-04-30 21:57 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-30 15:00 [REGRESSION][PATCH V4 0/3] bpf jit drops the ball on negative memory references Jan Seiffert 2012-03-30 15:08 ` [REGRESSION][PATCH V4 1/3] bpf jit: Make the filter.c::__load_pointer helper non-static for the jits Jan Seiffert 2012-03-30 18:56 ` Eric Dumazet 2012-04-02 9:18 ` David Laight 2012-04-02 13:02 ` Jan Seiffert 2012-04-03 22:02 ` David Miller 2012-03-30 15:35 ` [REGRESSION][PATCH V4 3/3] bpf jit: Let the powerpc jit handle negative offsets Jan Seiffert 2012-04-03 22:03 ` David Miller 2012-04-03 22:11 ` Benjamin Herrenschmidt 2012-04-30 2:43 ` Benjamin Herrenschmidt 2012-04-30 3:40 ` Benjamin Herrenschmidt 2012-04-30 3:43 ` Jan Seiffert 2012-04-30 4:11 ` Benjamin Herrenschmidt 2012-04-30 4:27 ` Jan Seiffert 2012-04-30 4:29 ` Benjamin Herrenschmidt 2012-04-30 4:43 ` Jan Seiffert 2012-04-30 5:26 ` Benjamin Herrenschmidt 2012-04-30 17:41 ` David Miller 2012-04-30 21:55 ` Benjamin Herrenschmidt 2012-04-30 21:57 ` Benjamin Herrenschmidt 2012-04-30 5:02 ` [REGRESSION][PATCH V5 " Jan Seiffert 2012-04-30 17:41 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).