[PATCH 0/6] test_bpf improvements

[PATCH 0/6] test_bpf improvements

[Nicolas Schichan]

Hello,

Please find below the patch series with my latest changes to test_bpf.

The first patch checks for unexpected NULL generated skbs before
running the filter.

The second patch adds fhe possibility for tests to generate fragmented
skbs.

The third patch tests LD_ABS and LD_IND on fragmented skbs.

The fourth patch adds the possibility to restrict the tests being run
by specifying the name/id/range of the test(s) to run via module
parameters.

The fifth patch tests LD_ABS and LD_IND on non fragmented skbs with
various sizes and alignments.

The sixth and final patch checks that the interpreter or JIT correctly
resets A and X to 0.

This serie is against today's net-next tree.

Regards,

Nicolas Schichan (6):
  test_bpf: avoid oopsing the kernel when generate_test_data() fails.
  test_bpf: allow tests to specify an skb fragment.
  test_bpf: test LD_ABS and LD_IND instructions on fragmented skbs.
  test_bpf: allow running of a single test specified as a module
    parameter.
  test_bpf: add more tests for LD_ABS and LD_IND.
  test_bpf: add tests checking that JIT/interpreter sets A and X to 0.

 lib/test_bpf.c | 645 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 644 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 1/6] test_bpf: avoid oopsing the kernel when generate_test_data() fails.

[Nicolas Schichan]

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
 lib/test_bpf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index 3afddf2..6843d0b 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -4672,6 +4672,11 @@ static int run_one(const struct bpf_prog *fp, struct bpf_test *test)
 			break;
 
 		data = generate_test_data(test, i);
+		if (!data && !(test->aux & FLAG_NO_DATA)) {
+			pr_cont("data generation failed ");
+			err_cnt++;
+			break;
+		}
 		ret = __run_one(fp, data, runs, &duration);
 		release_test_data(test, data);
 
-- 
1.9.1

Re: [PATCH 1/6] test_bpf: avoid oopsing the kernel when generate_test_data() fails.

[Daniel Borkmann]

On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[PATCH 2/6] test_bpf: allow tests to specify an skb fragment.

[Nicolas Schichan]

This introduce a new test->aux flag (FLAG_SKB_FRAG) to tell the
populate_skb() function to add a fragment to the test skb containing
the data specified in test->frag_data).

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
 lib/test_bpf.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index 6843d0b..acef1e9 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -23,6 +23,7 @@
 #include <linux/netdevice.h>
 #include <linux/if_vlan.h>
 #include <linux/random.h>
+#include <linux/highmem.h>
 
 /* General test specific settings */
 #define MAX_SUBTESTS	3
@@ -56,6 +57,7 @@
 /* Flags that can be passed to test cases */
 #define FLAG_NO_DATA		BIT(0)
 #define FLAG_EXPECTED_FAIL	BIT(1)
+#define FLAG_SKB_FRAG		BIT(2)
 
 enum {
 	CLASSIC  = BIT(6),	/* Old BPF instructions only. */
@@ -81,6 +83,7 @@ struct bpf_test {
 		__u32 result;
 	} test[MAX_SUBTESTS];
 	int (*fill_helper)(struct bpf_test *self);
+	__u8 frag_data[MAX_DATA];
 };
 
 /* Large test cases need separate allocation and fill handler. */
