selftests/bpf: don't use bpf helpers in non-bpf environment

PATCH In review Stanislav Fomichev 3 participants 7 messages Dec 11, 2018 – Dec 12, 2018

Subsystems (5)

Networking Network drivers Kselftest BPF +1 more subsystems

lore

Share & tools

lore

Stanislav Fomichev

author

2018-12-11 21:28 UTC #0 lore raw

PATCH 1/1 selftests/bpf: don't use bpf helpers in non-bpf environment

We're using bpf_htons in test_progs.c to initialize some static global data and I think I hit some weird case on an older compiler which doesn't have __builtin_bswap16 (and __builtin_constant_p expands to false).

In this case I see:
error: implicit declaration of function '__builtin_bswap16'

Let's explicitly use __constant_htons which should be exposed by the linux/byteorder.h uapi header.

Signed-off-by Stanislav Fomichev <[email protected]>

---
 tools/testing/selftests/bpf/test_progs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/testing/selftests/bpf/test_progs.c +4 -4

index 26f1fdf3e2bf..61593d319c0e 100644
@@ -51,10 +51,10 @@ static struct {
 	struct iphdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v4 = {
-	.eth.h_proto = bpf_htons(ETH_P_IP),
+	.eth.h_proto = __constant_htons(ETH_P_IP),
 	.iph.ihl = 5,
 	.iph.protocol = 6,
-	.iph.tot_len = bpf_htons(MAGIC_BYTES),
+	.iph.tot_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

@@ -64,9 +64,9 @@ static struct {
 	struct ipv6hdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v6 = {
-	.eth.h_proto = bpf_htons(ETH_P_IPV6),
+	.eth.h_proto = __constant_htons(ETH_P_IPV6),
 	.iph.nexthdr = 6,
-	.iph.payload_len = bpf_htons(MAGIC_BYTES),
+	.iph.payload_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

Signature · 1 line

2.20.0.rc2.403.gdbc3b29805-goog

Stanislav Fomichev

2018-12-11 21:49 UTC #1 lore raw

Quoted 10 lines · Stanislav Fomichev

We're using bpf_htons in test_progs.c to initialize some static global data and I think I hit some weird case on an older compiler which doesn't have __builtin_bswap16 (and __builtin_constant_p expands to false).
In this case I see:
error: implicit declaration of function '__builtin_bswap16'
Let's explicitly use __constant_htons which should be exposed by the linux/byteorder.h uapi header.

Forgot to mention, that using simple htons produces the following:
test_progs.c:54:17: error: braced-group within expression allowed only inside a function
.eth.h_proto = htons(ETH_P_IP),

Quoted 38 lines

Signed-off-by Stanislav Fomichev <[email protected]>

---
 tools/testing/selftests/bpf/test_progs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/testing/selftests/bpf/test_progs.c +4 -4

index 26f1fdf3e2bf..61593d319c0e 100644
@@ -51,10 +51,10 @@ static struct {
 	struct iphdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v4 = {
-	.eth.h_proto = bpf_htons(ETH_P_IP),
+	.eth.h_proto = __constant_htons(ETH_P_IP),
 	.iph.ihl = 5,
 	.iph.protocol = 6,
-	.iph.tot_len = bpf_htons(MAGIC_BYTES),
+	.iph.tot_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

@@ -64,9 +64,9 @@ static struct {
 	struct ipv6hdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v6 = {
-	.eth.h_proto = bpf_htons(ETH_P_IPV6),
+	.eth.h_proto = __constant_htons(ETH_P_IPV6),
 	.iph.nexthdr = 6,
-	.iph.payload_len = bpf_htons(MAGIC_BYTES),
+	.iph.payload_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

Signature · 1 line

2.20.0.rc2.403.gdbc3b29805-goog

Daniel Borkmann

maintainer

2018-12-12 01:56 UTC #2 lore raw

Quoted 8 lines · Stanislav Fomichev

Quoted 7 lines · Stanislav Fomichev
We're using bpf_htons in test_progs.c to initialize some static global data and I think I hit some weird case on an older compiler which doesn't have __builtin_bswap16 (and __builtin_constant_p expands to false).
In this case I see:
error: implicit declaration of function '__builtin_bswap16'

Is that gcc < 4.8?

Quoted 13 lines

Quoted 2 lines
Let's explicitly use __constant_htons which should be exposed by the linux/byteorder.h uapi header.
Forgot to mention, that using simple htons produces the following:
test_progs.c:54:17: error: braced-group within expression allowed only inside a function
.eth.h_proto = htons(ETH_P_IP),
Quoted 5 lines
Signed-off-by Stanislav Fomichev <[email protected]>
---
 tools/testing/selftests/bpf/test_progs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Quoted 10 lines

tools/testing/selftests/bpf/test_progs.c static struct { · line 54
Quoted 10 lines
tools/testing/selftests/bpf/test_progs.c +1 -1
index 26f1fdf3e2bf..61593d319c0e 100644
@@ -51,10 +51,10 @@ static struct {
 	struct iphdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v4 = {
-	.eth.h_proto = bpf_htons(ETH_P_IP),
+	.eth.h_proto = __constant_htons(ETH_P_IP),

If the __builtin_constant_p() evaluated to false on the constants (?), wouldn't using the __bpf_constant_htons() directly work as well given it's not using a builtin either? Should be fine either way though using the same api/header might be slightly nicer.

Quoted 22 lines

tools/testing/selftests/bpf/test_progs.c +3 -3

 	.iph.ihl = 5,
 	.iph.protocol = 6,
-	.iph.tot_len = bpf_htons(MAGIC_BYTES),
+	.iph.tot_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

@@ -64,9 +64,9 @@ static struct {
 	struct ipv6hdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v6 = {
-	.eth.h_proto = bpf_htons(ETH_P_IPV6),
+	.eth.h_proto = __constant_htons(ETH_P_IPV6),
 	.iph.nexthdr = 6,
-	.iph.payload_len = bpf_htons(MAGIC_BYTES),
+	.iph.payload_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

Signature · 1 line

2.20.0.rc2.403.gdbc3b29805-goog

Stanislav Fomichev

2018-12-12 03:13 UTC #3 lore raw

Quoted 11 lines · Daniel Borkmann

Quoted 8 lines · Stanislav Fomichev
Quoted 7 lines · Stanislav Fomichev
We're using bpf_htons in test_progs.c to initialize some static global data and I think I hit some weird case on an older compiler which doesn't have __builtin_bswap16 (and __builtin_constant_p expands to false).
In this case I see:
error: implicit declaration of function '__builtin_bswap16'
Is that gcc < 4.8?

Yes.

Quoted 13 lines

Quoted 13 lines
Quoted 2 lines
Let's explicitly use __constant_htons which should be exposed by the linux/byteorder.h uapi header.
Forgot to mention, that using simple htons produces the following:
test_progs.c:54:17: error: braced-group within expression allowed only inside a function
.eth.h_proto = htons(ETH_P_IP),
Quoted 5 lines
Signed-off-by Stanislav Fomichev <[email protected]>
---
 tools/testing/selftests/bpf/test_progs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Quoted 15 lines

Quoted 10 lines
tools/testing/selftests/bpf/test_progs.c static struct { · line 54
Quoted 10 lines
tools/testing/selftests/bpf/test_progs.c +1 -1
index 26f1fdf3e2bf..61593d319c0e 100644
@@ -51,10 +51,10 @@ static struct {
 	struct iphdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v4 = {
-	.eth.h_proto = bpf_htons(ETH_P_IP),
+	.eth.h_proto = __constant_htons(ETH_P_IP),
If the __builtin_constant_p() evaluated to false on the constants (?), wouldn't using the __bpf_constant_htons() directly work as well given it's not using a builtin either? Should be fine either way though using the same api/header might be slightly nicer.

I got it wrong, __builtin_constant_p() evaluates correctly, I played with it a bit. But for some reason it still complains about that branch that it doesn't take :-/

Using __bpf_constant_htons() is a good idea, I'll follow up with a v2.

Quoted 23 lines

Quoted 22 lines

tools/testing/selftests/bpf/test_progs.c +3 -3

 	.iph.ihl = 5,
 	.iph.protocol = 6,
-	.iph.tot_len = bpf_htons(MAGIC_BYTES),
+	.iph.tot_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

@@ -64,9 +64,9 @@ static struct {
 	struct ipv6hdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v6 = {
-	.eth.h_proto = bpf_htons(ETH_P_IPV6),
+	.eth.h_proto = __constant_htons(ETH_P_IPV6),
 	.iph.nexthdr = 6,
-	.iph.payload_len = bpf_htons(MAGIC_BYTES),
+	.iph.payload_len = __constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

Signature · 1 line

2.20.0.rc2.403.gdbc3b29805-goog

Daniel Borkmann

maintainer

2018-12-12 09:05 UTC #4 lore raw

Quoted 42 lines · Stanislav Fomichev

Quoted 11 lines · Daniel Borkmann
Quoted 8 lines · Stanislav Fomichev
Quoted 7 lines · Stanislav Fomichev
We're using bpf_htons in test_progs.c to initialize some static global data and I think I hit some weird case on an older compiler which doesn't have __builtin_bswap16 (and __builtin_constant_p expands to false).
In this case I see:
error: implicit declaration of function '__builtin_bswap16'
Is that gcc < 4.8?
Yes.
Quoted 13 lines
Quoted 13 lines
Quoted 2 lines
Let's explicitly use __constant_htons which should be exposed by the linux/byteorder.h uapi header.
Forgot to mention, that using simple htons produces the following:
test_progs.c:54:17: error: braced-group within expression allowed only inside a function
.eth.h_proto = htons(ETH_P_IP),
Quoted 5 lines
Signed-off-by Stanislav Fomichev <[email protected]>
---
 tools/testing/selftests/bpf/test_progs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Quoted 15 lines
Quoted 10 lines
tools/testing/selftests/bpf/test_progs.c static struct { · line 54
Quoted 10 lines
tools/testing/selftests/bpf/test_progs.c +1 -1
index 26f1fdf3e2bf..61593d319c0e 100644
@@ -51,10 +51,10 @@ static struct {
 	struct iphdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v4 = {
-	.eth.h_proto = bpf_htons(ETH_P_IP),
+	.eth.h_proto = __constant_htons(ETH_P_IP),
If the __builtin_constant_p() evaluated to false on the constants (?), wouldn't using the __bpf_constant_htons() directly work as well given it's not using a builtin either? Should be fine either way though using the same api/header might be slightly nicer.

Quoted 5 lines

I got it wrong, __builtin_constant_p() evaluates correctly, I played with it a bit. But for some reason it still complains about that branch that it doesn't take :-/
Using __bpf_constant_htons() is a good idea, I'll follow up with a v2.

Ok, sounds good, thanks!

Stanislav Fomichev

author

2018-12-12 03:20 UTC #5 lore raw

PATCH v2 1/1 selftests/bpf: use __bpf_constant_htons in test_prog.c

In reply to Daniel Borkmann

For some reason, my older GCC (< 4.8) isn't smart enough to optimize the !__builtin_constant_p() branch in bpf_htons, I see:
error: implicit declaration of function '__builtin_bswap16'

Let's use __bpf_constant_htons as suggested by Daniel Borkmann.

I tried to use simple htons, but it produces the following:
test_progs.c:54:17: error: braced-group within expression allowed only inside a function

    .eth.h_proto = htons(ETH_P_IP),

Signed-off-by Stanislav Fomichev <[email protected]>

---
 tools/testing/selftests/bpf/test_progs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/testing/selftests/bpf/test_progs.c +4 -4

index 26f1fdf3e2bf..126fc624290d 100644
@@ -51,10 +51,10 @@ static struct {
 	struct iphdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v4 = {
-	.eth.h_proto = bpf_htons(ETH_P_IP),
+	.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
 	.iph.ihl = 5,
 	.iph.protocol = 6,
-	.iph.tot_len = bpf_htons(MAGIC_BYTES),
+	.iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

@@ -64,9 +64,9 @@ static struct {
 	struct ipv6hdr iph;
 	struct tcphdr tcp;
 } __packed pkt_v6 = {
-	.eth.h_proto = bpf_htons(ETH_P_IPV6),
+	.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
 	.iph.nexthdr = 6,
-	.iph.payload_len = bpf_htons(MAGIC_BYTES),
+	.iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
 	.tcp.urg_ptr = 123,
 };

Signature · 1 line

2.20.0.rc2.403.gdbc3b29805-goog

Daniel Borkmann

maintainer Applied

2018-12-12 09:50 UTC #6 lore raw

Re: [PATCH v2 bpf-next] selftests/bpf: use __bpf_constant_htons in test_prog.c

Quoted 4 lines · Stanislav Fomichev

For some reason, my older GCC (< 4.8) isn't smart enough to optimize the !__builtin_constant_p() branch in bpf_htons, I see:
error: implicit declaration of function '__builtin_bswap16'

Quoted 8 lines

Let's use __bpf_constant_htons as suggested by Daniel Borkmann.
I tried to use simple htons, but it produces the following:
test_progs.c:54:17: error: braced-group within expression allowed only inside a function
    .eth.h_proto = htons(ETH_P_IP),
Signed-off-by Stanislav Fomichev <[email protected]>

Applied, thanks!