Skip to content

Commit

Permalink
ovn-northd: Drop IP packets destined to router owned IPs (after NAT).
Browse files Browse the repository at this point in the history 
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@redhat.com>
Reported-at: https://bugzilla.redhat.com/1876174
Acked-by: Han Zhou <hzhou@ovn.org>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
  • Loading branch information
@dceara @numansiddique
dceara authored and numansiddique committed Sep 22, 2020
1 parent d62d09a commit 802f927
Show file tree
Hide file tree
Showing 3 changed files with 225 additions and 81 deletions.
24 changes: 24 additions & 0 deletions northd/ovn-northd.8.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3087,6 +3087,30 @@ outport = <var>P</var>;
</p>
</li>

<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
Expand Down
194 changes: 113 additions & 81 deletions northd/ovn-northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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.
Expand All @@ -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;
}

Expand All @@ -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);
}
}
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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, ",
Expand All @@ -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, ",
Expand All @@ -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.
Expand Down
88 changes: 88 additions & 0 deletions tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21690,6 +21690,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

Expand Down

0 comments on commit 802f927

Please sign in to comment.