@@ -4525,6 +4528,10 @@ static struct sk_buff *populate_skb(char *buf, int size)
 
 static void *generate_test_data(struct bpf_test *test, int sub)
 {
+	struct sk_buff *skb;
+	struct page *page;
+	void *ptr;
+
 	if (test->aux & FLAG_NO_DATA)
 		return NULL;
 
@@ -4532,7 +4539,36 @@ static void *generate_test_data(struct bpf_test *test, int sub)
 	 * subtests generate skbs of different sizes based on
 	 * the same data.
 	 */
-	return populate_skb(test->data, test->test[sub].data_size);
+	skb = populate_skb(test->data, test->test[sub].data_size);
+	if (!skb)
+		return NULL;
+
+	if (test->aux & FLAG_SKB_FRAG) {
+		/*
+		 * when the test requires a fragmented skb, add a
+		 * single fragment to the skb, filled with
+		 * test->frag_data.
+		 */
+		page = alloc_page(GFP_KERNEL);
+
+		if (!page)
+			goto err_kfree_skb;
+
+		ptr = kmap(page);
+		if (!ptr)
+			goto err_free_page;
+		memcpy(ptr, test->frag_data, MAX_DATA);
+		kunmap(page);
+		skb_add_rx_frag(skb, 0, page, 0, MAX_DATA, MAX_DATA);
+	}
+
+	return skb;
+
+err_free_page:
+	__free_page(page);
+err_kfree_skb:
+	kfree_skb(skb);
+	return NULL;
 }
 
 static void release_test_data(const struct bpf_test *test, void *data)
-- 
1.9.1

Re: [PATCH 2/6] test_bpf: allow tests to specify an skb fragment.

[Daniel Borkmann]

On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> This introduce a new test->aux flag (FLAG_SKB_FRAG) to tell the
> populate_skb() function to add a fragment to the test skb containing
> the data specified in test->frag_data).
>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

I'm good with this change here, just a comment below in general.

>   enum {
>   	CLASSIC  = BIT(6),	/* Old BPF instructions only. */
> @@ -81,6 +83,7 @@ struct bpf_test {
>   		__u32 result;
>   	} test[MAX_SUBTESTS];
>   	int (*fill_helper)(struct bpf_test *self);
> +	__u8 frag_data[MAX_DATA];
>   };

We now have 286 tests, which is awesome!

Perhaps, we need to start thinking of a better test description method
soonish as the test_bpf.ko module grew to ~1.6M, i.e. whenever we add
to struct bpf_test, it adds memory overhead upon all test cases.

