[ovs-dev] [PATCH ovn v2] ovn-northd: Drop IP packets destined to router owned IPs (after NAT).

Numan Siddique numans at ovn.org
Tue Sep 8 10:58:41 UTC 2020


On Tue, Sep 8, 2020 at 2:13 PM 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 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>



Hi Dumitru,

Thanks for the fix.

I have few comments.

Suppose we have the below lr and NAT entries

router 392b19fe-adf9-4b3d-bf43-040d12240e54 (lr0)
    port lr0-sw0
        mac: "00:00:00:00:ff:01"
        networks: ["10.0.0.1/24"]
    port lr0-sw1
        mac: "00:00:00:00:ff:02"
        networks: ["20.0.0.1/24"]
    port lr0-public
        mac: "00:00:20:20:12:13"
        networks: ["172.168.0.100/24"]
        gateway chassis: [chassis-1]
    nat 4c26f3f0-0ae0-4c18-9a1e-9d2751389270
        external ip: "172.168.0.110"
        logical ip: "10.0.0.3"
        type: "dnat_and_snat"
    nat b9296dc8-ac3a-427a-a97a-a94bf16214b2
        external ip: "172.168.0.120"
        logical ip: "20.0.0.3"
        type: "dnat_and_snat"
    nat da28129c-4e81-4e20-865a-768bd88e5a26
        external ip: "172.168.0.100"
        logical ip: "10.0.0.0/24"
        type: "snat"

I see the below lflows added in lr_in_ip_input to drop the pkts for
router port IPs

table=3 (lr_in_ip_input     ), priority=60   , match=(ip4.dst ==
{10.0.0.1} || ip6.dst == {fe80::200:ff:fe00:ff01}), action=(drop;)
table=3 (lr_in_ip_input     ), priority=60   , match=(ip4.dst ==
{20.0.0.1} || ip6.dst == {fe80::200:ff:fe00:ff02}), action=(drop;)

So I think there is no need to add the lflows in lr_in_arp_resolve to
drop for 10.0.0.1 and 20.0.0.1 again.

I think it's sufficient to add lflows in lr_in_arp_resolve to drop the
packet only for packets destined to the router ips which have NAT
entries.

Thanks
Numan

>
> ---
> V2:
> - rebased changes.
> ---
>  northd/ovn-northd.8.xml | 24 ++++++++++++
>  northd/ovn-northd.c     | 48 ++++++++++++++++++++++++
>  tests/ovn.at            | 98 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 168 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 681c008..6e063ac 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -10588,6 +10588,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) {
> +            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) {
> +            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 1fe34b7..0e14dfa 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" "$@"
> @@ -21447,6 +21451,96 @@ 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
> +
>  AT_SETUP([ovn -- nb_cfg timestamp])
>  ovn_start
>
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list