* [PATCH v3 net-next 0/2] tools: psock_tpacket bug fixes
@ 2017-01-04 18:45 Sowmini Varadhan
2017-01-04 18:45 ` [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter Sowmini Varadhan
2017-01-04 18:45 ` [PATCH v3 net-next 2/2] tools: psock_tpacket: block Rx until socket filter has been added and socket has been bound to loopback Sowmini Varadhan
0 siblings, 2 replies; 14+ messages in thread
From: Sowmini Varadhan @ 2017-01-04 18:45 UTC (permalink / raw)
To: linux-kselftest, netdev, sowmini.varadhan; +Cc: daniel, willemb, davem, shuah
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(-)
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 18:45 [PATCH v3 net-next 0/2] tools: psock_tpacket bug fixes Sowmini Varadhan @ 2017-01-04 18:45 ` Sowmini Varadhan 2017-01-04 22:16 ` Daniel Borkmann 2017-01-04 22:37 ` Shuah Khan 2017-01-04 18:45 ` [PATCH v3 net-next 2/2] tools: psock_tpacket: block Rx until socket filter has been added and socket has been bound to loopback Sowmini Varadhan 1 sibling, 2 replies; 14+ messages in thread From: Sowmini Varadhan @ 2017-01-04 18:45 UTC (permalink / raw) To: linux-kselftest, netdev, sowmini.varadhan; +Cc: daniel, willemb, davem, shuah 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 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 18:45 ` [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter Sowmini Varadhan @ 2017-01-04 22:16 ` Daniel Borkmann 2017-01-04 22:22 ` Sowmini Varadhan 2017-01-04 22:37 ` Shuah Khan 1 sibling, 1 reply; 14+ messages in thread From: Daniel Borkmann @ 2017-01-04 22:16 UTC (permalink / raw) To: Sowmini Varadhan, linux-kselftest, netdev; +Cc: willemb, davem, shuah On 01/04/2017 07:45 PM, 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 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 */ 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; > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 22:16 ` Daniel Borkmann @ 2017-01-04 22:22 ` Sowmini Varadhan 2017-01-04 22:26 ` Daniel Borkmann 0 siblings, 1 reply; 14+ messages in thread From: Sowmini Varadhan @ 2017-01-04 22:22 UTC (permalink / raw) To: Daniel Borkmann; +Cc: linux-kselftest, netdev, willemb, davem, shuah 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 22:22 ` Sowmini Varadhan @ 2017-01-04 22:26 ` Daniel Borkmann 2017-01-04 22:48 ` Sowmini Varadhan 0 siblings, 1 reply; 14+ messages in thread From: Daniel Borkmann @ 2017-01-04 22:26 UTC (permalink / raw) To: Sowmini Varadhan; +Cc: linux-kselftest, netdev, willemb, davem, shuah On 01/04/2017 11:22 PM, Sowmini Varadhan wrote: > 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 . cBPF with its tooling is all documented here: Documentation/networking/filter.txt >> 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 22:26 ` Daniel Borkmann @ 2017-01-04 22:48 ` Sowmini Varadhan 2017-01-04 22:59 ` Daniel Borkmann 0 siblings, 1 reply; 14+ messages in thread From: Sowmini Varadhan @ 2017-01-04 22:48 UTC (permalink / raw) To: Daniel Borkmann; +Cc: linux-kselftest, netdev, willemb, davem, shuah On (01/04/17 23:26), Daniel Borkmann wrote: > > >>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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 22:48 ` Sowmini Varadhan @ 2017-01-04 22:59 ` Daniel Borkmann 0 siblings, 0 replies; 14+ messages in thread From: Daniel Borkmann @ 2017-01-04 22:59 UTC (permalink / raw) To: Sowmini Varadhan; +Cc: linux-kselftest, netdev, willemb, davem, shuah On 01/04/2017 11:48 PM, Sowmini Varadhan wrote: > On (01/04/17 23:26), Daniel Borkmann wrote: [...] >>>> 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 18:45 ` [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter Sowmini Varadhan 2017-01-04 22:16 ` Daniel Borkmann @ 2017-01-04 22:37 ` Shuah Khan 2017-01-04 22:49 ` Sowmini Varadhan 2017-01-04 22:55 ` Sowmini Varadhan 1 sibling, 2 replies; 14+ messages in thread From: Shuah Khan @ 2017-01-04 22:37 UTC (permalink / raw) To: Sowmini Varadhan, linux-kselftest, netdev Cc: daniel, willemb, davem, Shuah Khan On 01/04/2017 11:45 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 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' > + */ 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; > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 22:37 ` Shuah Khan @ 2017-01-04 22:49 ` Sowmini Varadhan 2017-01-04 22:55 ` Sowmini Varadhan 1 sibling, 0 replies; 14+ messages in thread From: Sowmini Varadhan @ 2017-01-04 22:49 UTC (permalink / raw) To: Shuah Khan; +Cc: linux-kselftest, netdev, daniel, willemb, davem 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 22:37 ` Shuah Khan 2017-01-04 22:49 ` Sowmini Varadhan @ 2017-01-04 22:55 ` Sowmini Varadhan 2017-01-04 23:26 ` Shuah Khan 1 sibling, 1 reply; 14+ messages in thread From: Sowmini Varadhan @ 2017-01-04 22:55 UTC (permalink / raw) To: Shuah Khan; +Cc: linux-kselftest, netdev, daniel, willemb, davem On (01/04/17 15:37), Shuah Khan wrote: > > + /* 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 22:55 ` Sowmini Varadhan @ 2017-01-04 23:26 ` Shuah Khan 2017-01-05 15:54 ` Sowmini Varadhan 0 siblings, 1 reply; 14+ messages in thread From: Shuah Khan @ 2017-01-04 23:26 UTC (permalink / raw) To: Sowmini Varadhan Cc: linux-kselftest, netdev, daniel, willemb, davem, Shuah Khan On 01/04/2017 03:55 PM, Sowmini Varadhan wrote: > On (01/04/17 15:37), Shuah Khan wrote: >>> + /* 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-04 23:26 ` Shuah Khan @ 2017-01-05 15:54 ` Sowmini Varadhan 2017-01-05 18:46 ` Shuah Khan 0 siblings, 1 reply; 14+ messages in thread From: Sowmini Varadhan @ 2017-01-05 15:54 UTC (permalink / raw) To: Shuah Khan; +Cc: netdev, daniel, willemb, davem 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter 2017-01-05 15:54 ` Sowmini Varadhan @ 2017-01-05 18:46 ` Shuah Khan 0 siblings, 0 replies; 14+ messages in thread From: Shuah Khan @ 2017-01-05 18:46 UTC (permalink / raw) To: Sowmini Varadhan; +Cc: netdev, daniel, willemb, davem, Shuah Khan On 01/05/2017 08:54 AM, Sowmini Varadhan wrote: > 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 > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 net-next 2/2] tools: psock_tpacket: block Rx until socket filter has been added and socket has been bound to loopback. 2017-01-04 18:45 [PATCH v3 net-next 0/2] tools: psock_tpacket bug fixes Sowmini Varadhan 2017-01-04 18:45 ` [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter Sowmini Varadhan @ 2017-01-04 18:45 ` Sowmini Varadhan 1 sibling, 0 replies; 14+ messages in thread From: Sowmini Varadhan @ 2017-01-04 18:45 UTC (permalink / raw) To: linux-kselftest, netdev, sowmini.varadhan; +Cc: daniel, willemb, davem, shuah 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-01-05 18:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-04 18:45 [PATCH v3 net-next 0/2] tools: psock_tpacket bug fixes Sowmini Varadhan 2017-01-04 18:45 ` [PATCH v3 net-next 1/2] tools: psock_lib: tighten conditions checked in sock_setfilter Sowmini Varadhan 2017-01-04 22:16 ` Daniel Borkmann 2017-01-04 22:22 ` Sowmini Varadhan 2017-01-04 22:26 ` Daniel Borkmann 2017-01-04 22:48 ` Sowmini Varadhan 2017-01-04 22:59 ` Daniel Borkmann 2017-01-04 22:37 ` Shuah Khan 2017-01-04 22:49 ` Sowmini Varadhan 2017-01-04 22:55 ` Sowmini Varadhan 2017-01-04 23:26 ` Shuah Khan 2017-01-05 15:54 ` Sowmini Varadhan 2017-01-05 18:46 ` Shuah Khan 2017-01-04 18:45 ` [PATCH v3 net-next 2/2] tools: psock_tpacket: block Rx until socket filter has been added and socket has been bound to loopback Sowmini Varadhan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).