[ovs-dev] [PATCH v3 ovn 1/4] ovn-northd: Drop IP packets destined to router owned IPs (after NAT).
Numan Siddique
numans at ovn.org
Tue Sep 22 06:17:28 UTC 2020
On Mon, Sep 21, 2020 at 12:08 PM Han Zhou <hzhou at ovn.org> wrote:
> On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara at 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 SNAT IPs
> in
> > table lr_in_arp_resolve.
> >
> > Reported-by: Tim Rozet <trozet at redhat.com>
> > Reported-at: https://bugzilla.redhat.com/1876174
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > ---
> > northd/ovn-northd.8.xml | 24 ++++++
> > northd/ovn-northd.c | 194
> +++++++++++++++++++++++++++--------------------
> > tests/ovn.at | 88 +++++++++++++++++++++
> > 3 files changed, 225 insertions(+), 81 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index bd42105..f1c7c9b 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -3089,6 +3089,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
> which are
> > + also SNAT IPs. This flow 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 IPv6 addresses
> which are
> > + also SNAT IPs. This flow 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 cfec6a2..d5d7631 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -623,6 +623,9 @@ struct ovn_datapath {
> > /* NAT entries configured on the router. */
> > struct ovn_nat *nat_entries;
> >
> > + /* SNAT IPs used by the router. */
> > + struct sset snat_ips;
> > +
> > struct ovn_port **localnet_ports;
> > size_t n_localnet_ports;
> >
> > @@ -641,6 +644,10 @@ struct ovn_nat {
> > struct lport_addresses ext_addrs;
> > };
> >
> > +static bool
> > +get_force_snat_ip(struct ovn_datapath *od, const char *key_type,
> > + struct lport_addresses *laddrs);
> > +
> > /* Returns true if a 'nat_entry' is valid, i.e.:
> > * - parsing was successful.
> > * - the string yielded exactly one IPv4 address or exactly one IPv6
> address.
> > @@ -663,7 +670,35 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry)
> > static void
> > init_nat_entries(struct ovn_datapath *od)
> > {
> > - if (!od->nbr || od->nbr->n_nat == 0) {
> > + struct lport_addresses snat_addrs;
> > +
> > + if (!od->nbr) {
> > + return;
> > + }
> > +
> > + sset_init(&od->snat_ips);
> > + if (get_force_snat_ip(od, "dnat", &snat_addrs)) {
> > + if (snat_addrs.n_ipv4_addrs) {
> > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> > + }
> > + if (snat_addrs.n_ipv6_addrs) {
> > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> > + }
> > + destroy_lport_addresses(&snat_addrs);
> > + }
> > +
> > + memset(&snat_addrs, 0, sizeof(snat_addrs));
> > + if (get_force_snat_ip(od, "lb", &snat_addrs)) {
> > + if (snat_addrs.n_ipv4_addrs) {
> > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> > + }
> > + if (snat_addrs.n_ipv6_addrs) {
> > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> > + }
> > + destroy_lport_addresses(&snat_addrs);
> > + }
> > +
> > + if (!od->nbr->n_nat) {
> > return;
> > }
> >
> > @@ -682,6 +717,13 @@ init_nat_entries(struct ovn_datapath *od)
> > VLOG_WARN_RL(&rl,
> > "Bad ip address %s in nat configuration "
> > "for router %s", nat->external_ip,
> od->nbr->name);
> > + continue;
> > + }
> > +
> > + if (!nat_entry_is_v6(nat_entry)) {
> > + sset_add(&od->snat_ips,
> nat_entry->ext_addrs.ipv4_addrs[0].addr_s);
> > + } else {
> > + sset_add(&od->snat_ips,
> nat_entry->ext_addrs.ipv6_addrs[0].addr_s);
> > }
> > }
> > }
> > @@ -693,6 +735,7 @@ destroy_nat_entries(struct ovn_datapath *od)
> > return;
> > }
> >
> > + sset_destroy(&od->snat_ips);
> > for (size_t i = 0; i < od->nbr->n_nat; i++) {
> > destroy_lport_addresses(&od->nat_entries[i].ext_addrs);
> > }
> > @@ -8744,6 +8787,68 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> > op, lflows, &match, &actions);
> > }
> >
> > + /* 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) {
> > + ds_clear(&match);
> > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > + if (!sset_find(&op->od->snat_ips,
> > + op->lrp_networks.ipv4_addrs[i].addr_s)) {
> > + continue;
> > + }
> > + ds_put_format(&match, "%s, ",
> > + op->lrp_networks.ipv4_addrs[i].addr_s);
> > + }
> > +
> > + if (ds_last(&match) != EOF) {
> > + ds_chomp(&match, ' ');
> > + ds_chomp(&match, ',');
> > +
> > + char *drop_match = xasprintf("ip4.dst == {%s}",
> > + ds_cstr(&match));
> > + /* Drop traffic with IP.dest == router-ip. */
> > + ovn_lflow_add_with_hint(lflows, op->od,
> > + S_ROUTER_IN_ARP_RESOLVE, 1,
> > + drop_match, "drop;",
> > + &op->nbrp->header_);
> > + free(drop_match);
> > + }
> > + }
> > +
> > + if (op->lrp_networks.n_ipv6_addrs) {
> > + ds_clear(&match);
> > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > + if (!sset_find(&op->od->snat_ips,
> > + op->lrp_networks.ipv6_addrs[i].addr_s)) {
> > + continue;
> > + }
> > + ds_put_format(&match, "%s, ",
> > + op->lrp_networks.ipv6_addrs[i].addr_s);
> > + }
> > +
> > + if (ds_last(&match) != EOF) {
> > + ds_chomp(&match, ' ');
> > + ds_chomp(&match, ',');
> > +
> > + char *drop_match = xasprintf("ip6.dst == {%s}",
> > + ds_cstr(&match));
> > + /* Drop traffic with IP.dest == router-ip. */
> > + ovn_lflow_add_with_hint(lflows, op->od,
> > + S_ROUTER_IN_ARP_RESOLVE, 1,
> > + drop_match, "drop;",
> > + &op->nbrp->header_);
> > + free(drop_match);
> > + }
> > + }
> > + }
> > +
> > HMAP_FOR_EACH (od, key_node, datapaths) {
> > if (!od->nbr) {
> > continue;
> > @@ -9035,77 +9140,15 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> > }
> >
> > /* A gateway router can have 4 SNAT IP addresses to force DNATed
> and
> > - * LBed traffic respectively to be SNATed. In addition, there
> can be
> > - * a number of SNAT rules in the NAT table. */
> > - struct v46_ip *snat_ips = xmalloc(sizeof *snat_ips
> > - * (op->od->nbr->n_nat + 4));
> > - size_t n_snat_ips = 0;
> > - struct lport_addresses snat_addrs;
> > -
> > - if (get_force_snat_ip(op->od, "dnat", &snat_addrs)) {
> > - if (snat_addrs.n_ipv4_addrs) {
> > - snat_ips[n_snat_ips].family = AF_INET;
> > - snat_ips[n_snat_ips++].ipv4 =
> snat_addrs.ipv4_addrs[0].addr;
> > - }
> > - if (snat_addrs.n_ipv6_addrs) {
> > - snat_ips[n_snat_ips].family = AF_INET6;
> > - snat_ips[n_snat_ips++].ipv6 =
> snat_addrs.ipv6_addrs[0].addr;
> > - }
> > - destroy_lport_addresses(&snat_addrs);
> > - }
> > -
> > - memset(&snat_addrs, 0, sizeof(snat_addrs));
> > - if (get_force_snat_ip(op->od, "lb", &snat_addrs)) {
> > - if (snat_addrs.n_ipv4_addrs) {
> > - snat_ips[n_snat_ips].family = AF_INET;
> > - snat_ips[n_snat_ips++].ipv4 =
> snat_addrs.ipv4_addrs[0].addr;
> > - }
> > - if (snat_addrs.n_ipv6_addrs) {
> > - snat_ips[n_snat_ips].family = AF_INET6;
> > - snat_ips[n_snat_ips++].ipv6 =
> snat_addrs.ipv6_addrs[0].addr;
> > - }
> > - destroy_lport_addresses(&snat_addrs);
> > - }
> > -
> > - for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> > - struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> > - const struct nbrec_nat *nat = nat_entry->nb;
> > -
> > - /* Skip entries we failed to parse. */
> > - if (!nat_entry_is_valid(nat_entry)) {
> > - continue;
> > - }
> > -
> > - if (!strcmp(nat->type, "snat")) {
> > - if (nat_entry_is_v6(nat_entry)) {
> > - struct in6_addr *ipv6 =
> > - &nat_entry->ext_addrs.ipv6_addrs[0].addr;
> > -
> > - snat_ips[n_snat_ips].family = AF_INET6;
> > - snat_ips[n_snat_ips++].ipv6 = *ipv6;
> > - } else {
> > - ovs_be32 ip =
> nat_entry->ext_addrs.ipv4_addrs[0].addr;
> > - snat_ips[n_snat_ips].family = AF_INET;
> > - snat_ips[n_snat_ips++].ipv4 = ip;
> > - }
> > - }
> > - }
> > -
> > + * LBed traffic respectively to be SNATed. In addition, there
> can be
> > + * a number of SNAT rules in the NAT table.
> > + * Skip all of them for drop flows. */
> > ds_clear(&match);
> > ds_put_cstr(&match, "ip4.dst == {");
> > bool has_drop_ips = false;
> > for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> > - bool snat_ip_is_router_ip = false;
> > - for (int j = 0; j < n_snat_ips; j++) {
> > - /* Packets to SNAT IPs should not be dropped. */
> > - if (snat_ips[j].family == AF_INET
> > - && op->lrp_networks.ipv4_addrs[i].addr
> > - == snat_ips[j].ipv4) {
> > - snat_ip_is_router_ip = true;
> > - break;
> > - }
> > - }
> > - if (snat_ip_is_router_ip) {
> > + if (sset_find(&op->od->snat_ips,
> > + op->lrp_networks.ipv4_addrs[i].addr_s)) {
> > continue;
> > }
> > ds_put_format(&match, "%s, ",
> > @@ -9122,17 +9165,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> > }
> >
> > for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> > - bool snat_ip_is_router_ip = false;
> > - for (int j = 0; j < n_snat_ips; j++) {
> > - /* Packets to SNAT IPs should not be dropped. */
> > - if (snat_ips[j].family == AF_INET6
> > - && !memcmp(&op->lrp_networks.ipv6_addrs[i].addr,
> > - &snat_ips[j].ipv6, sizeof
> snat_ips[j].ipv6)) {
> > - snat_ip_is_router_ip = true;
> > - break;
> > - }
> > - }
> > - if (snat_ip_is_router_ip) {
> > + if (sset_find(&op->od->snat_ips,
> > + op->lrp_networks.ipv6_addrs[i].addr_s)) {
> > continue;
> > }
> > ds_put_format(&match, "%s, ",
> > @@ -9151,8 +9185,6 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> > &op->nbrp->header_);
> > }
> >
> > - free(snat_ips);
> > -
> > /* ARP/NS packets are taken care of per router. The only
> exception
> > * is on the l3dgw_port where we might need to use a different
> > * ETH address.
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index a6f1fb5..cb7e7cc 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -21659,6 +21659,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], [1], [])
> > +
> > +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
> > +
> > AT_SETUP([ovn -- nb_cfg timestamp])
> > ovn_start
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Acked-by: Han Zhou <hzhou at ovn.org>
>
Thanks Dumitru and Han. I applied this patch to master and branch-20.09.
The patch doesn't apply cleanly to branch-20.06. There were many conflicts
and I didn't try resolving myself.
Dumitru - please submit the backport patches to branch-20.06 (and
branch-20.03 if you would like).
Thanks
Numan
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list