Re: [ovs-dev] [PATCH v2 1/3] conntrack: Do not create new connections from ICMP errors.

2016-12-23 Thread Daniele Di Proietto 





On 22/12/2016 18:55, "Darrell Ball"  wrote:

>
>
>On 12/22/16, 6:36 PM, "Daniele Di Proietto"  wrote:
>
>ICMP error packets (e.g. destination unreachable messages) are
>considered 'related' to another connection and are treated as part of
>that.
>
>However:
>
>* We shouldn't create new entries in the connection table if the
>  original connection is not found.  This is consistent with what the
>  kernel does.
>* We certainly shouldn't call valid_new() on the packet, because
>  valid_new() assumes the packet l4 type (might be TCP, UDP or ICMP)
>  to be consistent with the conn_key nw_proto type.
>
>Found by inspection.
>
>Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
>Signed-off-by: Daniele Di Proietto 
>---
>v2: Handle ICMP error for non existing connection in else branch without
>restructuring the whole code flow.
>---
> lib/conntrack.c |  6 +-
> tests/system-traffic.at | 27 ---
> 2 files changed, 21 insertions(+), 12 deletions(-)
>
>
>Acked-by: Darrell Ball 

Thanks, I pushed this to master and branch-2.6

>
>diff --git a/lib/conntrack.c b/lib/conntrack.c
>index 7c50a28..9bea3d9 100644
>--- a/lib/conntrack.c
>+++ b/lib/conntrack.c
>@@ -247,7 +247,11 @@ process_one(struct conntrack *ct, struct dp_packet 
> *pkt,
> }
> }
> } else {
>-conn = conn_not_found(ct, pkt, ctx, , commit, now);
>+if (ctx->related) {
>+state |= CS_INVALID;
>+} else {
>+conn = conn_not_found(ct, pkt, ctx, , commit, now);
>+}
> }
> 
> write_ct_md(pkt, state, zone, conn ? conn->mark : 0,
>diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>index 9ea6d6b..a5023d3 100644
>--- a/tests/system-traffic.at
>+++ b/tests/system-traffic.at
>@@ -1331,12 +1331,8 @@ ADD_VETH(p1, at_ns1, br0, "172.16.0.2/24")
> 
> dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from 
> ns1->ns0.
> AT_DATA([flows.txt], [dnl
>-priority=1,action=drop
>-priority=10,arp,action=normal
>-priority=100,in_port=1,udp,ct_state=-trk,action=ct(commit,table=0)
>-priority=100,in_port=1,ip,ct_state=+trk,actions=controller
>-priority=100,in_port=2,ip,ct_state=-trk,action=ct(table=0)
>-priority=100,in_port=2,ip,ct_state=+trk+rel+rpl,action=controller
>+table=0,ip,action=ct(commit,table=1)
>+table=1,ip,action=controller
> ])
> 
> AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows.txt])
>@@ -1345,22 +1341,31 @@ AT_CAPTURE_FILE([ofctl_monitor.log])
> AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir 
> --pidfile 2> ofctl_monitor.log])
> 
> dnl 1. Send an ICMP port unreach reply for port 8738, without any 
> previous request
>-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
> 'f64c473528c9c6f54ecb72db080045c0003d2e874001f355ac14ac130303553f4521317040004011b138ac13ac14000d20966369616f0a'])
>+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 resubmit\(,0\) 
> 'f64c473528c9c6f54ecb72db080045c0003d2e874001f351ac14ac130303da494521317040004011b138ac13ac14000d20966369616f0a'])
> 
> dnl 2. Send and UDP packet to port 
>-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 1 ct\(commit,table=0\) 
> 'c6f94ecb72dbe64c473528c908004521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
>+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 1 resubmit\(,0\) 
> 'c6f94ecb72dbe64c473528c908004521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
> 
> dnl 3. Send an ICMP port unreach reply for port , related to the 
> first packet
>-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
> 'e64c473528c9c6f94ecb72db080045c0003d2e874001f355ac12ac110303553f4521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
>+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 resubmit\(,0\) 
> 'e64c473528c9c6f94ecb72db080045c0003d2e874001f355ac12ac110303553f4521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
> 
> dnl Check this output. We only see the latter two packets, not the first.
> AT_CHECK([cat ofctl_monitor.log], [0], [dnl
>-NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=47 
> ct_state=new|trk,in_port=1 (via action) data_len=47 (unbuffered)
>+NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=75 
> ct_state=inv|trk,in_port=2 (via action) data_len=75 (unbuffered)
>
>

[ovs-dev] [PATCH v2 1/3] conntrack: Do not create new connections from ICMP errors.

2016-12-22 Thread Daniele Di Proietto 
ICMP error packets (e.g. destination unreachable messages) are
considered 'related' to another connection and are treated as part of
that.

However:

* We shouldn't create new entries in the connection table if the
  original connection is not found.  This is consistent with what the
  kernel does.
* We certainly shouldn't call valid_new() on the packet, because
  valid_new() assumes the packet l4 type (might be TCP, UDP or ICMP)
  to be consistent with the conn_key nw_proto type.

Found by inspection.

Fixes: a489b16854b5("conntrack: New userspace connection tracker.")
Signed-off-by: Daniele Di Proietto 
---
v2: Handle ICMP error for non existing connection in else branch without
restructuring the whole code flow.
---
 lib/conntrack.c |  6 +-
 tests/system-traffic.at | 27 ---
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 7c50a28..9bea3d9 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -247,7 +247,11 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 }
 }
 } else {
-conn = conn_not_found(ct, pkt, ctx, , commit, now);
+if (ctx->related) {
+state |= CS_INVALID;
+} else {
+conn = conn_not_found(ct, pkt, ctx, , commit, now);
+}
 }
 
 write_ct_md(pkt, state, zone, conn ? conn->mark : 0,
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 9ea6d6b..a5023d3 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1331,12 +1331,8 @@ ADD_VETH(p1, at_ns1, br0, "172.16.0.2/24")
 
 dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from 
ns1->ns0.
 AT_DATA([flows.txt], [dnl
-priority=1,action=drop
-priority=10,arp,action=normal
-priority=100,in_port=1,udp,ct_state=-trk,action=ct(commit,table=0)
-priority=100,in_port=1,ip,ct_state=+trk,actions=controller
-priority=100,in_port=2,ip,ct_state=-trk,action=ct(table=0)
-priority=100,in_port=2,ip,ct_state=+trk+rel+rpl,action=controller
+table=0,ip,action=ct(commit,table=1)
+table=1,ip,action=controller
 ])
 
 AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows.txt])