>   /* Large test cases need separate allocation and fill handler. */
> @@ -4525,6 +4528,10 @@ static struct sk_buff *populate_skb(char *buf, int size)
>
>   static void *generate_test_data(struct bpf_test *test, int sub)
>   {
> +	struct sk_buff *skb;
> +	struct page *page;
> +	void *ptr;
> +
>   	if (test->aux & FLAG_NO_DATA)
>   		return NULL;
>
> @@ -4532,7 +4539,36 @@ static void *generate_test_data(struct bpf_test *test, int sub)
>   	 * subtests generate skbs of different sizes based on
>   	 * the same data.
>   	 */
> -	return populate_skb(test->data, test->test[sub].data_size);
> +	skb = populate_skb(test->data, test->test[sub].data_size);
> +	if (!skb)
> +		return NULL;
> +
> +	if (test->aux & FLAG_SKB_FRAG) {

Really minor nit: declaration of page, ptr could have been only in this block.

Re: [PATCH 2/6] test_bpf: allow tests to specify an skb fragment.

[Nicolas Schichan]

On 08/03/2015 05:29 PM, Daniel Borkmann wrote:
> On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> We now have 286 tests, which is awesome!
> 
> Perhaps, we need to start thinking of a better test description method
> soonish as the test_bpf.ko module grew to ~1.6M, i.e. whenever we add
> to struct bpf_test, it adds memory overhead upon all test cases.

Indeed, test_bpf.ko is turning quite large (1.4M when compiled for ARM).

It looks like gzip is able to do wonders on the module though as I end up with
a 94.7K test_bpf.ko.gz file and if the modutils are compiled with
--enable-zlib, it will be gunziped automatically before being loaded to the
kernel.

I think that marking tests[] array as __initdata will help with the runtime
memory use if someone forgets to rmmod the test_bpf module after a completely
successful run.

>>   /* Large test cases need separate allocation and fill handler. */
>> @@ -4525,6 +4528,10 @@ static struct sk_buff *populate_skb(char *buf, int size)
>>
>>   static void *generate_test_data(struct bpf_test *test, int sub)
>>   {
>> +    struct sk_buff *skb;
>> +    struct page *page;
>> +    void *ptr;
>> +
>>       if (test->aux & FLAG_NO_DATA)
>>           return NULL;
>>
>> @@ -4532,7 +4539,36 @@ static void *generate_test_data(struct bpf_test
>> *test, int sub)
>>        * subtests generate skbs of different sizes based on
>>        * the same data.
>>        */
>> -    return populate_skb(test->data, test->test[sub].data_size);
>> +    skb = populate_skb(test->data, test->test[sub].data_size);
>> +    if (!skb)
>> +        return NULL;
>> +
>> +    if (test->aux & FLAG_SKB_FRAG) {
> 
> Really minor nit: declaration of page, ptr could have been only in this block.

I can certainly move the ptr declaration in the if block, but I'd rather leave
the struct page there to avoid to have the cleanup code awkwardly sitting in
the if block if that's okay with you.

Thanks,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH 2/6] test_bpf: allow tests to specify an skb fragment.

[Daniel Borkmann]

On 08/03/2015 06:38 PM, Nicolas Schichan wrote:
> On 08/03/2015 05:29 PM, Daniel Borkmann wrote:
>> On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
>> We now have 286 tests, which is awesome!
>>
>> Perhaps, we need to start thinking of a better test description method
>> soonish as the test_bpf.ko module grew to ~1.6M, i.e. whenever we add
>> to struct bpf_test, it adds memory overhead upon all test cases.
>
> Indeed, test_bpf.ko is turning quite large (1.4M when compiled for ARM).
>
> It looks like gzip is able to do wonders on the module though as I end up with
> a 94.7K test_bpf.ko.gz file and if the modutils are compiled with
> --enable-zlib, it will be gunziped automatically before being loaded to the
> kernel.

I think it just contains a lot of zero blocks, which then compress nicely.

> I think that marking tests[] array as __initdata will help with the runtime
> memory use if someone forgets to rmmod the test_bpf module after a completely
> successful run.

Can be done, too, yep. Do you want to send a patch? ;)

[PATCH 3/6] test_bpf: test LD_ABS and LD_IND instructions on fragmented skbs.

[Nicolas Schichan]

These new tests exercise various load sizes and offsets crossing the
head/fragment boundary.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
 lib/test_bpf.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index acef1e9..f5606fb 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -4493,6 +4493,148 @@ static struct bpf_test tests[] = {
 		{ { 1, 0xbef } },
 		.fill_helper = bpf_fill_ld_abs_vlan_push_pop,
 	},
+	/*
+	 * LD_IND / LD_ABS on fragmented SKBs
+	 */
+	{
+		"LD_IND byte frag",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x40),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_B, 0x0),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ },
+		{ {0x40, 0x42} },
+		.frag_data = {
+			0x42, 0x00, 0x00, 0x00,
+			0x43, 0x44, 0x00, 0x00,
+			0x21, 0x07, 0x19, 0x83,
+		},
+	},
+	{
+		"LD_IND halfword frag",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x40),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_H, 0x4),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ },
+		{ {0x40, 0x4344} },
+		.frag_data = {
+			0x42, 0x00, 0x00, 0x00,
+			0x43, 0x44, 0x00, 0x00,
+			0x21, 0x07, 0x19, 0x83,
+		},
+	},
+	{
+		"LD_IND word frag",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x40),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_W, 0x8),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ },
+		{ {0x40, 0x21071983} },
+		.frag_data = {
+			0x42, 0x00, 0x00, 0x00,
+			0x43, 0x44, 0x00, 0x00,
+			0x21, 0x07, 0x19, 0x83,
+		},
+	},
+	{
+		"LD_IND halfword mixed head/frag",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x40),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_H, -0x1),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ [0x3e] = 0x25, [0x3f] = 0x05, },
+		{ {0x40, 0x0519} },
+		.frag_data = { 0x19, 0x82 },
+	},
+	{
+		"LD_IND word mixed head/frag",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x40),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_W, -0x2),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ [0x3e] = 0x25, [0x3f] = 0x05, },
+		{ {0x40, 0x25051982} },
+		.frag_data = { 0x19, 0x82 },
+	},
+	{
+		"LD_ABS byte frag",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_B, 0x40),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ },
+		{ {0x40, 0x42} },
+		.frag_data = {
+			0x42, 0x00, 0x00, 0x00,
+			0x43, 0x44, 0x00, 0x00,
+			0x21, 0x07, 0x19, 0x83,
+		},
+	},
+	{
+		"LD_ABS halfword frag",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_H, 0x44),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ },
+		{ {0x40, 0x4344} },
+		.frag_data = {
+			0x42, 0x00, 0x00, 0x00,
+			0x43, 0x44, 0x00, 0x00,
+			0x21, 0x07, 0x19, 0x83,
+		},
+	},
+	{
+		"LD_ABS word frag",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_W, 0x48),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ },
+		{ {0x40, 0x21071983} },
+		.frag_data = {
+			0x42, 0x00, 0x00, 0x00,
+			0x43, 0x44, 0x00, 0x00,
+			0x21, 0x07, 0x19, 0x83,
+		},
+	},
+	{
+		"LD_ABS halfword mixed head/frag",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_H, 0x3f),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ [0x3e] = 0x25, [0x3f] = 0x05, },
+		{ {0x40, 0x0519} },
+		.frag_data = { 0x19, 0x82 },
+	},
+	{
+		"LD_ABS word mixed head/frag",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_W, 0x3e),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_SKB_FRAG,
+		{ [0x3e] = 0x25, [0x3f] = 0x05, },
+		{ {0x40, 0x25051982} },
+		.frag_data = { 0x19, 0x82 },
+	},
 };
 
 static struct net_device dev;
