[ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack

Ben Pfaff blp at ovn.org
Wed Mar 8 21:32:28 UTC 2017


Hi Darrell and Daniele, do one of you have an opinion on whether this is
the right approach?

Thanks,

Ben.

On Mon, Feb 27, 2017 at 11:29:14AM +0530, nusiddiq at redhat.com wrote:
> From: Numan Siddique <nusiddiq at redhat.com>
> 
> Presently, the icmp4 requests to the router gateway ip are sent to the
> connectiont tracker, but the icmp4 reply packets responded by
> 'lr_in_ip_input' stage are not sent to the connection tracker.
> Also no zone ids are assigned for the router ports. Because of which
> the icmp4 request packets in the connection tracker will be in the
> UNREPLIED state. If the CMS has added ACLs to drop packets which
> are not in ESTABLISHED state, the icmp4 replies will be dropped.
> 
> To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl'
> stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
> to the router gateway ips.
> 
> Alternate solution would be to assign zone ids for the router ports
> and send the packets from the router ports to the connection tracker.
> 
> The approach used in this patch seems to be simpler.
> 
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> ---
>  ovn/northd/ovn-northd.8.xml | 29 +++++++++++---
>  ovn/northd/ovn-northd.c     | 92 +++++++++++++++++++++++++++------------------
>  2 files changed, 79 insertions(+), 42 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index ab8fd88..06465ff 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -245,11 +245,30 @@
>      <p>
>        This table prepares flows for possible stateful ACL processing in
>        ingress table <code>ACLs</code>.  It contains a priority-0 flow that
> -      simply moves traffic to the next table.  If stateful ACLs are used in the
> -      logical datapath, a priority-100 flow is added that sets a hint
> -      (with <code>reg0[0] = 1; next;</code>) for table
> -      <code>Pre-stateful</code> to send IP packets to the connection tracker
> -      before eventually advancing to ingress table <code>ACLs</code>.
> +      simply moves traffic to the next table. It adds the following flows if
> +      stateful ACLs are used in the logical datapath.
> +
> +      <ul>
> +        <li>
> +          A priority-100 flow is added that sets a hint
> +          (with <code>reg0[0] = 1; next;</code>) for table
> +          <code>Pre-stateful</code> to send IP packets to the connection tracker
> +          before eventually advancing to ingress table <code>ACLs</code>.
> +        </li>
> +
> +        <li>
> +          A priority-110 flow which doesn't set the connection tracking hint for
> +          the packets from the router ports.
> +        </li>
> +
> +        <li>
> +          A priority-110 flow which doesn't set the connection tracking hint
> +          for the packets with the match
> +          <code>ip4 &amp;&amp; icmp4 &amp;&amp; ip4.dst = {<var>R</var>}</code>
> +          where <var>R</var> is the IPv4 address(es) of the router ports
> +          connected to the logical datapath.
> +        </li>
> +      </ul>
>      </p>
>  
>      <h3>Ingress Table 4: Pre-LB</h3>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 03dc850..4fc86f4 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2339,6 +2339,43 @@ has_stateful_acl(struct ovn_datapath *od)
>  }
>  
>  static void
> +op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool add_bcast)
> +{
> +    if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
> +        ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s);
> +        return;
> +    }
> +
> +    ds_put_cstr(ds, "{");
> +    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> +        ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
> +        if (add_bcast) {
> +            ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].bcast_s);
> +        }
> +    }
> +    ds_chomp(ds, ' ');
> +    ds_chomp(ds, ',');
> +    ds_put_cstr(ds, "}");
> +}
> +
> +static void
> +op_put_v6_networks(struct ds *ds, const struct ovn_port *op)
> +{
> +    if (op->lrp_networks.n_ipv6_addrs == 1) {
> +        ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s);
> +        return;
> +    }
> +
> +    ds_put_cstr(ds, "{");
> +    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> +        ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s);
> +    }
> +    ds_chomp(ds, ' ');
> +    ds_chomp(ds, ',');
> +    ds_put_cstr(ds, "}");
> +}
> +
> +static void
>  build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>  {
>      bool has_stateful = has_stateful_acl(od);
> @@ -2378,6 +2415,24 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>  
>              ds_destroy(&match_in);
>              ds_destroy(&match_out);
> +
> +            /* The icmp4 request to the router ip (eg. ip4.dst = 10.0.0.1) will
> +             * be sent to the CT, but the reply from the router ip is not sent
> +             * to the CT. Also the zone id's are not assigned for the router
> +             * ports. Because of which the icmp request packet in the CT will
> +             * be in the UNREPLIED state.
> +             * The icmp4 reply packets could be dropped if the CMS has added
> +             * ACLs to drop packets which are not in ct.est state.
> +             * So, don't send the icpmp4 request packet for the router ips
> +             * to the CT.
> +             */
> +            if (op->peer && op->peer->lrp_networks.n_ipv4_addrs) {
> +                struct ds match = DS_EMPTY_INITIALIZER;
> +                ds_put_format(&match, "ip4 && icmp4 && ip4.dst == ");
> +                op_put_v4_networks(&match, op->peer, false);
> +                ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110,
> +                                          ds_cstr(&match), "next;");
> +            }
>          }
>          /* Ingress and Egress Pre-ACL Table (Priority 110).
>           *
> @@ -3644,43 +3699,6 @@ free_prefix_s:
>      free(prefix_s);
>  }
>  
> -static void
> -op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool add_bcast)
> -{
> -    if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
> -        ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s);
> -        return;
> -    }
> -
> -    ds_put_cstr(ds, "{");
> -    for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> -        ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
> -        if (add_bcast) {
> -            ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].bcast_s);
> -        }
> -    }
> -    ds_chomp(ds, ' ');
> -    ds_chomp(ds, ',');
> -    ds_put_cstr(ds, "}");
> -}
> -
> -static void
> -op_put_v6_networks(struct ds *ds, const struct ovn_port *op)
> -{
> -    if (op->lrp_networks.n_ipv6_addrs == 1) {
> -        ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s);
> -        return;
> -    }
> -
> -    ds_put_cstr(ds, "{");
> -    for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> -        ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s);
> -    }
> -    ds_chomp(ds, ' ');
> -    ds_chomp(ds, ',');
> -    ds_put_cstr(ds, "}");
> -}
> -
>  static const char *
>  get_force_snat_ip(struct ovn_datapath *od, const char *key_type, ovs_be32 *ip)
>  {
> -- 
> 2.9.3
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list