kernel: bpf: remove dead code

PATCH In review Gustavo A. R. Silva 3 participants 6 messages May 22, 2017

Subsystems (4)

Networking Network drivers BPF BPF core

lore

Share & tools

lore

Gustavo A. R. Silva

author

2017-05-22 14:08 UTC #0 lore raw

PATCH 1/1 kernel: bpf: remove dead code

Execution cannot reach NET_IP_ALIGN inside the following statement:
ip_align = strict ? 2 : NET_IP_ALIGN

Addresses-Coverity-ID: 1409762

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
NOTE: variable ip_align could also be removed and use value 2 directly.

 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/bpf/verifier.c +1 -1

index 1eddb71..94f6e46 100644
@@ -812,7 +812,7 @@ static int check_pkt_ptr_alignment(const struct bpf_reg_state *reg,
 	 * we force this to 2 which is universally what architectures use
 	 * when they don't set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
 	 */
-	ip_align = strict ? 2 : NET_IP_ALIGN;
+	ip_align = 2;
 	if ((ip_align + reg_off + off) % size != 0) {
 		verbose("misaligned packet access off %d+%d+%d size %d\n",
 			ip_align, reg_off, off, size);

Signature · 1 line

2.5.0

David Miller

maintainer

2017-05-22 14:38 UTC #1 lore raw

From: "Gustavo A. R. Silva" <[email protected]>
Date: Mon, 22 May 2017 09:07:46 -0500

Quoted 7 lines

Execution cannot reach NET_IP_ALIGN inside the following statement:
ip_align = strict ? 2 : NET_IP_ALIGN
Addresses-Coverity-ID: 1409762
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
NOTE: variable ip_align could also be removed and use value 2 directly.

Incorrect.

Some platforms define NET_IP_ALIGN to zero, so the code must remain as is.

Gustavo A. R. Silva

author

2017-05-22 14:51 UTC #2 lore raw

Hi David,

Quoting David Miller <[email protected]>:

Quoted 11 lines

From: "Gustavo A. R. Silva" <[email protected]>
Date: Mon, 22 May 2017 09:07:46 -0500
Quoted 7 lines
Execution cannot reach NET_IP_ALIGN inside the following statement:
ip_align = strict ? 2 : NET_IP_ALIGN
Addresses-Coverity-ID: 1409762
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
NOTE: variable ip_align could also be removed and use value 2 directly.

Quoted 4 lines

Incorrect.
Some platforms define NET_IP_ALIGN to zero, so the code must remain as is.

The following piece of code at kernel/bpf/verifier.c:798 is preventing value NET_IP_ALIGN to be stored in variable ip_align when _strict_ is false:

798 if (!strict || size == 1)
799 return 0;

Thanks

Daniel Borkmann

maintainer

2017-05-22 14:52 UTC #3 lore raw

In reply to David Miller

Quoted 11 lines · David Miller

From: "Gustavo A. R. Silva" <[email protected]>
Date: Mon, 22 May 2017 09:07:46 -0500
Quoted 7 lines
Execution cannot reach NET_IP_ALIGN inside the following statement:
ip_align = strict ? 2 : NET_IP_ALIGN
Addresses-Coverity-ID: 1409762
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
NOTE: variable ip_align could also be removed and use value 2 directly.

Quoted 4 lines

Incorrect.
Some platforms define NET_IP_ALIGN to zero, so the code must remain as is.

In the check_pkt_ptr_alignment(), when !strict you would already return earlier from that function.

So, above test in ip_align will always give 2, meaning technically the patch is correct, although hard-coded value less clean.

Perhaps something like the below to keep intentions more clear (and it will get resolved during compile time anyway ...):

include/linux/skbuff.h +3 -1

index a098d95..3cf1d60 100644
@@ -2297,8 +2297,10 @@ static inline int pskb_network_may_pull(struct sk_buff *skb, unsigned int len)
  * Since this trade off varies between architectures, we allow NET_IP_ALIGN
  * to be overridden.
  */
+#define NET_IP_ALIGN_DEFAULT	2
+
 #ifndef NET_IP_ALIGN
-#define NET_IP_ALIGN	2
+#define NET_IP_ALIGN	NET_IP_ALIGN_DEFAULT
 #endif
 
 /*

kernel/bpf/verifier.c +1 -1

index 1eddb71..61f6aaa 100644
@@ -812,7 +812,7 @@ static int check_pkt_ptr_alignment(const struct bpf_reg_state *reg,
 	 * we force this to 2 which is universally what architectures use
 	 * when they don't set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
 	 */
-	ip_align = strict ? 2 : NET_IP_ALIGN;
+	ip_align = NET_IP_ALIGN ? : NET_IP_ALIGN_DEFAULT;
 	if ((ip_align + reg_off + off) % size != 0) {
 		verbose("misaligned packet access off %d+%d+%d size %d\n",
 			ip_align, reg_off, off, size);

David Miller

maintainer

2017-05-22 16:27 UTC #4 lore raw

From: Daniel Borkmann <[email protected]>
Date: Mon, 22 May 2017 16:52:24 +0200

Quoted 18 lines · David Miller

Quoted 12 lines
From: "Gustavo A. R. Silva" <[email protected]>
Date: Mon, 22 May 2017 09:07:46 -0500
Quoted 8 lines
Execution cannot reach NET_IP_ALIGN inside the following statement:
ip_align = strict ? 2 : NET_IP_ALIGN
Addresses-Coverity-ID: 1409762
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
NOTE: variable ip_align could also be removed and use value 2 directly.
Quoted 4 lines
Incorrect.
Some platforms define NET_IP_ALIGN to zero, so the code must remain as is.

Quoted 8 lines

In the check_pkt_ptr_alignment(), when !strict you would already return earlier from that function.
So, above test in ip_align will always give 2, meaning technically the patch is correct, although hard-coded value less clean.
Perhaps something like the below to keep intentions more clear (and it will get resolved during compile time anyway ...):

Ok I understand the issue now. Thanks for explaining.

I guess a hard-coded value of 2 and an adjusted comment above the assignment of ip_align is the way to go.

I'll push the following, thanks everyone:

====================
net: Make IP alignment calulations clearer.

The assignmnet:

	ip_align = strict ? 2 : NET_IP_ALIGN;

in compare_pkt_ptr_alignment() trips up Coverity because we can only get to this code when strict is true, therefore ip_align will always be 2 regardless of NET_IP_ALIGN's value.

So just assign directly to '2' and explain the situation in the comment above.

Reported-by "Gustavo A. R. Silva" <[email protected]>

Signed-off-by David S. Miller <[email protected]>

---
 kernel/bpf/verifier.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

kernel/bpf/verifier.c +8 -4

index 1eddb71..c72cd41 100644
@@ -808,11 +808,15 @@ static int check_pkt_ptr_alignment(const struct bpf_reg_state *reg,
 		reg_off += reg->aux_off;
 	}
 
-	/* skb->data is NET_IP_ALIGN-ed, but for strict alignment checking
-	 * we force this to 2 which is universally what architectures use
-	 * when they don't set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
+	/* For platforms that do not have a Kconfig enabling
+	 * CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS the value of
+	 * NET_IP_ALIGN is universally set to '2'.  And on platforms
+	 * that do set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, we get
+	 * to this code only in strict mode where we want to emulate
+	 * the NET_IP_ALIGN==2 checking.  Therefore use an
+	 * unconditional IP align value of '2'.
 	 */
-	ip_align = strict ? 2 : NET_IP_ALIGN;
+	ip_align = 2;
 	if ((ip_align + reg_off + off) % size != 0) {
 		verbose("misaligned packet access off %d+%d+%d size %d\n",
 			ip_align, reg_off, off, size);

Signature · 1 line

2.4.11

Daniel Borkmann

maintainer Acked-by

2017-05-22 17:00 UTC #5 lore raw

On 05/22/2017 06:27 PM, David Miller wrote:
[...]

Quoted 18 lines

Ok I understand the issue now. Thanks for explaining.
I guess a hard-coded value of 2 and an adjusted comment above the assignment of ip_align is the way to go.
I'll push the following, thanks everyone:
====================
net: Make IP alignment calulations clearer.
The assignmnet:
	ip_align = strict ? 2 : NET_IP_ALIGN;
in compare_pkt_ptr_alignment() trips up Coverity because we can only get to this code when strict is true, therefore ip_align will always be 2 regardless of NET_IP_ALIGN's value.

Quoted 5 lines

So just assign directly to '2' and explain the situation in the comment above.
Reported-by "Gustavo A. R. Silva" <[email protected]>
Signed-off-by David S. Miller <[email protected]>

Yeah, that's fine, thanks!

Acked-by Daniel Borkmann <[email protected]>