-- 
1.9.1

Re: [PATCH 3/6] test_bpf: test LD_ABS and LD_IND instructions on fragmented skbs.

[Daniel Borkmann]

On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> These new tests exercise various load sizes and offsets crossing the
> head/fragment boundary.
>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[PATCH 4/6] test_bpf: add module parameters to filter the tests to run.

[Nicolas Schichan]

When developping on the interpreter or a particular JIT, it can be
insteresting to restrict the test list to a specific test or a
particular range of tests.

This patch adds the following module parameters to the test_bpf module:

* test_name=<string>: only the specified named test will be run.

* test_id=<number>: only the test with the specified id will be run
  (see the output of test_pbf without parameters to get the test id).

* test_range=<number>,<number>: only the tests with IDs in the
  specified id range are run (see the output of test_pbf without
  parameters to get the test ids).

Any invalid range, test id or test name will result in -EINVAL being
returned and no tests being run.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
---
 lib/test_bpf.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index f5606fb..abd0507 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -4870,10 +4870,72 @@ static int run_one(const struct bpf_prog *fp, struct bpf_test *test)
 	return err_cnt;
 }
 
+static char test_name[64];
+module_param_string(test_name, test_name, sizeof(test_name), 0);
+
+static int test_id = -1;
+module_param(test_id, int, 0);
+
+static int test_range[2] = { -1, -1 };
+module_param_array(test_range, int, NULL, 0);
+
+static __init int find_test_index(const char *test_name)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		if (!strcmp(tests[i].descr, test_name))
+			return i;
+	}
+	return -1;
+}
+
 static __init int prepare_bpf_tests(void)
 {
 	int i;
 
+	if (test_id >= 0) {
+		/*
+		 * if a test_id was specified, use test_range to
+		 * conver only that test.
+		 */
+		if (test_id >= ARRAY_SIZE(tests)) {
+			pr_err("test_bpf: invalid test_id specified.\n");
+			return -EINVAL;
+		}
+
+		test_range[0] = test_id;
+		test_range[1] = test_id;
+	} else if (test_range[0] >= 0) {
+		/*
+		 * check that the supplied test_range is valid.
+		 */
+		if (test_range[0] >= ARRAY_SIZE(tests) ||
+		    test_range[1] >= ARRAY_SIZE(tests)) {
+			pr_err("test_bpf: test_range is out of bound.\n");
+			return -EINVAL;
+		}
+
+		if (test_range[1] < test_range[0]) {
+			pr_err("test_bpf: test_range is ending before it starts.\n");
+			return -EINVAL;
+		}
+	} else if (*test_name) {
+		/*
+		 * if a test_name was specified, find it and setup
+		 * test_range to cover only that test.
+		 */
+		int idx = find_test_index(test_name);
+
+		if (idx < 0) {
+			pr_err("test_bpf: no test named '%s' found.\n",
+			       test_name);
+			return -EINVAL;
+		}
+		test_range[0] = idx;
+		test_range[1] = idx;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(tests); i++) {
 		if (tests[i].fill_helper &&
 		    tests[i].fill_helper(&tests[i]) < 0)
@@ -4893,6 +4955,14 @@ static __init void destroy_bpf_tests(void)
 	}
 }
 
