[PATCH v2 net-next 0/2] tools: psock_tpacket bug fixes

[PATCH v2 net-next 0/2] tools: psock_tpacket bug fixes

[Sowmini Varadhan]

This patchset includes fixes to psock_tpacket for false-negatives
sporadically reported by the test when it was run concurrently with
other heavy network traffic (e.g., over an ssh session, as opposed
to running the test from the console of the test machine). The
test sometimes failed with errors reporting more recvd packets than 
expected (e.g., "walk_v0_rx: received 201 out of 100 pkts") or
the reception of non-IP packets (e.g., ARP packets).

There are 2 sources of network interference that can disrupt the test:

1. set_sockfilter() can use some hardening (currently passes up packets
   based on ip length field, and payload signature but this may potentially
   match other network traffic on the test machine) 

2. There is a race-window between packet_create() and packet_do_bind()
   in which packets from any interface (e.g., eth0) will get queued
   for Rx on the test socket.

Patch 1 fixes the first issue by cleaing up set_sockfilter() and
hardening it to make sure that it only permits UDP/IPv4 packets.

Patch 2 fixes the second issue by making sure we open the PF_PACKET
socket with protocol 0 to reject all packets, and make sure the
BPF filter is set up before binding the socket to ETH_P_ALL and lo.

Changes from v2: patch 2 reworked based on review comments.

Sowmini Varadhan (2):
  tools: tighten conditions checked in sock_setfilter
  tools: psock_tpacket: block Rx until socket filter has been added and
    socket has been bound to loopback.

 tools/testing/selftests/net/psock_lib.h     |   28 ++++++++++++++++++++------
 tools/testing/selftests/net/psock_tpacket.c |    6 ++--
 2 files changed, 24 insertions(+), 10 deletions(-)

[PATCH v2 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter

[Sowmini Varadhan]

The bpf_prog used in sock_setfilter() only attempts to check for
ip pktlen, and verifies that the contents of the 80'th packet in
the ethernet frame is 'a' or 'b'.  Thus many non-udp packets
could incorrectly pass through this filter and cause incorrect
test results.

This commit hardens the conditions checked by the filter so
that only UDP/IPv4 packets with the matching length and test-character
will be permitted by the filter. The filter has been cleaned up
to explicitly use the BPF macros to make it more readable.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Acked-by: Willem de Bruijn <willemb@google.com>
---
v2: commit comment edited based on review

 tools/testing/selftests/net/psock_lib.h |   28 +++++++++++++++++++++-------
 1 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h
index 24bc7ec..e62540e 100644
--- a/tools/testing/selftests/net/psock_lib.h
+++ b/tools/testing/selftests/net/psock_lib.h
@@ -27,6 +27,7 @@
 #include <string.h>
 #include <arpa/inet.h>
 #include <unistd.h>
+#include <netinet/udp.h>
 
 #define DATA_LEN			100
 #define DATA_CHAR			'a'
@@ -40,14 +41,27 @@
 
 static __maybe_unused void sock_setfilter(int fd, int lvl, int optnum)
 {
+	uint16_t ip_len = DATA_LEN +
+			  sizeof(struct iphdr) + sizeof(struct udphdr);
+	/* the filter below checks for all of the following conditions that
+	 * are based on the contents of create_payload()
+	 *  ether type 0x800 and
+	 *  ip proto udp     and
+	 *  ip len == ip_len and
+	 *  udp[38] == 'a' or udp[38] == 'b'
+	 */
 	struct sock_filter bpf_filter[] = {
-		{ 0x80, 0, 0, 0x00000000 },  /* LD  pktlen		      */
-		{ 0x35, 0, 4, DATA_LEN   },  /* JGE DATA_LEN  [f goto nomatch]*/
-		{ 0x30, 0, 0, 0x00000050 },  /* LD  ip[80]		      */
-		{ 0x15, 1, 0, DATA_CHAR  },  /* JEQ DATA_CHAR   [t goto match]*/
-		{ 0x15, 0, 1, DATA_CHAR_1},  /* JEQ DATA_CHAR_1 [t goto match]*/
-		{ 0x06, 0, 0, 0x00000060 },  /* RET match	              */
-		{ 0x06, 0, 0, 0x00000000 },  /* RET no match		      */
+		BPF_STMT(BPF_LD | BPF_H | BPF_ABS, 12),	/* LD ethertype */
+		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ETH_P_IP, 0, 8),
+		BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 23),	/* LD ip_proto */
+		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, IPPROTO_UDP, 0, 6),
+		BPF_STMT(BPF_LD|BPF_H|BPF_ABS, 16),	/* LD ip_len */
+		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ip_len, 0, 4),
+		BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 80),	/* LD udp[38] */
+		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR, 1, 0),
+		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR_1, 0, 1),
+		BPF_STMT(BPF_RET | BPF_K, ~0),		/* match */
+		BPF_STMT(BPF_RET | BPF_K, 0)		/* no match */
 	};
 	struct sock_fprog bpf_prog;
 
-- 
1.7.1

[PATCH v2 net-next 2/2] tools: psock_tpacket: block Rx until socket filter has been added and socket has been bound to loopback.

[Sowmini Varadhan]

Packets from any/all interfaces may be queued up on the PF_PACKET socket
before it is bound to the loopback interface by psock_tpacket, and
when these are passed up by the kernel, they could interfere
with the Rx tests.

Avoid interference from spurious packet by blocking Rx until the
socket filter has been set up, and the packet has been bound to the
desired (lo) interface. The effective sequence is

	socket(PF_PACKET, SOCK_RAW, 0)
	set up ring
	Invoke SO_ATTACH_FILTER
	bind to sll_protocol set to ETH_P_ALL, sll_ifindex for lo

