| Message ID | 1599494618-27057-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ovn-northd: Drop IP packets destined to router owned IPs (after NAT). | expand |
On Mon, Sep 7, 2020 at 9:34 PM Dumitru Ceara <dceara@redhat.com> wrote: > OVN was dropping IP packets destined to IPs owned by logical routers but > only if those IPs are not used for SNAT rules. However, if a packet > doesn't match an existing NAT session and its destination is still a > router owned IP, it can be safely dropped. Otherwise it will trigger an > unnecessary packet-in in stage lr_in_arp_request. > > To achieve that we add flows that drop traffic to router owned IPs in > table lr_in_arp_resolve. > > Reported-by: Tim Rozet <trozet@redhat.com> > Reported-at: https://bugzilla.redhat.com/1876174 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > Hi Dumitru, This patch needs a rebase. Thanks Numan > --- > northd/ovn-northd.8.xml | 24 ++++++++++++ > northd/ovn-northd.c | 48 ++++++++++++++++++++++++ > tests/ovn.at | 99 > ++++++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 169 insertions(+), 2 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 989e364..b9170fc 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -2943,6 +2943,30 @@ outport = <var>P</var>; > > <li> > <p> > + Traffic with IP destination an address owned by the router > should be > + dropped. Such traffic is normally dropped in ingress table > + <code>IP Input</code> except for IPs that are also shared with > SNAT > + rules. However, if there was no unSNAT operation that happened > + successfully until this point in the pipeline and the > destination IP > + of the packet is still a router owned IP, the packets can be > safely > + dropped. > + </p> > + > + <p> > + A priority-1 logical flow with match <code>ip4.dst = {..}</code> > + matches on traffic destined to router owned IPv4 addresses and > + has action <code>drop;</code>. > + </p> > + > + <p> > + A priority-1 logical flow with match <code>ip6.dst = {..}</code> > + matches on traffic destined to router owned IPv4 addresses and > + has action <code>drop;</code>. > + </p> > + </li> > + > + <li> > + <p> > Dynamic MAC bindings. These flows resolve MAC-to-IP bindings > that have become known dynamically through ARP or neighbor > discovery. (The ingress table <code>ARP Request</code> will > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 3de7161..dcce846 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -10587,6 +10587,54 @@ build_lrouter_flows(struct hmap *datapaths, > struct hmap *ports, > } > } > > + /* Drop IP traffic destined to router owned IPs. Part of it is dropped > + * in stage "lr_in_ip_input" but traffic that could have been unSNATed > + * but didn't match any existing session might still end up here. > + */ > + HMAP_FOR_EACH (op, key_node, ports) { > + if (!op->nbrp) { > + continue; > + } > + > + if (op->lrp_networks.n_ipv4_addrs > 0) { > + ds_clear(&match); > + ds_put_cstr(&match, "ip4.dst == {"); > + > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + ds_put_format(&match, "%s, ", > + op->lrp_networks.ipv4_addrs[i].addr_s); > + } > + > + ds_chomp(&match, ' '); > + ds_chomp(&match, ','); > + ds_put_char(&match, '}'); > + > + /* Drop traffic with IP.dest == router-ip. */ > + ovn_lflow_add_with_hint(lflows, op->od, > S_ROUTER_IN_ARP_RESOLVE, 1, > + ds_cstr(&match), "drop;", > + &op->nbrp->header_); > + } > + > + if (op->lrp_networks.n_ipv6_addrs > 0) { > + ds_clear(&match); > + ds_put_cstr(&match, "ip6.dst == {"); > + > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > + ds_put_format(&match, "%s, ", > + op->lrp_networks.ipv6_addrs[i].addr_s); > + } > + > + ds_chomp(&match, ' '); > + ds_chomp(&match, ','); > + ds_put_char(&match, '}'); > + > + /* Drop traffic with IP.dest == router-ip. */ > + ovn_lflow_add_with_hint(lflows, op->od, > S_ROUTER_IN_ARP_RESOLVE, 1, > + ds_cstr(&match), "drop;", > + &op->nbrp->header_); > + } > + } > + > HMAP_FOR_EACH (od, key_node, datapaths) { > if (!od->nbr) { > continue; > diff --git a/tests/ovn.at b/tests/ovn.at > index 2ee3862..d946c1d 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -21377,8 +21377,12 @@ OVN_POPULATE_ARP > > ovn-nbctl --wait=hv sync > > -AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.1], > [1], []) > -AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.2], > [1], []) > +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.1], > [0], [dnl > + table=13(lr_in_arp_resolve ), priority=1 , match=(ip4.dst == > {10.0.0.1}), action=(drop;) > +]) > +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.2], > [0], [dnl > + table=13(lr_in_arp_resolve ), priority=1 , match=(ip4.dst == > {10.0.0.2}), action=(drop;) > +]) > > ip_to_hex() { > printf "%02x%02x%02x%02x" "$@" > @@ -21446,3 +21450,94 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t > ovn-controller debug/status) = "xru > > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +# Test dropping traffic destined to router owned IPs. > +AT_SETUP([ovn -- gateway router drop traffic for own IPs]) > +ovn_start > + > +ovn-nbctl lr-add r1 -- set logical_router r1 options:chassis=hv1 > +ovn-nbctl ls-add s1 > + > +# Connnect r1 to s1. > +ovn-nbctl lrp-add r1 lrp-r1-s1 00:00:00:00:01:01 10.0.1.1/24 > +ovn-nbctl lsp-add s1 lsp-s1-r1 -- set Logical_Switch_Port lsp-s1-r1 > type=router \ > + options:router-port=lrp-r1-s1 addresses=router > + > +# Create logical port p1 in s1 > +ovn-nbctl lsp-add s1 p1 \ > +-- lsp-set-addresses p1 "f0:00:00:00:01:02 10.0.1.2" > + > +# Create two hypervisor and create OVS ports corresponding to logical > ports. > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +# Pre-populate the hypervisors' ARP tables so that we don't lose any > +# packets for ARP resolution (native tunneling doesn't queue packets > +# for ARP resolution). > +OVN_POPULATE_ARP > + > +ovn-nbctl --wait=hv sync > + > +sw_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding r1) > + > +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.1.1], > [0], [dnl > + table=13(lr_in_arp_resolve ), priority=1 , match=(ip4.dst == > {10.0.1.1}), action=(drop;) > +]) > + > +ip_to_hex() { > + printf "%02x%02x%02x%02x" "$@" > +} > + > +# Send ip packets from p1 to lrp-r1-s1 > +src_mac="f00000000102" > +dst_mac="000000000101" > +src_ip=`ip_to_hex 10 0 1 2` > +dst_ip=`ip_to_hex 10 0 1 1` > > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > + > +# No packet-ins should reach ovn-controller. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" > | grep -v n_packets=0 -c], [1], [dnl > +0 > +]) > + > +# The packet should have been dropped in the lr_in_ip_input stage. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=11, > n_packets=1,.* priority=60,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 > actions=drop" -c], [0], [dnl > +1 > +]) > + > +# Use the router IP as SNAT IP. > +ovn-nbctl set logical_router r1 options:lb_force_snat_ip=10.0.1.1 > +ovn-nbctl --wait=hv sync > + > +# Send ip packets from p1 to lrp-r1-s1 > +src_mac="f00000000102" > +dst_mac="000000000101" > +src_ip=`ip_to_hex 10 0 1 2` > +dst_ip=`ip_to_hex 10 0 1 1` > > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > + > +# Even after configuring a router owned IP for SNAT, no packet-ins should > +# reach ovn-controller. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" > | grep -v n_packets=0 -c], [1], [dnl > +0 > +]) > + > +# The packet should've been dropped in the lr_in_arp_resolve stage. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=21, > n_packets=1,.* priority=1,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 > actions=drop" -c], [0], [dnl > +1 > +]) > + > +OVN_CLEANUP([hv1]) > + > +AT_CLEANUP > \ No newline at end of file > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 9/8/20 10:28 AM, Numan Siddique wrote: > > > On Mon, Sep 7, 2020 at 9:34 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > OVN was dropping IP packets destined to IPs owned by logical routers but > only if those IPs are not used for SNAT rules. However, if a packet > doesn't match an existing NAT session and its destination is still a > router owned IP, it can be safely dropped. Otherwise it will trigger an > unnecessary packet-in in stage lr_in_arp_request. > > To achieve that we add flows that drop traffic to router owned IPs in > table lr_in_arp_resolve. > > Reported-by: Tim Rozet <trozet@redhat.com <mailto:trozet@redhat.com>> > Reported-at: https://bugzilla.redhat.com/1876174 > Signed-off-by: Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> > > > Hi Dumitru, > > This patch needs a rebase. > > Thanks > Numan > > Hi Numan, I sent a v2: http://patchwork.ozlabs.org/project/ovn/patch/1599554583-1698-1-git-send-email-dceara@redhat.com/ Thanks, Dumitru
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 989e364..b9170fc 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -2943,6 +2943,30 @@ outport = <var>P</var>; <li> <p> + Traffic with IP destination an address owned by the router should be + dropped. Such traffic is normally dropped in ingress table + <code>IP Input</code> except for IPs that are also shared with SNAT + rules. However, if there was no unSNAT operation that happened + successfully until this point in the pipeline and the destination IP + of the packet is still a router owned IP, the packets can be safely + dropped. + </p> + + <p> + A priority-1 logical flow with match <code>ip4.dst = {..}</code> + matches on traffic destined to router owned IPv4 addresses and + has action <code>drop;</code>. + </p> + + <p> + A priority-1 logical flow with match <code>ip6.dst = {..}</code> + matches on traffic destined to router owned IPv4 addresses and + has action <code>drop;</code>. + </p> + </li> + + <li> + <p> Dynamic MAC bindings. These flows resolve MAC-to-IP bindings that have become known dynamically through ARP or neighbor discovery. (The ingress table <code>ARP Request</code> will diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 3de7161..dcce846 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -10587,6 +10587,54 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, } } + /* Drop IP traffic destined to router owned IPs. Part of it is dropped + * in stage "lr_in_ip_input" but traffic that could have been unSNATed + * but didn't match any existing session might still end up here. + */ + HMAP_FOR_EACH (op, key_node, ports) { + if (!op->nbrp) { + continue; + } + + if (op->lrp_networks.n_ipv4_addrs > 0) { + ds_clear(&match); + ds_put_cstr(&match, "ip4.dst == {"); + + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { + ds_put_format(&match, "%s, ", + op->lrp_networks.ipv4_addrs[i].addr_s); + } + + ds_chomp(&match, ' '); + ds_chomp(&match, ','); + ds_put_char(&match, '}'); + + /* Drop traffic with IP.dest == router-ip. */ + ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ARP_RESOLVE, 1, + ds_cstr(&match), "drop;", + &op->nbrp->header_); + } + + if (op->lrp_networks.n_ipv6_addrs > 0) { + ds_clear(&match); + ds_put_cstr(&match, "ip6.dst == {"); + + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { + ds_put_format(&match, "%s, ", + op->lrp_networks.ipv6_addrs[i].addr_s); + } + + ds_chomp(&match, ' '); + ds_chomp(&match, ','); + ds_put_char(&match, '}'); + + /* Drop traffic with IP.dest == router-ip. */ + ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_ARP_RESOLVE, 1, + ds_cstr(&match), "drop;", + &op->nbrp->header_); + } + } + HMAP_FOR_EACH (od, key_node, datapaths) { if (!od->nbr) { continue; diff --git a/tests/ovn.at b/tests/ovn.at index 2ee3862..d946c1d 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -21377,8 +21377,12 @@ OVN_POPULATE_ARP ovn-nbctl --wait=hv sync -AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.1], [1], []) -AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.2], [1], []) +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.1], [0], [dnl + table=13(lr_in_arp_resolve ), priority=1 , match=(ip4.dst == {10.0.0.1}), action=(drop;) +]) +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.0.2], [0], [dnl + table=13(lr_in_arp_resolve ), priority=1 , match=(ip4.dst == {10.0.0.2}), action=(drop;) +]) ip_to_hex() { printf "%02x%02x%02x%02x" "$@" @@ -21446,3 +21450,94 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xru OVN_CLEANUP([hv1]) AT_CLEANUP + +# Test dropping traffic destined to router owned IPs. +AT_SETUP([ovn -- gateway router drop traffic for own IPs]) +ovn_start + +ovn-nbctl lr-add r1 -- set logical_router r1 options:chassis=hv1 +ovn-nbctl ls-add s1 + +# Connnect r1 to s1. +ovn-nbctl lrp-add r1 lrp-r1-s1 00:00:00:00:01:01 10.0.1.1/24 +ovn-nbctl lsp-add s1 lsp-s1-r1 -- set Logical_Switch_Port lsp-s1-r1 type=router \ + options:router-port=lrp-r1-s1 addresses=router + +# Create logical port p1 in s1 +ovn-nbctl lsp-add s1 p1 \ +-- lsp-set-addresses p1 "f0:00:00:00:01:02 10.0.1.2" + +# Create two hypervisor and create OVS ports corresponding to logical ports. +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +# Pre-populate the hypervisors' ARP tables so that we don't lose any +# packets for ARP resolution (native tunneling doesn't queue packets +# for ARP resolution). +OVN_POPULATE_ARP + +ovn-nbctl --wait=hv sync + +sw_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding r1) + +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.1.1], [0], [dnl + table=13(lr_in_arp_resolve ), priority=1 , match=(ip4.dst == {10.0.1.1}), action=(drop;) +]) + +ip_to_hex() { + printf "%02x%02x%02x%02x" "$@" +} + +# Send ip packets from p1 to lrp-r1-s1 +src_mac="f00000000102" +dst_mac="000000000101" +src_ip=`ip_to_hex 10 0 1 2` +dst_ip=`ip_to_hex 10 0 1 1` +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet + +# No packet-ins should reach ovn-controller. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" | grep -v n_packets=0 -c], [1], [dnl +0 +]) + +# The packet should have been dropped in the lr_in_ip_input stage. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=11, n_packets=1,.* priority=60,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 actions=drop" -c], [0], [dnl +1 +]) + +# Use the router IP as SNAT IP. +ovn-nbctl set logical_router r1 options:lb_force_snat_ip=10.0.1.1 +ovn-nbctl --wait=hv sync + +# Send ip packets from p1 to lrp-r1-s1 +src_mac="f00000000102" +dst_mac="000000000101" +src_ip=`ip_to_hex 10 0 1 2` +dst_ip=`ip_to_hex 10 0 1 1` +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet + +# Even after configuring a router owned IP for SNAT, no packet-ins should +# reach ovn-controller. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" | grep -v n_packets=0 -c], [1], [dnl +0 +]) + +# The packet should've been dropped in the lr_in_arp_resolve stage. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=21, n_packets=1,.* priority=1,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 actions=drop" -c], [0], [dnl +1 +]) + +OVN_CLEANUP([hv1]) + +AT_CLEANUP \ No newline at end of file
OVN was dropping IP packets destined to IPs owned by logical routers but only if those IPs are not used for SNAT rules. However, if a packet doesn't match an existing NAT session and its destination is still a router owned IP, it can be safely dropped. Otherwise it will trigger an unnecessary packet-in in stage lr_in_arp_request. To achieve that we add flows that drop traffic to router owned IPs in table lr_in_arp_resolve. Reported-by: Tim Rozet <trozet@redhat.com> Reported-at: https://bugzilla.redhat.com/1876174 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.8.xml | 24 ++++++++++++ northd/ovn-northd.c | 48 ++++++++++++++++++++++++ tests/ovn.at | 99 ++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 169 insertions(+), 2 deletions(-)