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

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.

v2: patch 2 reworked based on review comments.
v3: Shuah Khan nit.


Sowmini Varadhan (2):
  tools: psock_lib: 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     |   29 ++++++++++++++++++++------
 tools/testing/selftests/net/psock_tpacket.c |    6 ++--
 2 files changed, 25 insertions(+), 10 deletions(-)

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 <redacted>
Acked-by: Willem de Bruijn <willemb@google.com>
---
v2: commit comment edited based on Willem de Bruijn review
v3: Shuah Khan nit.

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

diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h
index 24bc7ec..9e5553a 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,28 @@
 
 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

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 <redacted>
---
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

On 01/04/2017 07:45 PM, Sowmini Varadhan wrote:

Just reading up on the thread, sorry to jump in late. Can't you just
use the generated code from bpf_asm (tools/net/) and add the asm program
as a comment above? Something like we do in net/core/ptp_classifier.c +13.
As it stands it makes it a bit harder to parse / less readable with macros
actually. Rest seems fine, thanks.

  	};
  	struct sock_fprog bpf_prog;

On (01/04/17 23:16), Daniel Borkmann wrote:

Just reading up on the thread, sorry to jump in late. Can't you just
use the generated code from bpf_asm (tools/net/) and add the asm program
as a comment above? Something like we do in net/core/ptp_classifier.c +13.

I was actually using the example from the BSD bpf(4) man page, 
and expanding on that one.. 
  https://www.freebsd.org/cgi/man.cgi?query=bpf&sektion=4&manpath=FreeBSD+4.7-RELEASE
(I could not find the equivalent linux man page).

It was a lot easier to parse than the existing code .

As it stands it makes it a bit harder to parse / less readable with macros
actually. Rest seems fine, thanks.

You think the earlier code was readable? I had to use
gcc -E, with help from the bpf(4) page, to make sense of it.

--Sowmini

On 01/04/2017 11:22 PM, Sowmini Varadhan wrote:

On (01/04/17 23:16), Daniel Borkmann wrote:

quoted

Just reading up on the thread, sorry to jump in late. Can't you just
use the generated code from bpf_asm (tools/net/) and add the asm program
as a comment above? Something like we do in net/core/ptp_classifier.c +13.

I was actually using the example from the BSD bpf(4) man page,
and expanding on that one..
   https://www.freebsd.org/cgi/man.cgi?query=bpf&sektion=4&manpath=FreeBSD+4.7-RELEASE
(I could not find the equivalent linux man page).

It was a lot easier to parse than the existing code .

cBPF with its tooling is all documented here:

   Documentation/networking/filter.txt

quoted

As it stands it makes it a bit harder to parse / less readable with macros
actually. Rest seems fine, thanks.

You think the earlier code was readable? I had to use
gcc -E, with help from the bpf(4) page, to make sense of it.

--Sowmini

On 01/04/2017 11:45 AM, Sowmini Varadhan wrote:

Looks like you have to do v4 anyway, please make sure your comment
block is one of the acceptable formats based on coding style:

https://marc.info/?l=linux-crypto-vger&m=146799837129319&w=2

 	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;
 

On (01/04/17 23:26), Daniel Borkmann wrote:

quoted

quoted

As it stands it makes it a bit harder to parse / less readable with macros
actually. Rest seems fine, thanks.

Usually macros are there (a) as an abstraction so you 
dont have to hard-code things, and, (b) to make things 
more readable. (maybe that's why the 1992 VJ paper on 
BPF came up with these macros?)

I think we differ on code-aesthetics (not correctness) here. 
It was not immediately obvious to me that "0x15 is actually 
BPF_JMP + BPF_JEQ + BPF_K" etc, when I wanted to extend
the bpf_prog to harden the checks in the existing code.

Would it be ok to leave the extremely subjective 
"make this more readable" part for you to tackle later? 
Or I can just drop patch1, and you can fix it to your 
satisfaction later.

--Sowmini

On (01/04/17 15:37), Shuah Khan wrote:

Looks like you have to do v4 anyway, please make sure your comment
block is one of the acceptable formats based on coding style:

I'm not sure about that.  I can just keep patch 2.

thanks,
--Sowmini