@@ -1345,22 +1341,31 @@ AT_CAPTURE_FILE([ofctl_monitor.log])
 AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl --detach --no-chdir 
--pidfile 2> ofctl_monitor.log])
 
 dnl 1. Send an ICMP port unreach reply for port 8738, without any previous 
request
-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
'f64c473528c9c6f54ecb72db080045c0003d2e874001f355ac14ac130303553f4521317040004011b138ac13ac14000d20966369616f0a'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 resubmit\(,0\) 
'f64c473528c9c6f54ecb72db080045c0003d2e874001f351ac14ac130303da494521317040004011b138ac13ac14000d20966369616f0a'])
 
 dnl 2. Send and UDP packet to port 
-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 1 ct\(commit,table=0\) 
'c6f94ecb72dbe64c473528c908004521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 1 resubmit\(,0\) 
'c6f94ecb72dbe64c473528c908004521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
 
 dnl 3. Send an ICMP port unreach reply for port , related to the first 
packet
-AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 ct\(table=0\) 
'e64c473528c9c6f94ecb72db080045c0003d2e874001f355ac12ac110303553f4521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 resubmit\(,0\) 
'e64c473528c9c6f94ecb72db080045c0003d2e874001f355ac12ac110303553f4521317040004011b138ac11ac12a28e15b3000d20966369616f0a'])
 
 dnl Check this output. We only see the latter two packets, not the first.
 AT_CHECK([cat ofctl_monitor.log], [0], [dnl
-NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=47 ct_state=new|trk,in_port=1 
(via action) data_len=47 (unbuffered)
+NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=75 
ct_state=inv|trk,in_port=2 (via action) data_len=75 (unbuffered)
+icmp,vlan_tci=0x,dl_src=c6:f5:4e:cb:72:db,dl_dst=f6:4c:47:35:28:c9,nw_src=172.16.0.4,nw_dst=172.16.0.3,nw_tos=192,nw_ecn=0,nw_ttl=64,icmp_type=3,icmp_code=3
 icmp_csum:da49
+NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=47 
ct_state=new|trk,in_port=1 (via action) data_len=47 (unbuffered)
 
udp,vlan_tci=0x,dl_src=e6:4c:47:35:28:c9,dl_dst=c6:f9:4e:cb:72:db,nw_src=172.16.0.1,nw_dst=172.16.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=41614,tp_dst=
 udp_csum:2096
-NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=75 
ct_state=rel|rpl|trk,in_port=2 (via action) data_len=75 (unbuffered)
+NXT_PACKET_IN2 (xid=0x0): table_id=1 cookie=0x0 total_len=75 
ct_state=rel|rpl|trk,in_port=2 (via action) data_len=75 (unbuffered)