+static bool exclude_test(int test_id)
+{
+	if (test_range[0] >= 0 &&
+	    (test_id < test_range[0] || test_id > test_range[1]))
+		return true;
+	return false;
+}
+
 static __init int test_bpf(void)
 {
 	int i, err_cnt = 0, pass_cnt = 0;
@@ -4902,6 +4972,9 @@ static __init int test_bpf(void)
 		struct bpf_prog *fp;
 		int err;
 
+		if (exclude_test(i))
+			continue;
+
 		pr_info("#%d %s ", i, tests[i].descr);
 
 		fp = generate_filter(i, &err);
-- 
1.9.1

Re: [PATCH 4/6] test_bpf: add module parameters to filter the tests to run.

[Daniel Borkmann]

On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> When developping on the interpreter or a particular JIT, it can be
> insteresting to restrict the test list to a specific test or a

s/insteresting/interesting/

> particular range of tests.
>
> This patch adds the following module parameters to the test_bpf module:
>
> * test_name=<string>: only the specified named test will be run.
>
> * test_id=<number>: only the test with the specified id will be run
>    (see the output of test_pbf without parameters to get the test id).

s/test_pbf/test_bpf/

> * test_range=<number>,<number>: only the tests with IDs in the
>    specified id range are run (see the output of test_pbf without

s/test_pbf/test_bpf/

>    parameters to get the test ids).
>
> Any invalid range, test id or test name will result in -EINVAL being
> returned and no tests being run.
>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>

Seems very useful for the test suite, thanks.

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

> ---
>   lib/test_bpf.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
>
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index f5606fb..abd0507 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -4870,10 +4870,72 @@ static int run_one(const struct bpf_prog *fp, struct bpf_test *test)
>   	return err_cnt;
>   }
>
> +static char test_name[64];
> +module_param_string(test_name, test_name, sizeof(test_name), 0);
> +
> +static int test_id = -1;
> +module_param(test_id, int, 0);
> +
> +static int test_range[2] = { -1, -1 };
> +module_param_array(test_range, int, NULL, 0);
> +
> +static __init int find_test_index(const char *test_name)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +		if (!strcmp(tests[i].descr, test_name))
> +			return i;
> +	}
> +	return -1;
> +}
> +
>   static __init int prepare_bpf_tests(void)
>   {
>   	int i;
>
> +	if (test_id >= 0) {
> +		/*
> +		 * if a test_id was specified, use test_range to
> +		 * conver only that test.

s/conver/cover/

> +		 */
> +		if (test_id >= ARRAY_SIZE(tests)) {
> +			pr_err("test_bpf: invalid test_id specified.\n");
> +			return -EINVAL;
> +		}
[...]
> @@ -4893,6 +4955,14 @@ static __init void destroy_bpf_tests(void)
>   	}
>   }
>
> +static bool exclude_test(int test_id)
> +{
> +	if (test_range[0] >= 0 &&
> +	    (test_id < test_range[0] || test_id > test_range[1]))
> +		return true;
> +	return false;

Minor nit: could directly return it, f.e.:

	return test_range[0] >= 0 && (test_id < test_range[0] ||
                                       test_id > test_range[1]);

Btw, for the range test in prepare_bpf_tests(), you could also reject
a negative lower bound index right there.

Re: [PATCH 4/6] test_bpf: add module parameters to filter the tests to run.

[Nicolas Schichan]

On 08/03/2015 05:58 PM, Daniel Borkmann wrote:
> On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
>> When developping on the interpreter or a particular JIT, it can be
>> insteresting to restrict the test list to a specific test or a
> 
> s/insteresting/interesting/
[...]
> s/test_pbf/test_bpf/
[...]
> s/test_pbf/test_bpf/
[...]
> s/conver/cover/

Sorry for the various typos, I'll fix that in a V2.

>> +         */
>> +        if (test_id >= ARRAY_SIZE(tests)) {
>> +            pr_err("test_bpf: invalid test_id specified.\n");
>> +            return -EINVAL;
>> +        }
> [...]
>> @@ -4893,6 +4955,14 @@ static __init void destroy_bpf_tests(void)
>>       }
>>   }
>>
>> +static bool exclude_test(int test_id)
>> +{
>> +    if (test_range[0] >= 0 &&
>> +        (test_id < test_range[0] || test_id > test_range[1]))
>> +        return true;
>> +    return false;
> 
> Minor nit: could directly return it, f.e.:
> 
>     return test_range[0] >= 0 && (test_id < test_range[0] ||
>                                       test_id > test_range[1]);

I will change that.

> Btw, for the range test in prepare_bpf_tests(), you could also reject
> a negative lower bound index right there.

I thought it was better to have all the sanity checks grouped in
prepare_bpf_tests() (with the checking of the test_name and test_id parameters
nearby) ? Also a negative lower bound is meaning that no range has been set so
all tests should be run.

Thanks,

-- 
Nicolas Schichan
Freebox SAS

Re: [PATCH 4/6] test_bpf: add module parameters to filter the tests to run.

[Daniel Borkmann]

On 08/03/2015 06:23 PM, Nicolas Schichan wrote:
...
>> Btw, for the range test in prepare_bpf_tests(), you could also reject
>> a negative lower bound index right there.
>
> I thought it was better to have all the sanity checks grouped in
> prepare_bpf_tests() (with the checking of the test_name and test_id parameters
> nearby) ? Also a negative lower bound is meaning that no range has been set so
> all tests should be run.

I just got a bit confused when loading test_range=-100,1 was not rejected, but
they do indeed all run in this case.

Thanks,
Daniel

[PATCH 5/6] test_bpf: add more tests for LD_ABS and LD_IND.

[Nicolas Schichan]

This exerces the LD_ABS and LD_IND instructions for various sizes and
alignments. This also checks that X when used as an offset to a
BPF_IND instruction first in a filter is correctly set to 0.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
 lib/test_bpf.c | 296 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 296 insertions(+)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index abd0507..a84a875 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -4635,6 +4635,302 @@ static struct bpf_test tests[] = {
 		{ {0x40, 0x25051982} },
 		.frag_data = { 0x19, 0x82 },
 	},
+	/*
+	 * LD_IND / LD_ABS on non fragmented SKBs
+	 */
+	{
+		/*
+		 * this tests that the JIT/interpreter correctly resets X
+		 * before using it in an LD_IND instruction.
+		 */
+		"LD_IND byte default X",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_IND | BPF_B, 0x1),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{ [0x1] = 0x42 },
+		{ {0x40, 0x42 } },
+	},
+	{
+		"LD_IND byte positive offset",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x3e),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_B, 0x1),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{ [0x3c] = 0x25, [0x3d] = 0x05,  [0x3e] = 0x19, [0x3f] = 0x82 },
+		{ {0x40, 0x82 } },
+	},
+	{
+		"LD_IND byte negative offset",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x3e),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_B, -0x1),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{ [0x3c] = 0x25, [0x3d] = 0x05,  [0x3e] = 0x19, [0x3f] = 0x82 },
+		{ {0x40, 0x05 } },
+	},
+	{
+		"LD_IND halfword positive offset",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_H, 0x2),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+		},
+		{ {0x40, 0xdd88 } },
+	},
+	{
+		"LD_IND halfword negative offset",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_H, -0x2),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+		},
+		{ {0x40, 0xbb66 } },
+	},
+	{
+		"LD_IND halfword unaligned",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_H, -0x1),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+		},
+		{ {0x40, 0x66cc } },
+	},
+	{
+		"LD_IND word positive offset",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_W, 0x4),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0xee99ffaa } },
+	},
+	{
+		"LD_IND word negative offset",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_W, -0x4),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0xaa55bb66 } },
+	},
+	{
+		"LD_IND word unaligned (addr & 3 == 2)",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_W, -0x2),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0xbb66cc77 } },
+	},
+	{
+		"LD_IND word unaligned (addr & 3 == 1)",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_W, -0x3),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0x55bb66cc } },
+	},
+	{
+		"LD_IND word unaligned (addr & 3 == 3)",
+		.u.insns = {
+			BPF_STMT(BPF_LDX | BPF_IMM, 0x20),
+			BPF_STMT(BPF_LD | BPF_IND | BPF_W, -0x1),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0x66cc77dd } },
+	},
+	{
+		"LD_ABS byte",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_B, 0x20),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0xcc } },
+	},
+	{
+		"LD_ABS halfword",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_H, 0x22),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0xdd88 } },
+	},
+	{
+		"LD_ABS halfword unaligned",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_H, 0x25),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0x99ff } },
+	},
+	{
+		"LD_ABS word",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_W, 0x1c),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0xaa55bb66 } },
+	},
+	{
+		"LD_ABS word unaligned (addr & 3 == 2)",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_W, 0x22),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0xdd88ee99 } },
+	},
+	{
+		"LD_ABS word unaligned (addr & 3 == 1)",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_W, 0x21),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0x77dd88ee } },
+	},
+	{
+		"LD_ABS word unaligned (addr & 3 == 3)",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_ABS | BPF_W, 0x23),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC,
+		{
+			[0x1c] = 0xaa, [0x1d] = 0x55,
+			[0x1e] = 0xbb, [0x1f] = 0x66,
+			[0x20] = 0xcc, [0x21] = 0x77,
+			[0x22] = 0xdd, [0x23] = 0x88,
+			[0x24] = 0xee, [0x25] = 0x99,
+			[0x26] = 0xff, [0x27] = 0xaa,
+		},
+		{ {0x40, 0x88ee99ff } },
+	},
 };
 
 static struct net_device dev;
