[ovs-dev] [PATCH] ovn-northd: Skip icmp4 packets destined for router ports from conntrack
Russell Bryant
russell at ovn.org
Wed Mar 8 21:36:57 UTC 2017
I'm also looking at this one. I was trying to review today, but have
been slowed down by getting an OpenStack test environment working for
testing this and looking closer.
On Wed, Mar 8, 2017 at 4:32 PM, Ben Pfaff <blp at ovn.org> wrote:
> 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 && icmp4 && 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
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
Russell Bryant
More information about the dev
mailing list