[PATCH 0/4] Seccomp filter JIT support on ARM.

[PATCH 0/4] Seccomp filter JIT support on ARM.

[Nicolas Schichan]

Greetings,

The following patches allow the use of the existing JIT code under
arch/arm for seccomp filters.

The first patch makes bpf_migrate_filter() available so that seccomp
code can use it.

The second patch invokes the classic BPF JIT code and if this fails
reverts to the internal BPF. The open coded double call to
bpf_convert_filter() and bpf_prog_select_runtime() is replaced by
bpf_migrate_filter()

The third patch adds support for loads from the seccomp_data structure in
the ARM BPF JIT code.

The fourth and final patch fixes a bug in the emit_udiv() function
when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
ARM BPF JIT code.

Regards,

Nicolas Schichan (4):
  net: filter: make bpf_migrate_filter available outside of
    net/core/filter.c
  seccomp: rework seccomp_prepare_filter().
  ARM: net: add JIT support for loads from struct seccomp_data.
  ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

 arch/arm/net/bpf_jit_32.c | 14 ++++++++++++--
 include/linux/filter.h    |  1 +
 kernel/seccomp.c          | 47 ++++++++++++++++++++---------------------------
 net/core/filter.c         |  2 +-
 4 files changed, 34 insertions(+), 30 deletions(-)

-- 
1.9.1

[PATCH 1/4] net: filter: make bpf_migrate_filter available outside of net/core/filter.c

[Nicolas Schichan]

This is in preparation of its use in the seccomp code.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 include/linux/filter.h | 1 +
 net/core/filter.c      | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index caac208..7e0811b 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -379,6 +379,7 @@ static inline void bpf_prog_unlock_free(struct bpf_prog *fp)
 
 int bpf_prog_create(struct bpf_prog **pfp, struct sock_fprog_kern *fprog);
 void bpf_prog_destroy(struct bpf_prog *fp);
+struct bpf_prog *bpf_migrate_filter(struct bpf_prog *fp);
 
 int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
 int sk_attach_bpf(u32 ufd, struct sock *sk);
diff --git a/net/core/filter.c b/net/core/filter.c
index f6bdc2b..b8ce66e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -876,7 +876,7 @@ bool sk_filter_charge(struct sock *sk, struct sk_filter *fp)
 	return false;
 }
 
-static struct bpf_prog *bpf_migrate_filter(struct bpf_prog *fp)
+struct bpf_prog *bpf_migrate_filter(struct bpf_prog *fp)
 {
 	struct sock_filter *old_prog;
 	struct bpf_prog *old_fp;
-- 
1.9.1

[PATCH 2/4] seccomp: rework seccomp_prepare_filter().

[Nicolas Schichan]

- Try to use the classic BPF JIT via bpf_jit_compile().

- Use bpf_migrate_filter() from NET filter code instead of the double
  bpf_convert_filter() followed by bpf_prog_select_runtime() if
  classic bpf_jit_compile() did not succeed in producing native code.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 kernel/seccomp.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 4f44028..4f9fea7 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -348,8 +348,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 {
 	struct seccomp_filter *filter;
 	unsigned long fp_size;
-	struct sock_filter *fp;
-	int new_len;
+	struct bpf_prog *fp;
 	long ret;
 
 	if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
@@ -368,29 +367,38 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 				     CAP_SYS_ADMIN) != 0)
 		return ERR_PTR(-EACCES);
 
-	fp = kzalloc(fp_size, GFP_KERNEL|__GFP_NOWARN);
+	fp = bpf_prog_alloc(bpf_prog_size(fprog->len), __GFP_NOWARN);
 	if (!fp)
 		return ERR_PTR(-ENOMEM);
 
 	/* Copy the instructions from fprog. */
 	ret = -EFAULT;
-	if (copy_from_user(fp, fprog->filter, fp_size))
+	if (copy_from_user(fp->insns, fprog->filter, fp_size))
 		goto free_prog;
+	fp->len = fprog->len;
 
 	/* Check and rewrite the fprog via the skb checker */