On (01/04/17 15:37), Shuah Khan wrote:

quoted

+	/* 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'
+	 */

Looks like you have to do v4 anyway, please make sure your comment
block is one of the acceptable formats based on coding style:

https://marc.info/?l=linux-crypto-vger&m=146799837129319&w=2

BTW, the above is conformant with the comment style required for
networking:

https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

which seems to be used in psock_fanout.c and reuseport_bpf.c as well.

Thanks
--Sowmini

On 01/04/2017 11:48 PM, Sowmini Varadhan wrote:

On (01/04/17 23:26), Daniel Borkmann wrote:

[...]

quoted

quoted

quoted

As it stands it makes it a bit harder to parse / less readable with macros
actually. Rest seems fine, thanks.

Usually macros are there (a) as an abstraction so you
dont have to hard-code things, and, (b) to make things
more readable. (maybe that's why the 1992 VJ paper on
BPF came up with these macros?)

I think we differ on code-aesthetics (not correctness) here.
It was not immediately obvious to me that "0x15 is actually
BPF_JMP + BPF_JEQ + BPF_K" etc, when I wanted to extend
the bpf_prog to harden the checks in the existing code.

Would it be ok to leave the extremely subjective
"make this more readable" part for you to tackle later?
Or I can just drop patch1, and you can fix it to your
satisfaction later.

I think we're talking past each other (?), my suggestion
from my original email was to use bpf_asm and paste the
(human readable) program as a comment above as done also
elsewhere. But just leave it as it is then, no big deal
either.

On 01/04/2017 03:55 PM, Sowmini Varadhan wrote:

On (01/04/17 15:37), Shuah Khan wrote:

quoted

quoted

+	/* 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'
+	 */

Looks like you have to do v4 anyway, please make sure your comment
block is one of the acceptable formats based on coding style:

https://marc.info/?l=linux-crypto-vger&m=146799837129319&w=2

BTW, the above is conformant with the comment style required for
networking:

https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

which seems to be used in psock_fanout.c and reuseport_bpf.c as well.

I would like to see the comment blocks in selftest consistent with the
Kernel coding style.

Thanks
--Sowmini

Could you please split this patch into two. Hardening part in one and
the cleanup in a separate patch. This way I can get the hardening fix
into 4.10 in my next Kselftest update. Cleanup patch can go in later.

thanks,
-- Shuah

On (01/04/17 16:26), Shuah Khan wrote:

Could you please split this patch into two. Hardening part in one and
the cleanup in a separate patch. This way I can get the hardening fix
into 4.10 in my next Kselftest update. Cleanup patch can go in later.

thanks,
-- Shuah

I'm a little confused by the comments above.

Dan's suggestion was that I could have used some other
tool to generate the code, rather than hand-crafting it as I did.
In his last message, he suggests that it may be ok to leave
the hand-crafted version as is (for now), as well.

To make it clear:
the current v3 version *is* the "hardening" part. Dan's suggestion is
that the hand-crafted version can be replaced by bpf_asm generated code
later. That would be the "cleanup" part, which I was going to do in a
later commit.

Does that help?

--Sowmini

On 01/05/2017 08:54 AM, Sowmini Varadhan wrote:

On (01/04/17 16:26), Shuah Khan wrote:

quoted

Could you please split this patch into two. Hardening part in one and
the cleanup in a separate patch. This way I can get the hardening fix
into 4.10 in my next Kselftest update. Cleanup patch can go in later.

thanks,
-- Shuah

I'm a little confused by the comments above.

Dan's suggestion was that I could have used some other
tool to generate the code, rather than hand-crafting it as I did.
In his last message, he suggests that it may be ok to leave
the hand-crafted version as is (for now), as well.

To make it clear:
the current v3 version *is* the "hardening" part. Dan's suggestion is
that the hand-crafted version can be replaced by bpf_asm generated code
later. That would be the "cleanup" part, which I was going to do in a
later commit.

Does that help?

--Sowmini

Let's try this again. I want to see a separate patch for the
filter cleanup. I don't want that included in the non-udp packet
check. Please address the readability review comments from me and
Daniel when you send your next version.

thanks,
-- Shuah