[ovs-dev] physical: do not forward traffic from localport to a localnet one

Since the localnet port is available on each hv, do not forward traffic
to the localnet port if it is present in order to avoid switch fdb
misconfiguration.
Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1942877

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 controller/physical.c        | 23 +++++++++++++++++++++++
 include/ovn/logical-fields.h |  4 ++++
 tests/ovn.at                 | 17 +++++++++++++++++
 3 files changed, 44 insertions(+)

Acked-by: Mark Michelson

I have a small nit below, but since it's strictly with a comment, it's 
easy for whoever merges this to correct the comment when they do it. 
There's no reason to spin up a new version of this.

On 5/4/21 1:59 PM, Lorenzo Bianconi wrote:
> Since the localnet port is available on each hv, do not forward traffic
> to the localnet port if it is present in order to avoid switch fdb
> misconfiguration.
> Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1942877
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   controller/physical.c        | 23 +++++++++++++++++++++++
>   include/ovn/logical-fields.h |  4 ++++
>   tests/ovn.at                 | 17 +++++++++++++++++
>   3 files changed, 44 insertions(+)
> 
> diff --git a/controller/physical.c b/controller/physical.c
> index 96c959d18..258842634 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1193,6 +1193,11 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>   
>           load_logical_ingress_metadata(binding, &zone_ids, ofpacts_p);
>   
> +        if (!strcmp(binding->type, "localport")) {
> +            /* mark the packet as incoming from a localport */
> +            put_load(1, MFF_LOG_FLAGS, MLF_LOCALPORT_BIT, 1, ofpacts_p);
> +        }
> +
>           /* Resubmit to first logical ingress pipeline table. */
>           put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
>           ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG,
> @@ -1251,6 +1256,24 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>                                                 ofport, flow_table);
>           }
>   
> +        /* Table 34, priority 160.

Nit: Table 34 is incorrect here. OFTABLE_CHECK_LOOPBACK is table 39 now. 
I think the issue here is that when the tables shifted, nobody updated 
the comments in this file, and so it's wrong here and several other 
places in physical.c

> +         * =======================
> +         *
> +         * Do not forward local traffic from a localport to a localnet port.
> +         */
> +        if (!strcmp(binding->type, "localnet")) {
> +            /* do not forward traffic from localport to localnet port */
> +            match_init_catchall(&match);
> +            ofpbuf_clear(ofpacts_p);
> +            match_set_metadata(&match, htonll(dp_key));
> +            match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
> +            match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
> +                                 MLF_LOCALPORT, MLF_LOCALPORT);
> +            ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160,
> +                            binding->header_.uuid.parts[0], &match,
> +                            ofpacts_p, &binding->header_.uuid);
> +        }
> +
>       } else if (!tun && !is_ha_remote) {
>           /* Remote port connected by localnet port */
>           /* Table 33, priority 100.
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index d44b30b30..ef97117b9 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -67,6 +67,7 @@ enum mff_log_flags_bits {
>       MLF_LOOKUP_LB_HAIRPIN_BIT = 7,
>       MLF_LOOKUP_FDB_BIT = 8,
>       MLF_SKIP_SNAT_FOR_LB_BIT = 9,
> +    MLF_LOCALPORT_BIT = 10,
>   };
>   
>   /* MFF_LOG_FLAGS_REG flag assignments */
> @@ -107,6 +108,9 @@ enum mff_log_flags {
>       /* Indicate that a packet must not SNAT in the gateway router when
>        * load-balancing has taken place. */
>       MLF_SKIP_SNAT_FOR_LB = (1 << MLF_SKIP_SNAT_FOR_LB_BIT),
> +
> +    /* Indicate the packet has been received from a localport */
> +    MLF_LOCALPORT = (1 << MLF_LOCALPORT_BIT),
>   };
>   
>   /* OVN logical fields
> diff --git a/tests/ovn.at b/tests/ovn.at
> index fe6a7c85b..99764b24b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -11823,10 +11823,17 @@ OVN_FOR_EACH_NORTHD([
>   AT_SETUP([ovn -- localport suppress gARP])
>   ovn_start
>   
> +send_garp() {
> +    local inport=$1 eth_src=$2 eth_dst=$3 spa=$4 tpa=$5
> +    local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
> +    as hv1 ovs-appctl netdev-dummy/receive vif$inport $request
> +}
> +
>   net_add n1
>   sim_add hv1
>   as hv1
>   check ovs-vsctl add-br br-phys
> +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
>   ovn_attach n1 br-phys 192.168.0.1
>   
>   check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> @@ -11837,6 +11844,7 @@ check ovn-nbctl ls-add ls \
>       -- lsp-set-addresses lp "00:00:00:00:00:01 10.0.0.1" \
>       -- lsp-add ls ln \
>       -- lsp-set-type ln localnet \
> +    -- lsp-set-addresses ln unknown \
>       -- lsp-set-options ln network_name=phys \
>       -- lsp-add ls lsp \
>       -- lsp-set-addresses lsp "00:00:00:00:00:02 10.0.0.2"
> @@ -11870,6 +11878,15 @@ AT_CHECK([
>       test 0 -eq $pkts
>   ])
>   
> +spa=$(ip_to_hex 10 0 0 1)
> +tpa=$(ip_to_hex 10 0 0 100)
> +send_garp 1 000000000001 ffffffffffff $spa $tpa
> +
> +dnl traffic from localport should not be sent to localnet
> +AT_CHECK([tcpdump -r hv1/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000064 | wc -l],[0],[dnl
> +0
> +],[ignore])
> +
>   OVN_CLEANUP([hv1])
>   AT_CLEANUP
>   ])
>

On Thu, May 20, 2021 at 5:05 PM Mark Michelson <mmichels@redhat.com> wrote:
>
> Acked-by: Mark Michelson
>
> I have a small nit below, but since it's strictly with a comment, it's
> easy for whoever merges this to correct the comment when they do it.
> There's no reason to spin up a new version of this.
>
> On 5/4/21 1:59 PM, Lorenzo Bianconi wrote:
> > Since the localnet port is available on each hv, do not forward traffic
> > to the localnet port if it is present in order to avoid switch fdb
> > misconfiguration.
> > Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1942877
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> >   controller/physical.c        | 23 +++++++++++++++++++++++
> >   include/ovn/logical-fields.h |  4 ++++
> >   tests/ovn.at                 | 17 +++++++++++++++++
> >   3 files changed, 44 insertions(+)
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 96c959d18..258842634 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -1193,6 +1193,11 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >
> >           load_logical_ingress_metadata(binding, &zone_ids, ofpacts_p);
> >
> > +        if (!strcmp(binding->type, "localport")) {
> > +            /* mark the packet as incoming from a localport */
> > +            put_load(1, MFF_LOG_FLAGS, MLF_LOCALPORT_BIT, 1, ofpacts_p);
> > +        }
> > +
> >           /* Resubmit to first logical ingress pipeline table. */
> >           put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
> >           ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG,
> > @@ -1251,6 +1256,24 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >                                                 ofport, flow_table);
> >           }
> >
> > +        /* Table 34, priority 160.
>
> Nit: Table 34 is incorrect here. OFTABLE_CHECK_LOOPBACK is table 39 now.
> I think the issue here is that when the tables shifted, nobody updated
> the comments in this file, and so it's wrong here and several other
> places in physical.c
>

I corrected the comment and applied the patch to the main branch.

Numan

> > +         * =======================
> > +         *
> > +         * Do not forward local traffic from a localport to a localnet port.
> > +         */
> > +        if (!strcmp(binding->type, "localnet")) {
> > +            /* do not forward traffic from localport to localnet port */
> > +            match_init_catchall(&match);
> > +            ofpbuf_clear(ofpacts_p);
> > +            match_set_metadata(&match, htonll(dp_key));
> > +            match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
> > +            match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
> > +                                 MLF_LOCALPORT, MLF_LOCALPORT);
> > +            ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160,
> > +                            binding->header_.uuid.parts[0], &match,
> > +                            ofpacts_p, &binding->header_.uuid);
> > +        }
> > +
> >       } else if (!tun && !is_ha_remote) {
> >           /* Remote port connected by localnet port */
> >           /* Table 33, priority 100.
> > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> > index d44b30b30..ef97117b9 100644
> > --- a/include/ovn/logical-fields.h
> > +++ b/include/ovn/logical-fields.h
> > @@ -67,6 +67,7 @@ enum mff_log_flags_bits {
> >       MLF_LOOKUP_LB_HAIRPIN_BIT = 7,
> >       MLF_LOOKUP_FDB_BIT = 8,
> >       MLF_SKIP_SNAT_FOR_LB_BIT = 9,
> > +    MLF_LOCALPORT_BIT = 10,
> >   };
> >
> >   /* MFF_LOG_FLAGS_REG flag assignments */
> > @@ -107,6 +108,9 @@ enum mff_log_flags {
> >       /* Indicate that a packet must not SNAT in the gateway router when
> >        * load-balancing has taken place. */
> >       MLF_SKIP_SNAT_FOR_LB = (1 << MLF_SKIP_SNAT_FOR_LB_BIT),
> > +
> > +    /* Indicate the packet has been received from a localport */
> > +    MLF_LOCALPORT = (1 << MLF_LOCALPORT_BIT),
> >   };
> >
> >   /* OVN logical fields
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index fe6a7c85b..99764b24b 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -11823,10 +11823,17 @@ OVN_FOR_EACH_NORTHD([
> >   AT_SETUP([ovn -- localport suppress gARP])
> >   ovn_start
> >
> > +send_garp() {
> > +    local inport=$1 eth_src=$2 eth_dst=$3 spa=$4 tpa=$5
> > +    local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
> > +    as hv1 ovs-appctl netdev-dummy/receive vif$inport $request
> > +}
> > +
> >   net_add n1
> >   sim_add hv1
> >   as hv1
> >   check ovs-vsctl add-br br-phys
> > +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> >   ovn_attach n1 br-phys 192.168.0.1
> >
> >   check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> > @@ -11837,6 +11844,7 @@ check ovn-nbctl ls-add ls \
> >       -- lsp-set-addresses lp "00:00:00:00:00:01 10.0.0.1" \
> >       -- lsp-add ls ln \
> >       -- lsp-set-type ln localnet \
> > +    -- lsp-set-addresses ln unknown \
> >       -- lsp-set-options ln network_name=phys \
> >       -- lsp-add ls lsp \
> >       -- lsp-set-addresses lsp "00:00:00:00:00:02 10.0.0.2"
> > @@ -11870,6 +11878,15 @@ AT_CHECK([
> >       test 0 -eq $pkts
> >   ])
> >
> > +spa=$(ip_to_hex 10 0 0 1)
> > +tpa=$(ip_to_hex 10 0 0 100)
> > +send_garp 1 000000000001 ffffffffffff $spa $tpa
> > +
> > +dnl traffic from localport should not be sent to localnet
> > +AT_CHECK([tcpdump -r hv1/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000064 | wc -l],[0],[dnl
> > +0
> > +],[ignore])
> > +
> >   OVN_CLEANUP([hv1])
> >   AT_CLEANUP
> >   ])
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

> On Thu, May 20, 2021 at 5:05 PM Mark Michelson <mmichels@redhat.com> wrote:
> >
> > Acked-by: Mark Michelson
> >
> > I have a small nit below, but since it's strictly with a comment, it's
> > easy for whoever merges this to correct the comment when they do it.
> > There's no reason to spin up a new version of this.

ack, thx Mark and Numan.

Regards,
Lorenzo

> >
> > On 5/4/21 1:59 PM, Lorenzo Bianconi wrote:
> > > Since the localnet port is available on each hv, do not forward traffic
> > > to the localnet port if it is present in order to avoid switch fdb
> > > misconfiguration.
> > > Related bz: https://bugzilla.redhat.com/show_bug.cgi?id=1942877
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > > ---
> > >   controller/physical.c        | 23 +++++++++++++++++++++++
> > >   include/ovn/logical-fields.h |  4 ++++
> > >   tests/ovn.at                 | 17 +++++++++++++++++
> > >   3 files changed, 44 insertions(+)
> > >
> > > diff --git a/controller/physical.c b/controller/physical.c
> > > index 96c959d18..258842634 100644
> > > --- a/controller/physical.c
> > > +++ b/controller/physical.c
> > > @@ -1193,6 +1193,11 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > >
> > >           load_logical_ingress_metadata(binding, &zone_ids, ofpacts_p);
> > >
> > > +        if (!strcmp(binding->type, "localport")) {
> > > +            /* mark the packet as incoming from a localport */
> > > +            put_load(1, MFF_LOG_FLAGS, MLF_LOCALPORT_BIT, 1, ofpacts_p);
> > > +        }
> > > +
> > >           /* Resubmit to first logical ingress pipeline table. */
> > >           put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
> > >           ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG,
> > > @@ -1251,6 +1256,24 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > >                                                 ofport, flow_table);
> > >           }
> > >
> > > +        /* Table 34, priority 160.
> >
> > Nit: Table 34 is incorrect here. OFTABLE_CHECK_LOOPBACK is table 39 now.
> > I think the issue here is that when the tables shifted, nobody updated
> > the comments in this file, and so it's wrong here and several other
> > places in physical.c
> >
> 
> I corrected the comment and applied the patch to the main branch.
> 
> Numan
> 
> > > +         * =======================
> > > +         *
> > > +         * Do not forward local traffic from a localport to a localnet port.
> > > +         */
> > > +        if (!strcmp(binding->type, "localnet")) {
> > > +            /* do not forward traffic from localport to localnet port */
> > > +            match_init_catchall(&match);
> > > +            ofpbuf_clear(ofpacts_p);
> > > +            match_set_metadata(&match, htonll(dp_key));
> > > +            match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
> > > +            match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0,
> > > +                                 MLF_LOCALPORT, MLF_LOCALPORT);
> > > +            ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 160,
> > > +                            binding->header_.uuid.parts[0], &match,
> > > +                            ofpacts_p, &binding->header_.uuid);
> > > +        }
> > > +
> > >       } else if (!tun && !is_ha_remote) {
> > >           /* Remote port connected by localnet port */
> > >           /* Table 33, priority 100.
> > > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> > > index d44b30b30..ef97117b9 100644
> > > --- a/include/ovn/logical-fields.h
> > > +++ b/include/ovn/logical-fields.h
> > > @@ -67,6 +67,7 @@ enum mff_log_flags_bits {
> > >       MLF_LOOKUP_LB_HAIRPIN_BIT = 7,
> > >       MLF_LOOKUP_FDB_BIT = 8,
> > >       MLF_SKIP_SNAT_FOR_LB_BIT = 9,
> > > +    MLF_LOCALPORT_BIT = 10,
> > >   };
> > >
> > >   /* MFF_LOG_FLAGS_REG flag assignments */
> > > @@ -107,6 +108,9 @@ enum mff_log_flags {
> > >       /* Indicate that a packet must not SNAT in the gateway router when
> > >        * load-balancing has taken place. */
> > >       MLF_SKIP_SNAT_FOR_LB = (1 << MLF_SKIP_SNAT_FOR_LB_BIT),
> > > +
> > > +    /* Indicate the packet has been received from a localport */
> > > +    MLF_LOCALPORT = (1 << MLF_LOCALPORT_BIT),
> > >   };
> > >
> > >   /* OVN logical fields
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index fe6a7c85b..99764b24b 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -11823,10 +11823,17 @@ OVN_FOR_EACH_NORTHD([
> > >   AT_SETUP([ovn -- localport suppress gARP])
> > >   ovn_start
> > >
> > > +send_garp() {
> > > +    local inport=$1 eth_src=$2 eth_dst=$3 spa=$4 tpa=$5
> > > +    local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
> > > +    as hv1 ovs-appctl netdev-dummy/receive vif$inport $request
> > > +}
> > > +
> > >   net_add n1
> > >   sim_add hv1
> > >   as hv1
> > >   check ovs-vsctl add-br br-phys
> > > +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> > >   ovn_attach n1 br-phys 192.168.0.1
> > >
> > >   check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> > > @@ -11837,6 +11844,7 @@ check ovn-nbctl ls-add ls \
> > >       -- lsp-set-addresses lp "00:00:00:00:00:01 10.0.0.1" \
> > >       -- lsp-add ls ln \
> > >       -- lsp-set-type ln localnet \
> > > +    -- lsp-set-addresses ln unknown \
> > >       -- lsp-set-options ln network_name=phys \
> > >       -- lsp-add ls lsp \
> > >       -- lsp-set-addresses lsp "00:00:00:00:00:02 10.0.0.2"
> > > @@ -11870,6 +11878,15 @@ AT_CHECK([
> > >       test 0 -eq $pkts
> > >   ])
> > >
> > > +spa=$(ip_to_hex 10 0 0 1)
> > > +tpa=$(ip_to_hex 10 0 0 100)
> > > +send_garp 1 000000000001 ffffffffffff $spa $tpa
> > > +
> > > +dnl traffic from localport should not be sent to localnet
> > > +AT_CHECK([tcpdump -r hv1/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000064 | wc -l],[0],[dnl
> > > +0
> > > +],[ignore])
> > > +
> > >   OVN_CLEANUP([hv1])
> > >   AT_CLEANUP
> > >   ])
> > >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>