After this sequence, the only packets that will be passed up are
those received on loopback that pass the attached filter.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
v2: patch reworked based on comments from Willem de Bruijn

 tools/testing/selftests/net/psock_tpacket.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/psock_tpacket.c b/tools/testing/selftests/net/psock_tpacket.c
index 4a1bc64..7f6cd9f 100644
--- a/tools/testing/selftests/net/psock_tpacket.c
+++ b/tools/testing/selftests/net/psock_tpacket.c
@@ -110,7 +110,7 @@ struct block_desc {
 
 static int pfsocket(int ver)
 {
-	int ret, sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
+	int ret, sock = socket(PF_PACKET, SOCK_RAW, 0);
 	if (sock == -1) {
 		perror("socket");
 		exit(1);
@@ -239,7 +239,6 @@ static void walk_v1_v2_rx(int sock, struct ring *ring)
 	bug_on(ring->type != PACKET_RX_RING);
 
 	pair_udp_open(udp_sock, PORT_BASE);
-	pair_udp_setfilter(sock);
 
 	memset(&pfd, 0, sizeof(pfd));
 	pfd.fd = sock;
@@ -601,7 +600,6 @@ static void walk_v3_rx(int sock, struct ring *ring)
 	bug_on(ring->type != PACKET_RX_RING);
 
 	pair_udp_open(udp_sock, PORT_BASE);
-	pair_udp_setfilter(sock);
 
 	memset(&pfd, 0, sizeof(pfd));
 	pfd.fd = sock;
@@ -741,6 +739,8 @@ static void bind_ring(int sock, struct ring *ring)
 {
 	int ret;
 
+	pair_udp_setfilter(sock);
+
 	ring->ll.sll_family = PF_PACKET;
 	ring->ll.sll_protocol = htons(ETH_P_ALL);
 	ring->ll.sll_ifindex = if_nametoindex("lo");
-- 
1.7.1

Re: [PATCH v2 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter

[Shuah Khan]

Hi Sowmini,

Thanks for the patch.

On 01/04/2017 10:33 AM, Sowmini Varadhan wrote:
> The bpf_prog used in sock_setfilter() only attempts to check for
> ip pktlen, and verifies that the contents of the 80'th packet in
> the ethernet frame is 'a' or 'b'.  Thus many non-udp packets
> could incorrectly pass through this filter and cause incorrect
> test results.
> 
> This commit hardens the conditions checked by the filter so
> that only UDP/IPv4 packets with the matching length and test-character
> will be permitted by the filter. The filter has been cleaned up
> to explicitly use the BPF macros to make it more readable.
> 
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> Acked-by: Willem de Bruijn <willemb@google.com>
> ---
> v2: commit comment edited based on review
> 
>  tools/testing/selftests/net/psock_lib.h |   28 +++++++++++++++++++++-------
>  1 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h
> index 24bc7ec..e62540e 100644
> --- a/tools/testing/selftests/net/psock_lib.h
> +++ b/tools/testing/selftests/net/psock_lib.h
> @@ -27,6 +27,7 @@
>  #include <string.h>
>  #include <arpa/inet.h>
>  #include <unistd.h>
> +#include <netinet/udp.h>
>  
>  #define DATA_LEN			100
>  #define DATA_CHAR			'a'
> @@ -40,14 +41,27 @@
>  
>  static __maybe_unused void sock_setfilter(int fd, int lvl, int optnum)
>  {
> +	uint16_t ip_len = DATA_LEN +
> +			  sizeof(struct iphdr) + sizeof(struct udphdr);

This following will improve readability.

uint16_t ip_len = DATA_LEN +
                  sizeof(struct iphdr) +
                  sizeof(struct udphdr);

> +	/* the filter below checks for all of the following conditions that

Change the above to the following. Makes it easier to read.

        /*
         * the filter below checks for all of the following conditions that
> +	 * are based on the contents of create_payload()
> +	 *  ether type 0x800 and
> +	 *  ip proto udp     and
> +	 *  ip len == ip_len and
> +	 *  udp[38] == 'a' or udp[38] == 'b'
> +	 */

>  	struct sock_filter bpf_filter[] = {
> -		{ 0x80, 0, 0, 0x00000000 },  /* LD  pktlen		      */
> -		{ 0x35, 0, 4, DATA_LEN   },  /* JGE DATA_LEN  [f goto nomatch]*/
> -		{ 0x30, 0, 0, 0x00000050 },  /* LD  ip[80]		      */
> -		{ 0x15, 1, 0, DATA_CHAR  },  /* JEQ DATA_CHAR   [t goto match]*/
> -		{ 0x15, 0, 1, DATA_CHAR_1},  /* JEQ DATA_CHAR_1 [t goto match]*/
> -		{ 0x06, 0, 0, 0x00000060 },  /* RET match	              */
> -		{ 0x06, 0, 0, 0x00000000 },  /* RET no match		      */
> +		BPF_STMT(BPF_LD | BPF_H | BPF_ABS, 12),	/* LD ethertype */
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ETH_P_IP, 0, 8),
> +		BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 23),	/* LD ip_proto */
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, IPPROTO_UDP, 0, 6),
> +		BPF_STMT(BPF_LD|BPF_H|BPF_ABS, 16),	/* LD ip_len */
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ip_len, 0, 4),
> +		BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 80),	/* LD udp[38] */
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR, 1, 0),
> +		BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR_1, 0, 1),
> +		BPF_STMT(BPF_RET | BPF_K, ~0),		/* match */
> +		BPF_STMT(BPF_RET | BPF_K, 0)		/* no match */
>  	};
>  	struct sock_fprog bpf_prog;
>  
> 

thanks,
-- Shuah