-	ret = bpf_check_classic(fp, fprog->len);
+	ret = bpf_check_classic(fp->insns, fp->len);
 	if (ret)
 		goto free_prog;
 
 	/* Check and rewrite the fprog for seccomp use */
-	ret = seccomp_check_filter(fp, fprog->len);
+	ret = seccomp_check_filter(fp->insns, fp->len);
 	if (ret)
 		goto free_prog;
 
-	/* Convert 'sock_filter' insns to 'bpf_insn' insns */
-	ret = bpf_convert_filter(fp, fprog->len, NULL, &new_len);
-	if (ret)
-		goto free_prog;
+	/* try to use the classic JIT */
+	bpf_jit_compile(fp);
+
+	if (!fp->jited) {
+		/*
+		 * if classic JIT has failed, try to convert it to an
+		 * internal filter
+		 */
+		fp = bpf_migrate_filter(fp);
+		if (IS_ERR(fp))
+			return fp;
+	}
 
 	/* Allocate a new seccomp_filter */
 	ret = -ENOMEM;
@@ -399,28 +407,13 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 	if (!filter)
 		goto free_prog;
 
-	filter->prog = bpf_prog_alloc(bpf_prog_size(new_len), __GFP_NOWARN);
-	if (!filter->prog)
-		goto free_filter;
-
-	ret = bpf_convert_filter(fp, fprog->len, filter->prog->insnsi, &new_len);
-	if (ret)
-		goto free_filter_prog;
-
-	kfree(fp);
+	filter->prog = fp;
 	atomic_set(&filter->usage, 1);
-	filter->prog->len = new_len;
-
-	bpf_prog_select_runtime(filter->prog);
 
 	return filter;
 
-free_filter_prog:
-	__bpf_prog_free(filter->prog);
-free_filter:
-	kfree(filter);
 free_prog:
-	kfree(fp);
+	bpf_prog_destroy(fp);
 	return ERR_PTR(ret);
 }
 
-- 
1.9.1

[PATCH 3/4] ARM: net: add JIT support for loads from struct seccomp_data.

[Nicolas Schichan]

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/net/bpf_jit_32.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index e1268f9..b5f470d 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -852,6 +852,16 @@ b_epilogue:
 			off = offsetof(struct sk_buff, queue_mapping);
 			emit(ARM_LDRH_I(r_A, r_skb, off), ctx);
 			break;
+		case BPF_LDX | BPF_W | BPF_ABS:
+			/*
+			 * load a 32bit word from struct seccomp_data.
+			 * seccomp_check_filter() will already have checked
+			 * that k is 32bit aligned and lies within the
+			 * struct seccomp_data.
+			 */
+			ctx->seen |= SEEN_SKB;
+			emit(ARM_LDR_I(r_A, r_skb, k), ctx);
+			break;
 		default:
 			return -1;
 		}
-- 
1.9.1

[PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Nicolas Schichan]

In that case, emit_udiv() will be called with rn == ARM_R0 (r_scratch)
and loading rm first into ARM_R0 will result in jit_udiv() function
being called the same dividend and divisor. Fix that by loading rn
first into ARM_R1 and then rm into ARM_R0.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 arch/arm/net/bpf_jit_32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index b5f470d..ffaf311 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
 		return;
 	}
 #endif
-	if (rm != ARM_R0)
-		emit(ARM_MOV_R(ARM_R0, rm), ctx);
 	if (rn != ARM_R1)
 		emit(ARM_MOV_R(ARM_R1, rn), ctx);
+	if (rm != ARM_R0)
+		emit(ARM_MOV_R(ARM_R0, rm), ctx);
 
 	ctx->seen |= SEEN_CALL;
 	emit_mov_i(ARM_R3, (u32)jit_udiv, ctx);
-- 
1.9.1

Re: [PATCH 0/4] Seccomp filter JIT support on ARM.

[Daniel Borkmann]

On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
...
> The fourth and final patch fixes a bug in the emit_udiv() function
> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
> ARM BPF JIT code.

Shouldn't that fix go separately, so it can be included into 4.1
resp. -stable?

Would be good if you also Cc Mircea, who wrote the code. Was that
caught by lib/test_bpf.c suite (if not, would be good to add a test
case for it) ?

