net: filter: fix length calculation in BPF testsuite

PATCH Superseded Chema Gonzalez 4 participants 7 messages May 29, 2014 – Jun 2, 2014

Subsystems (3)

Networking Network drivers Library code

lore

Share & tools

lore

Chema Gonzalez

author

2014-05-29 19:29 UTC #0 lore raw

PATCH 1/1 net: filter: fix length calculation in BPF testsuite

The current probe_filter_length() (the function that calculates the length of a test BPF filter) behavior is to declare the end of the filter as soon as it finds {0, *, *, 0}. This is actually a valid insn ("ld #0"), so any filter with includes "BPF_STMT(BPF_LD | BPF_IMM, 0)" fails (its length is cut short).

We are changing probe_filter_length() so as to start from the end, and declare the end of the filter as the first instruction which is not {0, *, *, 0}. This solution produces a simpler patch than the alternative of using an explicit end-of-filter mark. It is technically incorrect if your filter ends up with "ld #0", but that should not happen anyway.

We also add a new test (LD_IMM_0) that includes ld #0 (does not work without this patch).

Signed-off-by Chema Gonzalez <[email protected]>

---
 lib/test_bpf.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

lib/test_bpf.c +16 -5

index af677cb..e893e3b 100644
@@ -158,6 +158,18 @@ static struct bpf_test tests[] = {
 		{ { 0, 0x800000ff }, { 1, 0x800000ff } },
 	},
 	{
+		"LD_IMM_0",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_IMM, 0), /* ld #0 */
+			BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0, 1, 0),
+			BPF_STMT(BPF_RET | BPF_K, 0),
+			BPF_STMT(BPF_RET | BPF_K, 1),
+		},
+		CLASSIC,
+		{ },
+		{ { 1, 1 } },
+	},
+	{
 		"LD_IND",
 		.u.insns = {
 			BPF_STMT(BPF_LDX | BPF_LEN, 0),

@@ -1542,12 +1554,11 @@ static int probe_filter_length(struct sock_filter *fp)
 {
 	int len = 0;
 
-	while (fp->code != 0 || fp->k != 0) {
-		fp++;
-		len++;
-	}
+	for (len = MAX_INSNS-1; len > 0; --len)
+		if (fp[len].code != 0 || fp[len].k != 0)
+			break;
 
-	return len;
+	return len+1;
 }
 
 static struct sk_filter *generate_filter(int which, int *err)

Signature · 1 line

1.9.1.423.g4596e3a

Daniel Borkmann

Acked-by on v1 1/1

2014-05-29 20:41 UTC #1 lore raw

Quoted 13 lines · Chema Gonzalez

The current probe_filter_length() (the function that calculates the length of a test BPF filter) behavior is to declare the end of the filter as soon as it finds {0, *, *, 0}. This is actually a valid insn ("ld #0"), so any filter with includes "BPF_STMT(BPF_LD | BPF_IMM, 0)" fails (its length is cut short).
We are changing probe_filter_length() so as to start from the end, and declare the end of the filter as the first instruction which is not {0, *, *, 0}. This solution produces a simpler patch than the alternative of using an explicit end-of-filter mark. It is technically incorrect if your filter ends up with "ld #0", but that should not happen anyway.

Quoted 4 lines

We also add a new test (LD_IMM_0) that includes ld #0 (does not work without this patch).
Signed-off-by Chema Gonzalez <[email protected]>

Looks good to me, thanks a lot Chema!

Acked-by Daniel Borkmann <[email protected]>

lib/test_bpf.c static int probe_filter_length(struct sock_filter *fp) · line 1561

Quoted 6 lines

lib/test_bpf.c +4 -1

+	for (len = MAX_INSNS-1; len > 0; --len)
+		if (fp[len].code != 0 || fp[len].k != 0)
+			break;
 
-	return len;
+	return len+1;

Nit: would be great to have a whitespace between MAX_INSNS-1 and len+1 but that shouldn't matter that much, perhaps.

Chema Gonzalez

author

2014-05-30 17:15 UTC #2 lore raw

Quoted 31 lines · Daniel Borkmann

Quoted 15 lines · Chema Gonzalez
The current probe_filter_length() (the function that calculates the length of a test BPF filter) behavior is to declare the end of the filter as soon as it finds {0, *, *, 0}. This is actually a valid insn ("ld #0"), so any filter with includes "BPF_STMT(BPF_LD | BPF_IMM, 0)"
fails (its length is cut short).
We are changing probe_filter_length() so as to start from the end, and declare the end of the filter as the first instruction which is not {0, *, *, 0}. This solution produces a simpler patch than the alternative of using an explicit end-of-filter mark. It is technically incorrect if your filter ends up with "ld #0", but that should not happen anyway.
Quoted 4 lines
We also add a new test (LD_IMM_0) that includes ld #0 (does not work without this patch).
Signed-off-by Chema Gonzalez <[email protected]>
Looks good to me, thanks a lot Chema!
Acked-by Daniel Borkmann <[email protected]>
Quoted 4 lines
lib/test_bpf.c +3 -0
+       for (len = MAX_INSNS-1; len > 0; --len)
+               if (fp[len].code != 0 || fp[len].k != 0)
+                       break;

Quoted 6 lines

lib/test_bpf.c static int probe_filter_length(struct sock_filter *fp) · line 1561
Quoted 2 lines
lib/test_bpf.c +1 -1
-       return len;
+       return len+1;
Nit: would be great to have a whitespace between MAX_INSNS-1 and len+1 but that shouldn't matter that much, perhaps.

Done.

-Chema

Chema Gonzalez

author

2014-05-30 17:15 UTC #3 lore raw

PATCH v2 1/1 net: filter: fix length calculation in BPF testsuite

In reply to Daniel Borkmann

We also add a new test (LD_IMM_0) that includes ld #0 (does not work without this patch).

Signed-off-by Chema Gonzalez <[email protected]>

---
 lib/test_bpf.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

lib/test_bpf.c +16 -5

index af677cb..e60f766 100644
@@ -158,6 +158,18 @@ static struct bpf_test tests[] = {
 		{ { 0, 0x800000ff }, { 1, 0x800000ff } },
 	},
 	{
+		"LD_IMM_0",
+		.u.insns = {
+			BPF_STMT(BPF_LD | BPF_IMM, 0), /* ld #0 */
+			BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0, 1, 0),
+			BPF_STMT(BPF_RET | BPF_K, 0),
+			BPF_STMT(BPF_RET | BPF_K, 1),
+		},
+		CLASSIC,
+		{ },
+		{ { 1, 1 } },
+	},
+	{
 		"LD_IND",
 		.u.insns = {
 			BPF_STMT(BPF_LDX | BPF_LEN, 0),

@@ -1542,12 +1554,11 @@ static int probe_filter_length(struct sock_filter *fp)
 {
 	int len = 0;
 
-	while (fp->code != 0 || fp->k != 0) {
-		fp++;
-		len++;
-	}
+	for (len = MAX_INSNS - 1; len > 0; --len)
+		if (fp[len].code != 0 || fp[len].k != 0)
+			break;
 
-	return len;
+	return len + 1;
 }
 
 static struct sk_filter *generate_filter(int which, int *err)

Signature · 1 line

1.9.1.423.g4596e3a

Daniel Borkmann

Acked-by on v2 1/1

2014-05-31 19:22 UTC #4 lore raw

Quoted 13 lines · Chema Gonzalez

The current probe_filter_length() (the function that calculates the length of a test BPF filter) behavior is to declare the end of the filter as soon as it finds {0, *, *, 0}. This is actually a valid insn ("ld #0"), so any filter with includes "BPF_STMT(BPF_LD | BPF_IMM, 0)" fails (its length is cut short).
We are changing probe_filter_length() so as to start from the end, and declare the end of the filter as the first instruction which is not {0, *, *, 0}. This solution produces a simpler patch than the alternative of using an explicit end-of-filter mark. It is technically incorrect if your filter ends up with "ld #0", but that should not happen anyway.

Quoted 4 lines

We also add a new test (LD_IMM_0) that includes ld #0 (does not work without this patch).
Signed-off-by Chema Gonzalez <[email protected]>

Thanks (sorry for the delay as I'm currently on travel).

Acked-by Daniel Borkmann <[email protected]>

Alexei Starovoitov

Acked-by

2014-06-02 06:33 UTC #5 lore raw

Quoted 16 lines · Daniel Borkmann

Quoted 15 lines · Chema Gonzalez
The current probe_filter_length() (the function that calculates the length of a test BPF filter) behavior is to declare the end of the filter as soon as it finds {0, *, *, 0}. This is actually a valid insn ("ld #0"), so any filter with includes "BPF_STMT(BPF_LD | BPF_IMM, 0)"
fails (its length is cut short).
We are changing probe_filter_length() so as to start from the end, and declare the end of the filter as the first instruction which is not {0, *, *, 0}. This solution produces a simpler patch than the alternative of using an explicit end-of-filter mark. It is technically incorrect if your filter ends up with "ld #0", but that should not happen anyway.

Quoted 4 lines

Quoted 4 lines
We also add a new test (LD_IMM_0) that includes ld #0 (does not work without this patch).
Signed-off-by Chema Gonzalez <[email protected]>

Thank you for addressing all our comments.

Acked-by Alexei Starovoitov <[email protected]>

Quoted 4 lines

Thanks (sorry for the delay as I'm currently on travel).
Acked-by Daniel Borkmann <[email protected]>

David Miller

maintainer Applied

2014-06-02 23:33 UTC #6 lore raw

In reply to Chema Gonzalez

From: Chema Gonzalez <[email protected]>
Date: Fri, 30 May 2014 10:15:12 -0700

Quoted 13 lines

The current probe_filter_length() (the function that calculates the length of a test BPF filter) behavior is to declare the end of the filter as soon as it finds {0, *, *, 0}. This is actually a valid insn ("ld #0"), so any filter with includes "BPF_STMT(BPF_LD | BPF_IMM, 0)" fails (its length is cut short).
We are changing probe_filter_length() so as to start from the end, and declare the end of the filter as the first instruction which is not {0, *, *, 0}. This solution produces a simpler patch than the alternative of using an explicit end-of-filter mark. It is technically incorrect if your filter ends up with "ld #0", but that should not happen anyway.

Quoted 4 lines

We also add a new test (LD_IMM_0) that includes ld #0 (does not work without this patch).
Signed-off-by Chema Gonzalez <[email protected]>

Applied, thank you.