[ovs-dev] [PATCH] ofproto-dpif-xlate: Fix recirculation when in_port is OFPP_CONTROLLER.

[Numan Siddique]

On Fri, Mar 20, 2020 at 6:26 AM Ben Pfaff <blp at ovn.org> wrote:
>
> Recirculation usually requires finding the pre-recirculation input port.
> Packets sent by the controller, with in_port of OFPP_CONTROLLER or
> OFPP_NONE, do not have a real input port data structure, only a port
> number.  The code in xlate_lookup_ofproto_() mishandled this case,
> failing to return the ofproto data structure.  This commit fixes the
> problem and adds a test to guard against regression.
>
> Reported-by: Numan Siddique <numans at ovn.org>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/368642.html
> Signed-off-by: Ben Pfaff <blp at ovn.org>

Thanks for fixing this issue.

Tested-by: Numan Siddique <numans at ovn.org>
Acked-by: Numan Siddique <numans at ovn.org>

Can you please also backport this patch to branch-2.13 ?

Thanks
Numan

> ---
>  ofproto/ofproto-dpif-xlate.c | 25 +++++++++++++++++++++----
>  tests/ofproto-dpif.at        | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index adf57a5e8929..28dcc67ddac3 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1520,15 +1520,32 @@ xlate_lookup_ofproto_(const struct dpif_backer *backer,
>              return NULL;
>          }
>
> -        /* If recirculation was initiated due to bond (in_port = OFPP_NONE)
> -         * then frozen state is static and xport_uuid is not defined, so xport
> -         * cannot be restored from frozen state. */
> -        if (recirc_id_node->state.metadata.in_port != OFPP_NONE) {
> +        ofp_port_t in_port = recirc_id_node->state.metadata.in_port;
> +        if (in_port != OFPP_NONE && in_port != OFPP_CONTROLLER) {
>              struct uuid xport_uuid = recirc_id_node->state.xport_uuid;
>              xport = xport_lookup_by_uuid(xcfg, &xport_uuid);
>              if (xport && xport->xbridge && xport->xbridge->ofproto) {
>                  goto out;
>              }
> +        } else {
> +            /* OFPP_NONE and OFPP_CONTROLLER are not real ports.  They indicate
> +             * that the packet originated from the controller via an OpenFlow
> +             * "packet-out".  The right thing to do is to find just the
> +             * ofproto.  There is no xport, which is OK.
> +             *
> +             * OFPP_NONE can also indicate that a bond caused recirculation. */
> +            struct uuid uuid = recirc_id_node->state.ofproto_uuid;
> +            const struct xbridge *bridge = xbridge_lookup_by_uuid(xcfg, &uuid);
> +            if (bridge && bridge->ofproto) {
> +                if (errorp) {
> +                    *errorp = NULL;
> +                }
> +                *xportp = NULL;
> +                if (ofp_in_port) {
> +                    *ofp_in_port = in_port;
> +                }
> +                return bridge->ofproto;
> +            }
>          }
>      }
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index ff1cc93707b8..d444cf0844a9 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5171,6 +5171,36 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 2
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> +# Checks for regression against a bug in which OVS dropped packets
> +# with in_port=CONTROLLER when they were recirculated (because
> +# CONTROLLER isn't a real port and could not be looked up).
> +AT_SETUP([ofproto-dpif - packet-out recirculation])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_DATA([flows.txt], [dnl
> +table=0 ip actions=mod_dl_dst:83:83:83:83:83:83,ct(table=1)
> +table=1 ip actions=ct(commit),output:2
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +packet=ffffffffffff00102030405008004500001c00000000401100000a000002ffffffff0035111100080000
> +AT_CHECK([ovs-ofctl packet-out br0 "in_port=controller packet=$packet actions=table"])
> +
> +# Dumps out the flow table, extracts the number of packets that have gone
> +# through the (single) flow in table 1, and returns success if it's exactly 1.
> +#
> +# If this remains 0, then the recirculation isn't working properly since the
> +# packet never goes through flow in table 1.
> +check_flows () {
> +    n=$(ovs-ofctl dump-flows br0 table=1 | sed -n 's/.*n_packets=\([[0-9]]\{1,\}\).*/\1/p')
> +    echo "n_packets=$n"
> +    test "$n" = 1
> +}
> +OVS_WAIT_UNTIL([check_flows], [ovs dump-flows br0])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
>
>  AT_SETUP([ofproto-dpif - debug_slow action])
>  OVS_VSWITCHD_START
> --
> 2.24.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>