Re: [PATCH 0/4] Seccomp filter JIT support on ARM.

[Alexei Starovoitov]

On Wed, Apr 29, 2015 at 03:37:33PM +0200, Nicolas Schichan wrote:
> Greetings,
> 
> The following patches allow the use of the existing JIT code under
> arch/arm for seccomp filters.
> 
> The first patch makes bpf_migrate_filter() available so that seccomp
> code can use it.
> 
> The second patch invokes the classic BPF JIT code and if this fails
> reverts to the internal BPF. The open coded double call to
> bpf_convert_filter() and bpf_prog_select_runtime() is replaced by
> bpf_migrate_filter()
> 
> The third patch adds support for loads from the seccomp_data structure in
> the ARM BPF JIT code.

the patches 1,2,3 look fine.

> The fourth and final patch fixes a bug in the emit_udiv() function
> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
> ARM BPF JIT code.

this one looks independent from first three and probably should
be sent to stable?

Your cc list is different for every patch, why? What tree are you
thinking to go through? 1st patch is trivial, but 2nd belongs in
seccomp, 3rd and 4th are in arm32... I'd would say arm tree if Kees
is ok with it.

Re: [PATCH 2/4] seccomp: rework seccomp_prepare_filter().

[Daniel Borkmann]

On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
> - Try to use the classic BPF JIT via bpf_jit_compile().
>
> - Use bpf_migrate_filter() from NET filter code instead of the double
>    bpf_convert_filter() followed by bpf_prog_select_runtime() if
>    classic bpf_jit_compile() did not succeed in producing native code.
>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>

[ I had to look that one up manually, would be good if you keep
   people in Cc, also netdev for BPF in general. ]

I see, you need that to make it available to the old bpf_jit_compile()
for probing on classic JITs. Actually, I really would prefer, if instead
of duplicating that code, you could export bpf_prepare_filter() and
pass seccomp_check_filter() as an argument to bpf_prepare_filter().

Otherwise, in case bpf_prepare_filter() changes, people will easily
forget to update seccomp related code, really.

Thanks,
Daniel

Re: [PATCH 2/4] seccomp: rework seccomp_prepare_filter().

[Nicolas Schichan]

On 04/29/2015 07:12 PM, Daniel Borkmann wrote:
> On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
>> - Try to use the classic BPF JIT via bpf_jit_compile().
>>
>> - Use bpf_migrate_filter() from NET filter code instead of the double
>>    bpf_convert_filter() followed by bpf_prog_select_runtime() if
>>    classic bpf_jit_compile() did not succeed in producing native code.
>>
>> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> 
> [ I had to look that one up manually, would be good if you keep
>   people in Cc, also netdev for BPF in general. ]

Hello Daniel,

Sorry about that, I used git-send-email with a --to-cmd set to:

"./scripts/get_maintainer.pl -m --norolestats --git-max-maintainer 2"

and your email didn't show up for this particular patch. Additionally the
other emails in the serie that were addressed to you were addressed to an old
disabled email address of yours.

It didn't show up either without "--git-max-maintainer 2".

I'll take more care about the receiver list for the v2 of this serie.

> I see, you need that to make it available to the old bpf_jit_compile()
> for probing on classic JITs. Actually, I really would prefer, if instead
> of duplicating that code, you could export bpf_prepare_filter() and
> pass seccomp_check_filter() as an argument to bpf_prepare_filter().

Just to be sure you want me to pass a pointer to seccomp_check_filter to
bpf_prepare_filter so that it can run it between bpf_check_classic() and
bpf_jit_compile ?

> Otherwise, in case bpf_prepare_filter() changes, people will easily
> forget to update seccomp related code, really.

Fair point.

Thanks,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH 0/4] Seccomp filter JIT support on ARM.

[Nicolas Schichan]

On 04/29/2015 06:37 PM, Daniel Borkmann wrote:
> On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
> ...
>> The fourth and final patch fixes a bug in the emit_udiv() function
>> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
>> ARM BPF JIT code.
> 
> Shouldn't that fix go separately, so it can be included into 4.1
> resp. -stable?

