* [PATCH net] sparc: bpf_jit: fix loads from negative offsets
@ 2014-09-23 20:50 Alexei Starovoitov
2014-09-24 19:08 ` David Miller
0 siblings, 1 reply; 3+ messages in thread
From: Alexei Starovoitov @ 2014-09-23 20:50 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
- fix BPF_LD|ABS|IND from negative offsets:
make sure to sign extend lower 32 bits in 64-bit register
before calling C helpers from JITed code, otherwise 'int k'
argument of bpf_internal_load_pointer_neg_helper() function
will be added as large unsigned integer, causing packet size
check to trigger and abort the program.
It's worth noting that JITed code for 'A = A op K' will affect
upper 32 bits differently depending whether K is simm13 or not.
Since small constants are sign extended, whereas large constants
are stored in temp register and zero extended.
That is ok and we don't have to pay a penalty of sign extension
for every sethi, since all classic BPF instructions have 32-bit
semantics and we only need to set correct upper bits when
transitioning from JITed code into C.
- though instructions 'A &= 0' and 'A *= 0' are odd, JIT compiler
should not optimize them out
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
More detailed explanation of 1st bug:
BPF_STMT(BPF_LD | BPF_B | BPF_ABS, SKF_LL_OFF)
will be JITed as:
sethi %hi(0xffe00000), %g3
or %g3, 0, %g3
call bpf_jit_load_byte_negative_offset
which will do:
mov %g3, %o1
call bpf_internal_load_pointer_neg_helper
mov 1, %o2
void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, unsigned int size)
{
...
ptr = skb_mac_header(skb) + k - SKF_LL_OFF;
here 'k' will be added as 0xffe00000 instead of SKF_LL_OFF == -0x200000
After these fixes, test_bpf is now clean on sparc64.
Compile tested only on sparc32
arch/sparc/net/bpf_jit_asm.S | 3 +++
arch/sparc/net/bpf_jit_comp.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/sparc/net/bpf_jit_asm.S b/arch/sparc/net/bpf_jit_asm.S
index 9d016c7..8c83f4b 100644
--- a/arch/sparc/net/bpf_jit_asm.S
+++ b/arch/sparc/net/bpf_jit_asm.S
@@ -6,10 +6,12 @@
#define SAVE_SZ 176
#define SCRATCH_OFF STACK_BIAS + 128
#define BE_PTR(label) be,pn %xcc, label
+#define SIGN_EXTEND(reg) sra reg, 0, reg
#else
#define SAVE_SZ 96
#define SCRATCH_OFF 72
#define BE_PTR(label) be label
+#define SIGN_EXTEND(reg)
#endif
#define SKF_MAX_NEG_OFF (-0x200000) /* SKF_LL_OFF from filter.h */
@@ -135,6 +137,7 @@ bpf_slow_path_byte_msh:
save %sp, -SAVE_SZ, %sp; \
mov %i0, %o0; \
mov r_OFF, %o1; \
+ SIGN_EXTEND(%o1); \
call bpf_internal_load_pointer_neg_helper; \
mov (LEN), %o2; \
mov %o0, r_TMP; \
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 51ae87b..ece4af0 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -184,7 +184,7 @@ do { \
*/
#define emit_alu_K(OPCODE, K) \
do { \
- if (K) { \
+ if (K || OPCODE == AND || OPCODE == MUL) { \
unsigned int _insn = OPCODE; \
_insn |= RS1(r_A) | RD(r_A); \
if (is_simm13(K)) { \
--
1.7.9.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] sparc: bpf_jit: fix loads from negative offsets
2014-09-23 20:50 [PATCH net] sparc: bpf_jit: fix loads from negative offsets Alexei Starovoitov
@ 2014-09-24 19:08 ` David Miller
2014-09-25 6:30 ` Alexei Starovoitov
0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2014-09-24 19:08 UTC (permalink / raw)
To: ast; +Cc: netdev
From: Alexei Starovoitov <ast@plumgrid.com>
Date: Tue, 23 Sep 2014 13:50:10 -0700
> - fix BPF_LD|ABS|IND from negative offsets:
> make sure to sign extend lower 32 bits in 64-bit register
> before calling C helpers from JITed code, otherwise 'int k'
> argument of bpf_internal_load_pointer_neg_helper() function
> will be added as large unsigned integer, causing packet size
> check to trigger and abort the program.
>
> It's worth noting that JITed code for 'A = A op K' will affect
> upper 32 bits differently depending whether K is simm13 or not.
> Since small constants are sign extended, whereas large constants
> are stored in temp register and zero extended.
> That is ok and we don't have to pay a penalty of sign extension
> for every sethi, since all classic BPF instructions have 32-bit
> semantics and we only need to set correct upper bits when
> transitioning from JITed code into C.
>
> - though instructions 'A &= 0' and 'A *= 0' are odd, JIT compiler
> should not optimize them out
>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
Applied, thanks for fixing this.
What we could do longer-term is change is_simm13() into is_simm12()
and therefore know that all constants loaded are 32-bit zero extended
regardless of size.
BTW, there is a trick to load arbitrary negative 32-bit constants sign
extended to 64-bit, in just two instructions:
sethi %(~value), reg
xor reg, ~(0x400 | (value & 0x3ff)), reg
Or something like that, it's been a while. GCC knows how to emit
that sequence too.
Anyways, we could adjust the JIT to do that too.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] sparc: bpf_jit: fix loads from negative offsets
2014-09-24 19:08 ` David Miller
@ 2014-09-25 6:30 ` Alexei Starovoitov
0 siblings, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2014-09-25 6:30 UTC (permalink / raw)
To: David Miller; +Cc: Network Development
On Wed, Sep 24, 2014 at 12:08 PM, David Miller <davem@davemloft.net> wrote:
>
> BTW, there is a trick to load arbitrary negative 32-bit constants sign
> extended to 64-bit, in just two instructions:
>
> sethi %(~value), reg
> xor reg, ~(0x400 | (value & 0x3ff)), reg
>
> Or something like that, it's been a while. GCC knows how to emit
> that sequence too.
nice, that's a neat trick.
gcc computes it as:
sethi %(~value), reg
xor reg, -0x400 | (value & 0x3ff), reg
and llvm as:
sethi %(~value), reg
xor reg, ~(~value & 0x3ff), reg
both are equivalent. I'll try to teach JIT to emit this.
Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-09-25 6:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-23 20:50 [PATCH net] sparc: bpf_jit: fix loads from negative offsets Alexei Starovoitov
2014-09-24 19:08 ` David Miller
2014-09-25 6:30 ` Alexei Starovoitov
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).