-- 
1.9.1

Re: [PATCH 5/6] test_bpf: add more tests for LD_ABS and LD_IND.

[Daniel Borkmann]

On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> This exerces the LD_ABS and LD_IND instructions for various sizes and
> alignments. This also checks that X when used as an offset to a
> BPF_IND instruction first in a filter is correctly set to 0.
>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[PATCH 6/6] test_bpf: add tests checking that JIT/interpreter sets A and X to 0.

[Nicolas Schichan]

It is mandatory for the JIT or interpreter to reset the A and X
registers to 0 before running the filter. Check that it is the case on
various ALU and JMP instructions.

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Acked-by: Alexei Starovoitov <ast@plumgrid.com>
---
 lib/test_bpf.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index a84a875..51f94f9 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c
@@ -4931,6 +4931,164 @@ static struct bpf_test tests[] = {
 		},
 		{ {0x40, 0x88ee99ff } },
 	},
+	/*
+	 * verify that the interpreter or JIT correctly sets A and X
+	 * to 0.
+	 */
+	{
+		"ADD default X",
+		.u.insns = {
+			/*
+			 * A = 0x42
+			 * A = A + X
+			 * ret A
+			 */
+			BPF_STMT(BPF_LD | BPF_IMM, 0x42),
+			BPF_STMT(BPF_ALU | BPF_ADD | BPF_X, 0),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x42 } },
+	},
+	{
+		"ADD default A",
+		.u.insns = {
+			/*
+			 * A = A + 0x42
+			 * ret A
+			 */
+			BPF_STMT(BPF_ALU | BPF_ADD | BPF_K, 0x42),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x42 } },
+	},
+	{
+		"SUB default X",
+		.u.insns = {
+			/*
+			 * A = 0x66
+			 * A = A - X
+			 * ret A
+			 */
+			BPF_STMT(BPF_LD | BPF_IMM, 0x66),
+			BPF_STMT(BPF_ALU | BPF_SUB | BPF_X, 0),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x66 } },
+	},
+	{
+		"SUB default A",
+		.u.insns = {
+			/*
+			 * A = A - -0x66
+			 * ret A
+			 */
+			BPF_STMT(BPF_ALU | BPF_SUB | BPF_K, -0x66),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x66 } },
+	},
+	{
+		"MUL default X",
+		.u.insns = {
+			/*
+			 * A = 0x42
+			 * A = A * X
+			 * ret A
+			 */
+			BPF_STMT(BPF_LD | BPF_IMM, 0x42),
+			BPF_STMT(BPF_ALU | BPF_MUL | BPF_X, 0),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x0 } },
+	},
+	{
+		"MUL default A",
+		.u.insns = {
+			/*
+			 * A = A * 0x66
+			 * ret A
+			 */
+			BPF_STMT(BPF_ALU | BPF_MUL | BPF_K, 0x66),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x0 } },
+	},
+	{
+		"DIV default X",
+		.u.insns = {
+			/*
+			 * A = 0x42
+			 * A = A / X ; this halt the filter execution if X is 0
+			 * ret 0x42
+			 */
+			BPF_STMT(BPF_LD | BPF_IMM, 0x42),
+			BPF_STMT(BPF_ALU | BPF_DIV | BPF_X, 0),
+			BPF_STMT(BPF_RET | BPF_K, 0x42),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x0 } },
+	},
+	{
+		"DIV default A",
+		.u.insns = {
+			/*
+			 * A = A / 1
+			 * ret A
+			 */
+			BPF_STMT(BPF_ALU | BPF_DIV | BPF_K, 0x1),
+			BPF_STMT(BPF_RET | BPF_A, 0x0),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x0 } },
+	},
+	{
+		"JMP EQ default A",
+		.u.insns = {
+			/*
+			 * cmp A, 0x0, 0, 1
+			 * ret 0x42
+			 * ret 0x66
+			 */
+			BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0x0, 0, 1),
+			BPF_STMT(BPF_RET | BPF_K, 0x42),
+			BPF_STMT(BPF_RET | BPF_K, 0x66),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x42 } },
+	},
+	{
+		"JMP EQ default X",
+		.u.insns = {
+			/*
+			 * A = 0x0
+			 * cmp A, X, 0, 1
+			 * ret 0x42
+			 * ret 0x66
+			 */
+			BPF_STMT(BPF_LD | BPF_IMM, 0x0),
+			BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_X, 0x0, 0, 1),
+			BPF_STMT(BPF_RET | BPF_K, 0x42),
+			BPF_STMT(BPF_RET | BPF_K, 0x66),
+		},
+		CLASSIC | FLAG_NO_DATA,
+		{},
+		{ {0x1, 0x42 } },
+	},
 };
 
 static struct net_device dev;
-- 
1.9.1

Re: [PATCH 6/6] test_bpf: add tests checking that JIT/interpreter sets A and X to 0.

[Daniel Borkmann]

On 08/03/2015 04:02 PM, Nicolas Schichan wrote:
> It is mandatory for the JIT or interpreter to reset the A and X
> registers to 0 before running the filter. Check that it is the case on
> various ALU and JMP instructions.
>
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>