Sure, shall I resend that separately from the v2 of the serie or is it fine in
its current form ?

> Would be good if you also Cc Mircea, who wrote the code. Was that
> caught by lib/test_bpf.c suite (if not, would be good to add a test
> case for it) ?

It was detected by an internal test suite we have here. I see that there are
some BPF_ALU | BPF_DIV | BPF_K instructions so it might also be caught by
lib/test_bpf.c as well.

Regards,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH 2/4] seccomp: rework seccomp_prepare_filter().

[Daniel Borkmann]

On 04/30/2015 02:27 PM, Nicolas Schichan wrote:
...
> I'll take more care about the receiver list for the v2 of this serie.

Ok, cool.

>> I see, you need that to make it available to the old bpf_jit_compile()
>> for probing on classic JITs. Actually, I really would prefer, if instead
>> of duplicating that code, you could export bpf_prepare_filter() and
>> pass seccomp_check_filter() as an argument to bpf_prepare_filter().
>
> Just to be sure you want me to pass a pointer to seccomp_check_filter to
> bpf_prepare_filter so that it can run it between bpf_check_classic() and
> bpf_jit_compile ?

For example, what comes to mind is something along these lines:

struct bpf_prog *
bpf_prepare_filter(struct bpf_prog *fp,
		   int (*aux_trans_classic)(struct sock_filter *filter,
					    unsigned int flen))
{
	int err;

	fp->bpf_func = NULL;
	fp->jited = false;

	err = bpf_check_classic(fp->insns, fp->len);
	if (err) {
		__bpf_prog_release(fp);
		return ERR_PTR(err);
	}

	/* There might be additional checks and transformations
	 * needed on classic filters, f.e. in case of seccomp.
	 */
	if (aux_trans_classic) {
		err = aux_trans_classic(fp->insns,
					fp->len);
		if (err) {
			__bpf_prog_release(fp);
			return ERR_PTR(err);
		}
	}

	/* Probe if we can JIT compile the filter and if so, do
	 * the compilation of the filter.
	 */
	bpf_jit_compile(fp);

	/* JIT compiler couldn't process this filter, so do the
	 * internal BPF translation for the optimized interpreter.
	 */
	if (!fp->jited)
		fp = bpf_migrate_filter(fp);

	return fp;
}

 From seccomp side, you invoke:

... bpf_prepare_filter(fp, seccomp_check_filter);

I would actually like to see that customized code in seccomp_prepare_filter()
further reduced instead of increased, would certainly make it easier to
maintain and understand.

Do you think something like the above is possible?

>> Otherwise, in case bpf_prepare_filter() changes, people will easily
>> forget to update seccomp related code, really.
>
> Fair point.
>
> Thanks,

Thanks a lot,
Daniel

Re: [PATCH 0/4] Seccomp filter JIT support on ARM.

[Daniel Borkmann]

On 04/30/2015 02:35 PM, Nicolas Schichan wrote:
> On 04/29/2015 06:37 PM, Daniel Borkmann wrote:
>> On 04/29/2015 03:37 PM, Nicolas Schichan wrote:
>> ...
>>> The fourth and final patch fixes a bug in the emit_udiv() function
>>> when used to convert a BPF_ALU | BPF_DIV | BPF_K instruction in the
>>> ARM BPF JIT code.
>>
>> Shouldn't that fix go separately, so it can be included into 4.1
>> resp. -stable?
>
> Sure, shall I resend that separately from the v2 of the serie or is it fine in
> its current form ?

Would be great if you could send that as an individual patch, since
it's a stand-alone fix and independent from the (feature) patch set.

>> Would be good if you also Cc Mircea, who wrote the code. Was that
>> caught by lib/test_bpf.c suite (if not, would be good to add a test
>> case for it) ?
>
> It was detected by an internal test suite we have here. I see that there are
> some BPF_ALU | BPF_DIV | BPF_K instructions so it might also be caught by
> lib/test_bpf.c as well.

If there are some additional tests that are not yet covered by lib/test_bpf.c,
I'd be happy if you could add them there. This can also be as a follow-up,
but if we can increase coverage for others as well, the better.

