| Message ID | 3451a2008d953f33d6576a35eaefecad883eaeb5.1483482971.git.sowmini.varadhan@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jan 3, 2017 at 6:27 PM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > 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 should not be counted > toward the conditions needed to pass/fail the Rx tests. The common and simpler solution to this problem is to open the socket with protocol 0 to reject all packets, add the BPF filter and only then bind with sll_ifindex set to lo. That way no false positives can arrive.
On (01/04/17 09:30), Willem de Bruijn wrote: > > The common and simpler solution to this problem is to open the socket > with protocol 0 to reject all packets, add the BPF filter and only then bind > with sll_ifindex set to lo. That way no false positives can arrive. Yes, I thought of that too (and I've seen that done in one commercial implementation), but given that tpacket nicely returns the incoming interface, I figured, why not use the test prog to use this (thus verifying it, and also showing how to use it) --Sowmini
On Wed, Jan 4, 2017 at 9:44 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (01/04/17 09:30), Willem de Bruijn wrote: >> >> The common and simpler solution to this problem is to open the socket >> with protocol 0 to reject all packets, add the BPF filter and only then bind >> with sll_ifindex set to lo. That way no false positives can arrive. > > Yes, I thought of that too (and I've seen that done in one commercial > implementation), but given that tpacket nicely returns the incoming > interface, I figured, why not use the test prog to use this (thus > verifying it, and also showing how to use it) This approach is less restrictive. It still allows incorrect packets to be enqueued in the time between the socket call and attaching the bpf filter. Also, if packets are restricted to a single packet, using bind with sll_ifindex is simpler.
On (01/04/17 10:03), Willem de Bruijn wrote: > > This approach is less restrictive. It still allows incorrect packets > to be enqueued in the time between the socket call and attaching the > bpf filter. Also, if packets are restricted to a single packet, using > bind with sll_ifindex is simpler. Do you want me to change this to first set up pfsocket() with proto 0, then set up filter, and then bind_ring() to the desired ifindex with ETH_P_ALL? I can spin out v2 (and if I have to that, I can also fix the comments) if you feel strongly about it. --Sowmini
On Wed, Jan 4, 2017 at 10:13 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (01/04/17 10:03), Willem de Bruijn wrote: >> >> This approach is less restrictive. It still allows incorrect packets >> to be enqueued in the time between the socket call and attaching the >> bpf filter. Also, if packets are restricted to a single packet, using >> bind with sll_ifindex is simpler. > > Do you want me to change this to first set up pfsocket() with > proto 0, then set up filter, and then bind_ring() to the desired > ifindex with ETH_P_ALL? Please do. Then the patch is just a one-line change to the third argument of the socket call. Thanks!
On (01/04/17 11:07), Willem de Bruijn wrote: > > Please do. Then the patch is just a one-line change to > the third argument of the socket call. Thanks! ok but it's going to be more than a one-line change. Today you have sock = pfsocket(version); memset(&ring, 0, sizeof(ring)); setup_ring(sock, &ring, version, type); mmap_ring(sock, &ring); bind_ring(sock, &ring); walk_ring(sock, &ring); And the pair_udp_setfilter() only gets set up out of walk_ring. I think you want to move that up to be done before bind_ring.
On Wed, Jan 4, 2017 at 11:12 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (01/04/17 11:07), Willem de Bruijn wrote: >> >> Please do. Then the patch is just a one-line change to >> the third argument of the socket call. Thanks! > > ok but it's going to be more than a one-line change. Today you > have > sock = pfsocket(version); > memset(&ring, 0, sizeof(ring)); > setup_ring(sock, &ring, version, type); > mmap_ring(sock, &ring); > bind_ring(sock, &ring); > walk_ring(sock, &ring); > > And the pair_udp_setfilter() only gets set up out of > walk_ring. I think you want to move that up to be done before > bind_ring. Oh, good point. It may require some more refactoring. Feel free to leave it for me if you prefer.
On (01/04/17 11:24), Willem de Bruijn wrote: > > Oh, good point. It may require some more refactoring. Feel free to > leave it for me if you prefer. actually it may not be so bad, let me do it, since I already have a reliable way of reproducing this.. --Sowmini
diff --git a/tools/testing/selftests/net/psock_tpacket.c b/tools/testing/selftests/net/psock_tpacket.c index 4a1bc64..3c3f1aa 100644 --- a/tools/testing/selftests/net/psock_tpacket.c +++ b/tools/testing/selftests/net/psock_tpacket.c @@ -235,6 +235,7 @@ static void walk_v1_v2_rx(int sock, struct ring *ring) int udp_sock[2]; union frame_map ppd; unsigned int frame_num = 0; + int ifindex = ring->ll.sll_ifindex; bug_on(ring->type != PACKET_RX_RING); @@ -255,12 +256,18 @@ static void walk_v1_v2_rx(int sock, struct ring *ring) switch (ring->version) { case TPACKET_V1: + if (ppd.v1->s_ll.sll_ifindex != ifindex) + goto skip; + test_payload((uint8_t *) ppd.raw + ppd.v1->tp_h.tp_mac, ppd.v1->tp_h.tp_snaplen); total_bytes += ppd.v1->tp_h.tp_snaplen; break; case TPACKET_V2: + if (ppd.v2->s_ll.sll_ifindex != ifindex) + goto skip; + test_payload((uint8_t *) ppd.raw + ppd.v2->tp_h.tp_mac, ppd.v2->tp_h.tp_snaplen); total_bytes += ppd.v2->tp_h.tp_snaplen; @@ -270,6 +277,7 @@ static void walk_v1_v2_rx(int sock, struct ring *ring) status_bar_update(); total_packets++; +skip: __v1_v2_rx_user_ready(ppd.raw, ring->version); frame_num = (frame_num + 1) % ring->rd_num; @@ -553,11 +561,13 @@ static void __v3_test_block_header(struct block_desc *pbd, const int block_num) __v3_test_block_seq_num(pbd); } -static void __v3_walk_block(struct block_desc *pbd, const int block_num) +static void __v3_walk_block(struct block_desc *pbd, const int block_num, + int ifindex) { int num_pkts = pbd->h1.num_pkts, i; unsigned long bytes = 0, bytes_with_padding = ALIGN_8(sizeof(*pbd)); struct tpacket3_hdr *ppd; + struct sockaddr_ll *s_ll; __v3_test_block_header(pbd, block_num); @@ -572,10 +582,15 @@ static void __v3_walk_block(struct block_desc *pbd, const int block_num) else bytes_with_padding += ALIGN_8(ppd->tp_snaplen + ppd->tp_mac); - test_payload((uint8_t *) ppd + ppd->tp_mac, ppd->tp_snaplen); + s_ll = (struct sockaddr_ll *)&ppd[1]; - status_bar_update(); - total_packets++; + if (ifindex == s_ll->sll_ifindex) { + test_payload((uint8_t *) ppd + ppd->tp_mac, + ppd->tp_snaplen); + + status_bar_update(); + total_packets++; + } ppd = (struct tpacket3_hdr *) ((uint8_t *) ppd + ppd->tp_next_offset); __sync_synchronize(); @@ -616,7 +631,7 @@ static void walk_v3_rx(int sock, struct ring *ring) while ((pbd->h1.block_status & TP_STATUS_USER) == 0) poll(&pfd, 1, 1); - __v3_walk_block(pbd, block_num); + __v3_walk_block(pbd, block_num, ring->ll.sll_ifindex); __v3_flush_block(pbd); block_num = (block_num + 1) % ring->rd_num;
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 should not be counted toward the conditions needed to pass/fail the Rx tests. psock_tpacket discards these packets by examining the sll_ifindex sent up in each frame and ensuring that this is the same as the ifindex that was used in bind_ring() Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> --- tools/testing/selftests/net/psock_tpacket.c | 25 ++++++++++++++++++++----- 1 files changed, 20 insertions(+), 5 deletions(-)