| Message ID | 20200320005626.3669994-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Commit | 323ae1e808e6ac503f5c7ddd50a79d908fdd0e41 |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-xlate: Fix recirculation when in_port is OFPP_CONTROLLER. | expand |
On Fri, Mar 20, 2020 at 6:26 AM Ben Pfaff <blp@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@ovn.org> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/368642.html > Signed-off-by: Ben Pfaff <blp@ovn.org> Thanks for fixing this issue. Tested-by: Numan Siddique <numans@ovn.org> Acked-by: Numan Siddique <numans@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Mar 20, 2020 at 02:12:55PM +0530, Numan Siddique wrote: > On Fri, Mar 20, 2020 at 6:26 AM Ben Pfaff <blp@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@ovn.org> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/368642.html > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Thanks for fixing this issue. > > Tested-by: Numan Siddique <numans@ovn.org> > Acked-by: Numan Siddique <numans@ovn.org> > > Can you please also backport this patch to branch-2.13 ? Thanks for testing and for the high quality bug report. I applied this to master and branch-2.13.
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
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@ovn.org> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/368642.html Signed-off-by: Ben Pfaff <blp@ovn.org> --- ofproto/ofproto-dpif-xlate.c | 25 +++++++++++++++++++++---- tests/ofproto-dpif.at | 30 ++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-)