Thanks again,
Daniel

Re: [PATCH 2/4] seccomp: rework seccomp_prepare_filter().

[Nicolas Schichan]

On 04/30/2015 02:46 PM, Daniel Borkmann wrote:
>> Just to be sure you want me to pass a pointer to seccomp_check_filter to
>> bpf_prepare_filter so that it can run it between bpf_check_classic() and
>> bpf_jit_compile ?
> 
> For example, what comes to mind is something along these lines:
> 
> struct bpf_prog *
> bpf_prepare_filter(struct bpf_prog *fp,
>            int (*aux_trans_classic)(struct sock_filter *filter,
>                         unsigned int flen))
> {
>     int err;
> 
>     fp->bpf_func = NULL;
>     fp->jited = false;
> 
>     err = bpf_check_classic(fp->insns, fp->len);
>     if (err) {
>         __bpf_prog_release(fp);
>         return ERR_PTR(err);
>     }
> 
>     /* There might be additional checks and transformations
>      * needed on classic filters, f.e. in case of seccomp.
>      */
>     if (aux_trans_classic) {
>         err = aux_trans_classic(fp->insns,
>                     fp->len);
>         if (err) {
>             __bpf_prog_release(fp);
>             return ERR_PTR(err);
>         }
>     }
> 
>     /* Probe if we can JIT compile the filter and if so, do
>      * the compilation of the filter.
>      */
>     bpf_jit_compile(fp);
> 
>     /* JIT compiler couldn't process this filter, so do the
>      * internal BPF translation for the optimized interpreter.
>      */
>     if (!fp->jited)
>         fp = bpf_migrate_filter(fp);
> 
>     return fp;
> }
> 
> From seccomp side, you invoke:
> 
> ... bpf_prepare_filter(fp, seccomp_check_filter);

Thanks for the precisions, I'll look into that.

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH 0/4] Seccomp filter JIT support on ARM.

[Alexei Starovoitov]

On 4/30/15 5:51 AM, Daniel Borkmann wrote:
>
> If there are some additional tests that are not yet covered by
> lib/test_bpf.c,
> I'd be happy if you could add them there. This can also be as a follow-up,
> but if we can increase coverage for others as well, the better.

btw, Michael have been working on adding 173 new tests to test_bpf.
Michael, could you submit them to net-next ?

Re: [PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Russell King - ARM Linux]

On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote:
> In that case, emit_udiv() will be called with rn == ARM_R0 (r_scratch)
> and loading rm first into ARM_R0 will result in jit_udiv() function
> being called the same dividend and divisor. Fix that by loading rn
> first into ARM_R1 and then rm into ARM_R0.
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> ---
>  arch/arm/net/bpf_jit_32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index b5f470d..ffaf311 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
>  		return;
>  	}
>  #endif
> -	if (rm != ARM_R0)
> -		emit(ARM_MOV_R(ARM_R0, rm), ctx);
>  	if (rn != ARM_R1)
>  		emit(ARM_MOV_R(ARM_R1, rn), ctx);
> +	if (rm != ARM_R0)
> +		emit(ARM_MOV_R(ARM_R0, rm), ctx);

I don't think you've thought enough about this.  What if rm is ARM_R1?
What if rn = ARM_R0 and rm = ARM_R1?

How about:

	if (rn == ARM_R0 && rm == ARM_R1) {
		emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn)
		emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm)
		emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3
	} else if (rn == ARM_R0) {
		emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
		if (rm != ARM_R0)
			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
	} else {
		if (rm != ARM_R0)
			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
		if (rn != ARM_R1)
			emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
	}

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Nicolas Schichan]

On 05/01/2015 07:37 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote:
[...]
>> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
>> index b5f470d..ffaf311 100644
>> --- a/arch/arm/net/bpf_jit_32.c
>> +++ b/arch/arm/net/bpf_jit_32.c
>> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
>>  		return;
>>  	}
>>  #endif
>> -	if (rm != ARM_R0)
>> -		emit(ARM_MOV_R(ARM_R0, rm), ctx);
>>  	if (rn != ARM_R1)
>>  		emit(ARM_MOV_R(ARM_R1, rn), ctx);
>> +	if (rm != ARM_R0)
>> +		emit(ARM_MOV_R(ARM_R0, rm), ctx);
> 
> I don't think you've thought enough about this.  What if rm is ARM_R1?
> What if rn = ARM_R0 and rm = ARM_R1?
>
> How about:
> 
> 	if (rn == ARM_R0 && rm == ARM_R1) {
> 		emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn)
> 		emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm)
> 		emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3
> 	} else if (rn == ARM_R0) {
> 		emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> 		if (rm != ARM_R0)
> 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> 	} else {
> 		if (rm != ARM_R0)
> 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> 		if (rn != ARM_R1)
> 			emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> 	}
> 

Hello Russell,

In the current JIT, emit_udiv() is only being called with:

- rm = ARM_R4 (r_A) and rn = ARM_R0 (r_scrach) for BPF_ALU | BPF_DIV | BPF_K

- rm = ARM_R4 (r_A) and rn = ARM_R5 (r_X) for BPF_ALU | BPF_DIV | BPF_X

so it should not cause any issue in the current code state.

But yes, I'll rework the patch to avoid any other nasty surprises should the
code change.

Thanks,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH 4/4] ARM: net fix emit_udiv() for BPF_ALU | BPF_DIV | BPF_K intruction.

[Russell King - ARM Linux]

On Mon, May 04, 2015 at 06:16:30PM +0200, Nicolas Schichan wrote:
> On 05/01/2015 07:37 PM, Russell King - ARM Linux wrote:
> > On Wed, Apr 29, 2015 at 03:37:37PM +0200, Nicolas Schichan wrote:
> [...]
> >> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> >> index b5f470d..ffaf311 100644
> >> --- a/arch/arm/net/bpf_jit_32.c
> >> +++ b/arch/arm/net/bpf_jit_32.c
> >> @@ -449,10 +449,10 @@ static inline void emit_udiv(u8 rd, u8 rm, u8 rn, struct jit_ctx *ctx)
> >>  		return;
> >>  	}
> >>  #endif
> >> -	if (rm != ARM_R0)
> >> -		emit(ARM_MOV_R(ARM_R0, rm), ctx);
> >>  	if (rn != ARM_R1)
> >>  		emit(ARM_MOV_R(ARM_R1, rn), ctx);
> >> +	if (rm != ARM_R0)
> >> +		emit(ARM_MOV_R(ARM_R0, rm), ctx);
> > 
> > I don't think you've thought enough about this.  What if rm is ARM_R1?
> > What if rn = ARM_R0 and rm = ARM_R1?
> >
> > How about:
> > 
> > 	if (rn == ARM_R0 && rm == ARM_R1) {
> > 		emit(ARM_MOV_R(ARM_R3, rn), ctx); // r3 <- r0(rn)
> > 		emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- r1(rm)
> > 		emit(ARM_MOV_R(ARM_R1, ARM_R3), ctx); // r1 <- r3
> > 	} else if (rn == ARM_R0) {
> > 		emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> > 		if (rm != ARM_R0)
> > 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> > 	} else {
> > 		if (rm != ARM_R0)
> > 			emit(ARM_MOV_R(ARM_R0, rm), ctx); // r0 <- rm
> > 		if (rn != ARM_R1)
> > 			emit(ARM_MOV_R(ARM_R1, rn), ctx); // r1 <- rn
> > 	}
> > 
> 
> Hello Russell,
> 
> In the current JIT, emit_udiv() is only being called with:
> 
> - rm = ARM_R4 (r_A) and rn = ARM_R0 (r_scrach) for BPF_ALU | BPF_DIV | BPF_K
> 
> - rm = ARM_R4 (r_A) and rn = ARM_R5 (r_X) for BPF_ALU | BPF_DIV | BPF_X
> 
> so it should not cause any issue in the current code state.
> 
> But yes, I'll rework the patch to avoid any other nasty surprises should the
> code change.

Maybe then add a comment detailing the current conditions that this is
coded for so that if you're not around when the code changes, others are
aware of the issue.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.