| Message ID | HE1PR07MB3082699FF247D4E255FE71E48A0D0@HE1PR07MB3082.eurprd07.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] xlate: fix xport lookup for recirc | expand |
On Thu, Dec 21, 2017 at 02:22:43PM +0000, Zoltán Balogh wrote: > Xlate_lookup and xlate_lookup_ofproto_() provides in_port and ofproto > based on xport determined using flow, which is extracted from packet. > The lookup can happen due to recirculation as well. It can happen, that > packet_type has been modified during xlate before recirculation is > triggered, so the lookup fails or delivers wrong xport. > This can be worked around by propagating xport to ctx->xin after the very > first lookup and store it in frozen state of the recirculation. > So, when lookup is performed due to recirculation, the xport can be > retrieved from the frozen state. > > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com> > CC: Jan Scheurich <jan.scheurich@ericsson.com> Thanks for working on finding and fixing bugs. Storing a pointer to an xport, then checking later that it points to a valid xport, is risky because it opens up the possibility that the pointer points to a different xport that just happens to have the same address. It's hard to guess how likely that coincidence is, but it would be better to avoid it. This is the reason that frozen_state uses a random UUID to locate "ofprotos". Probably, that approach would be better for xports too. When xport_in is nonnull but invalid, wouldn't it be better to abort than to search for an xport the fallback way? What is the reason for the special case for patch ports? Can you provide a good example of where this is important? Thanks, Ben.
> On Thu, Dec 21, 2017 at 02:22:43PM +0000, Zoltán Balogh wrote: > > Xlate_lookup and xlate_lookup_ofproto_() provides in_port and ofproto > > based on xport determined using flow, which is extracted from packet. > > The lookup can happen due to recirculation as well. It can happen, that > > packet_type has been modified during xlate before recirculation is > > triggered, so the lookup fails or delivers wrong xport. > > This can be worked around by propagating xport to ctx->xin after the very > > first lookup and store it in frozen state of the recirculation. > > So, when lookup is performed due to recirculation, the xport can be > > retrieved from the frozen state. > > > > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com> > > CC: Jan Scheurich <jan.scheurich@ericsson.com> > > Thanks for working on finding and fixing bugs. > > Storing a pointer to an xport, then checking later that it points to a > valid xport, is risky because it opens up the possibility that the > pointer points to a different xport that just happens to have the same > address. It's hard to guess how likely that coincidence is, but it > would be better to avoid it. This is the reason that frozen_state uses > a random UUID to locate "ofprotos". Probably, that approach would be > better for xports too. I see, thank you for the explanation. I'm going to create a new patch using xport UUIDs. > > When xport_in is nonnull but invalid, wouldn't it be better to abort > than to search for an xport the fallback way? Yes, that would be better. Especially if UUID is used to get the xport. > > What is the reason for the special case for patch ports? I've performed some tests before created the patch. I had a setup with two bridges connected with patch ports: p1 +-------+ +-------+ p2 ->-o | | o->- | br1 o-----o br2 | +-------+ +-------+ Recirculation can occur in the 2nd bridge as well, for instance when pop_mpls action is followed by resubmit. In this case a new upcall is performed and xlate_lookup_ofproto_() and xport_lookup() are invoked again. As I observed, xport_lookup() returns xport referring to p1 in br1, i.e. the very first port where the packet was received on. I assume, this is not a bug in OVS, is it? So, I would like to store this very first port in the frozen state. That's the reason why I do store xport in ctx.xin->xport_in only if xport is not a patch port. If it's not a patch port, it can be only the very first input port. > > Can you provide a good example of where this is important? The main goal of this patch is to avoid invocation of xlate_lookup() in case of recirculation (except recirc due to bond), because it can return a wrong xport. For instance, if L3 packet with MPLS label is received on a L3 tunnel port and pop_mpls + resubmit actions are performed, then first packet_type is changed due to pushing a dummy ethernet header, MPLS label is removed, then resubmit action is processed. This triggers recirculation, where xport_lookup() fails due to former change of packet_type as I noted in the commit message. > > Thanks, > > Ben.
Hi Zoltan,
As far as I can see the frozen state already contains the ofproto UUID and in frozen_metadata the OF port ID. Together these should be sufficient to look up the xport in xlate_lookup_ofproto_():
First look up the xbridge through xbridge_lookup_by_uuid() and afterwards scan the xports hmap of the xbridge for the OF port ID as done in static function get_ofp_port(xbridge, ofp_port).
Thus, there should be no need to add a UUID to the xport and store that in frozen_state.
Please also consider and deal with the other bug we found in xlate_actions:
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d94e9dc..bfa3acd 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7037,11 +7037,28 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
}
ctx.wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab;
+ /* FIXME: The line below looks up the ingress xport from the in_port
+ * in base_flow, which is populated with the initial OF port
+ * determined early in the upcall from the ODP port in the dp_packet's
+ * flow. In the case of recirculation in a subsequent bridge (think patch
+ * port or tunnel port) the ODP port is a port in the initial bridge,
+ * and its OF port number has no meaning in the current bridge. In the
+ * best case there is a miss, in the worst case the base_flow.in_port
+ * matches a bogus port in the current bridge. */
+
/* Get the proximate input port of the packet. (If xin->frozen_state,
* flow->in_port is the ultimate input port of the packet.) */
struct xport *in_port = get_ofp_port(xbridge,
ctx.base_flow.in_port.ofp_port);
+ /* It would be more correct to look up the xport from
+ * ctx.in.flow.in_port, which after recirculation has already been set
+ * with the thawed in_port in frozen_metadata_to_flow() above.
+ * Only in the case of bond recirculation there will be no valid in_port
+ * in the static recirculation context. But perhaps this is not a real
+ * problem as the output to bond action is typically the last action
+ * and the in_port won't matter anymore. */
+
if (flow->packet_type != htonl(PT_ETH) && in_port &&
in_port->pt_mode == NETDEV_PT_LEGACY_L3 && ctx.table_id == 0) {
/* Add dummy Ethernet header to non-L2 packet if it's coming from a
BR, Jan
> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Zoltán Balogh
> Sent: Friday, 05 January, 2018 12:27
> To: Ben Pfaff <blp@ovn.org>
> Cc: 'dev@openvswitch.org' <dev@openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH] xlate: fix xport lookup for recirc
>
> > On Thu, Dec 21, 2017 at 02:22:43PM +0000, Zoltán Balogh wrote:
> > > Xlate_lookup and xlate_lookup_ofproto_() provides in_port and ofproto
> > > based on xport determined using flow, which is extracted from packet.
> > > The lookup can happen due to recirculation as well. It can happen, that
> > > packet_type has been modified during xlate before recirculation is
> > > triggered, so the lookup fails or delivers wrong xport.
> > > This can be worked around by propagating xport to ctx->xin after the very
> > > first lookup and store it in frozen state of the recirculation.
> > > So, when lookup is performed due to recirculation, the xport can be
> > > retrieved from the frozen state.
> > >
> > > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> > > CC: Jan Scheurich <jan.scheurich@ericsson.com>
> >
> > Thanks for working on finding and fixing bugs.
> >
> > Storing a pointer to an xport, then checking later that it points to a
> > valid xport, is risky because it opens up the possibility that the
> > pointer points to a different xport that just happens to have the same
> > address. It's hard to guess how likely that coincidence is, but it
> > would be better to avoid it. This is the reason that frozen_state uses
> > a random UUID to locate "ofprotos". Probably, that approach would be
> > better for xports too.
>
> I see, thank you for the explanation. I'm going to create a new patch using
> xport UUIDs.
>
> >
> > When xport_in is nonnull but invalid, wouldn't it be better to abort
> > than to search for an xport the fallback way?
>
> Yes, that would be better. Especially if UUID is used to get the xport.
>
> >
> > What is the reason for the special case for patch ports?
>
> I've performed some tests before created the patch. I had a setup with two
> bridges connected with patch ports:
>
> p1 +-------+ +-------+ p2
> ->-o | | o->-
> | br1 o-----o br2 |
> +-------+ +-------+
>
> Recirculation can occur in the 2nd bridge as well, for instance when pop_mpls
> action is followed by resubmit. In this case a new upcall is performed and
> xlate_lookup_ofproto_() and xport_lookup() are invoked again. As I observed,
> xport_lookup() returns xport referring to p1 in br1, i.e. the very first port
> where the packet was received on. I assume, this is not a bug in OVS, is it?
>
> So, I would like to store this very first port in the frozen state. That's the
> reason why I do store xport in ctx.xin->xport_in only if xport is not a patch
> port. If it's not a patch port, it can be only the very first input port.
>
> >
> > Can you provide a good example of where this is important?
>
> The main goal of this patch is to avoid invocation of xlate_lookup() in case of
> recirculation (except recirc due to bond), because it can return a wrong xport.
> For instance, if L3 packet with MPLS label is received on a L3 tunnel port and
> pop_mpls + resubmit actions are performed, then first packet_type is changed
> due to pushing a dummy ethernet header, MPLS label is removed, then resubmit
> action is processed. This triggers recirculation, where xport_lookup() fails
> due to former change of packet_type as I noted in the commit message.
>
> >
> > Thanks,
> >
> > Ben.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hello Jan, > As far as I can see the frozen state already contains the ofproto UUID and in frozen_metadata the OF port ID. > Together these should be sufficient to look up the xport in xlate_lookup_ofproto_(): I know, that ofproto_uuid (bridge to resume from) and in_port (incoming port) are stored in frozen_state and frozen_metadata. As I mentioned these in a private message yesterday. I also wrote to you, that the stored ofproto_uuid refers to the bridge to resume from, and this can differ from the one xlate_lookup_ofproto_() could provide. For instance, in the case of using patch ports and recirculate in the second bridge. Please, double check your mail-box. > > First look up the xbridge through xbridge_lookup_by_uuid() and afterwards scan the xports hmap of the xbridge for > the OF port ID as done in static function get_ofp_port(xbridge, ofp_port). > > Thus, there should be no need to add a UUID to the xport and store that in frozen_state. > Actually, because of xlate_lookup_ofproto_() could provide a different ofproto, than stored in frozen_state, we cannot use ofproto_uuid from frozen_state to retrieve the correct xport. Am I wrong? > Please also consider and deal with the other bug we found in xlate_actions: > I hope, Ben (or the competent maintainer) will answer my question in my previous e-mail and clarify if the code you mentioned below needs to be modified or not. Best regards, Zoltan > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index d94e9dc..bfa3acd 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -7037,11 +7037,28 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) > } > ctx.wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; > > + /* FIXME: The line below looks up the ingress xport from the in_port > + * in base_flow, which is populated with the initial OF port > + * determined early in the upcall from the ODP port in the dp_packet's > + * flow. In the case of recirculation in a subsequent bridge (think patch > + * port or tunnel port) the ODP port is a port in the initial bridge, > + * and its OF port number has no meaning in the current bridge. In the > + * best case there is a miss, in the worst case the base_flow.in_port > + * matches a bogus port in the current bridge. */ > + > /* Get the proximate input port of the packet. (If xin->frozen_state, > * flow->in_port is the ultimate input port of the packet.) */ > struct xport *in_port = get_ofp_port(xbridge, > ctx.base_flow.in_port.ofp_port); > > + /* It would be more correct to look up the xport from > + * ctx.in.flow.in_port, which after recirculation has already been set > + * with the thawed in_port in frozen_metadata_to_flow() above. > + * Only in the case of bond recirculation there will be no valid in_port > + * in the static recirculation context. But perhaps this is not a real > + * problem as the output to bond action is typically the last action > + * and the in_port won't matter anymore. */ > + > if (flow->packet_type != htonl(PT_ETH) && in_port && > in_port->pt_mode == NETDEV_PT_LEGACY_L3 && ctx.table_id == 0) { > /* Add dummy Ethernet header to non-L2 packet if it's coming from a > > BR, Jan > > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Zoltán Balogh > > Sent: Friday, 05 January, 2018 12:27 > > To: Ben Pfaff <blp@ovn.org> > > Cc: 'dev@openvswitch.org' <dev@openvswitch.org> > > Subject: Re: [ovs-dev] [PATCH] xlate: fix xport lookup for recirc > > > > > On Thu, Dec 21, 2017 at 02:22:43PM +0000, Zoltán Balogh wrote: > > > > Xlate_lookup and xlate_lookup_ofproto_() provides in_port and ofproto > > > > based on xport determined using flow, which is extracted from packet. > > > > The lookup can happen due to recirculation as well. It can happen, that > > > > packet_type has been modified during xlate before recirculation is > > > > triggered, so the lookup fails or delivers wrong xport. > > > > This can be worked around by propagating xport to ctx->xin after the very > > > > first lookup and store it in frozen state of the recirculation. > > > > So, when lookup is performed due to recirculation, the xport can be > > > > retrieved from the frozen state. > > > > > > > > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com> > > > > CC: Jan Scheurich <jan.scheurich@ericsson.com> > > > > > > Thanks for working on finding and fixing bugs. > > > > > > Storing a pointer to an xport, then checking later that it points to a > > > valid xport, is risky because it opens up the possibility that the > > > pointer points to a different xport that just happens to have the same > > > address. It's hard to guess how likely that coincidence is, but it > > > would be better to avoid it. This is the reason that frozen_state uses > > > a random UUID to locate "ofprotos". Probably, that approach would be > > > better for xports too. > > > > I see, thank you for the explanation. I'm going to create a new patch using > > xport UUIDs. > > > > > > > > When xport_in is nonnull but invalid, wouldn't it be better to abort > > > than to search for an xport the fallback way? > > > > Yes, that would be better. Especially if UUID is used to get the xport. > > > > > > > > What is the reason for the special case for patch ports? > > > > I've performed some tests before created the patch. I had a setup with two > > bridges connected with patch ports: > > > > p1 +-------+ +-------+ p2 > > ->-o | | o->- > > | br1 o-----o br2 | > > +-------+ +-------+ > > > > Recirculation can occur in the 2nd bridge as well, for instance when pop_mpls > > action is followed by resubmit. In this case a new upcall is performed and > > xlate_lookup_ofproto_() and xport_lookup() are invoked again. As I observed, > > xport_lookup() returns xport referring to p1 in br1, i.e. the very first port > > where the packet was received on. I assume, this is not a bug in OVS, is it? > > > > So, I would like to store this very first port in the frozen state. That's the > > reason why I do store xport in ctx.xin->xport_in only if xport is not a patch > > port. If it's not a patch port, it can be only the very first input port. > > > > > > > > Can you provide a good example of where this is important? > > > > The main goal of this patch is to avoid invocation of xlate_lookup() in case of > > recirculation (except recirc due to bond), because it can return a wrong xport. > > For instance, if L3 packet with MPLS label is received on a L3 tunnel port and > > pop_mpls + resubmit actions are performed, then first packet_type is changed > > due to pushing a dummy ethernet header, MPLS label is removed, then resubmit > > action is processed. This triggers recirculation, where xport_lookup() fails > > due to former change of packet_type as I noted in the commit message. > > > > > > > > Thanks, > > > > > > Ben. > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c index fc5700489..79de90520 100644 --- a/ofproto/ofproto-dpif-rid.c +++ b/ofproto/ofproto-dpif-rid.c @@ -158,7 +158,8 @@ frozen_state_equal(const struct frozen_state *a, const struct frozen_state *b) && ofpacts_equal(a->ofpacts, a->ofpacts_len, b->ofpacts, b->ofpacts_len) && ofpacts_equal(a->action_set, a->action_set_len, - b->action_set, b->action_set_len)); + b->action_set, b->action_set_len) + && a->xport_in == b->xport_in); } /* Lockless RCU protected lookup. If node is needed accross RCU quiescent @@ -285,6 +286,7 @@ recirc_alloc_id(struct ofproto_dpif *ofproto) .ipv6_dst = in6addr_any, }, .in_port = OFPP_NONE }, + .xport_in = NULL, }; return recirc_alloc_id__(&state, frozen_state_hash(&state))->id; } diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index 19fc27c7c..f02221094 100644 --- a/ofproto/ofproto-dpif-rid.h +++ b/ofproto/ofproto-dpif-rid.h @@ -143,6 +143,7 @@ struct frozen_state { size_t stack_size; mirror_mask_t mirrors; /* Mirrors already output. */ bool conntracked; /* Conntrack occurred prior to freeze. */ + const struct xport *xport_in; /* Port packet was received on. */ /* Actions to be translated when thawing. */ struct ofpact *ofpacts; diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 02cf5415b..92a92d288 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -236,6 +236,8 @@ struct upcall { const struct nlattr *out_tun_key; /* Datapath output tunnel key. */ uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */ + + const struct xport *xport_in; /* Port the packet was received on. */ }; /* Ukeys must transition through these states using transition_ukey(). */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 9b3a2f28a..c9fb32338 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1362,12 +1362,36 @@ xlate_lookup_ofproto_(const struct dpif_backer *backer, const struct flow *flow, struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); const struct xport *xport; + /* If packet is recirculated, xport can be retrieved from frozen state. */ + if (flow->recirc_id) { + const struct recirc_id_node *recirc_id_node; + + recirc_id_node = recirc_id_node_find(flow->recirc_id); + + if (OVS_UNLIKELY(!recirc_id_node)) { + return NULL; + } + + /* If recirculation was initiated due to bond (in_port = OFPP_NONE), + * xport cannot be retrieved from frozen state. */ + if (recirc_id_node->state.metadata.in_port != OFPP_NONE) { + xport = recirc_id_node->state.xport_in; + /* Verify, if xport is valid. */ + if (xport && hmap_contains(&xcfg->xports, &xport->hmap_node) && + xport->xbridge && xport->xbridge->ofproto) { + goto out; + } + } + } + xport = xport_lookup(xcfg, tnl_port_should_receive(flow) ? tnl_port_receive(flow) : odp_port_to_ofport(backer, flow->in_port.odp_port)); if (OVS_UNLIKELY(!xport)) { return NULL; } + +out: *xportp = xport; if (ofp_in_port) { *ofp_in_port = xport->ofp_port; @@ -4623,6 +4647,7 @@ finish_freezing__(struct xlate_ctx *ctx, uint8_t table) .stack_size = ctx->stack.size, .mirrors = ctx->mirrors, .conntracked = ctx->conntracked, + .xport_in = ctx->xin->xport_in, .ofpacts = ctx->frozen_actions.data, .ofpacts_len = ctx->frozen_actions.size, .action_set = ctx->action_set.data, @@ -6613,6 +6638,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto, xin->odp_actions = odp_actions; xin->in_packet_out = false; xin->recirc_queue = NULL; + xin->xport_in = NULL; /* Do recirc lookup. */ xin->frozen_state = NULL; @@ -7040,6 +7066,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) * flow->in_port is the ultimate input port of the packet.) */ struct xport *in_port = get_ofp_port(xbridge, ctx.base_flow.in_port.ofp_port); + if (in_port && !in_port->peer) { + ctx.xin->xport_in = in_port; + } if (flow->packet_type != htonl(PT_ETH) && in_port && in_port->pt_mode == NETDEV_PT_LEGACY_L3 && ctx.table_id == 0) { @@ -7279,6 +7308,7 @@ xlate_resume(struct ofproto_dpif *ofproto, .stack_size = pin->stack_size, .mirrors = pin->mirrors, .conntracked = pin->conntracked, + .xport_in = NULL, /* When there are no actions, xlate_actions() will search the flow * table. We don't want it to do that (we want it to resume), so diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h index 39542de2b..932ebc492 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/ofproto/ofproto-dpif-xlate.h @@ -162,6 +162,9 @@ struct xlate_in { /* ofproto/trace maintains this queue to trace flows that require * recirculation. */ struct ovs_list *recirc_queue; + + /* Non-patch port packet was received on.*/ + const struct xport *xport_in; }; void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
Xlate_lookup and xlate_lookup_ofproto_() provides in_port and ofproto based on xport determined using flow, which is extracted from packet. The lookup can happen due to recirculation as well. It can happen, that packet_type has been modified during xlate before recirculation is triggered, so the lookup fails or delivers wrong xport. This can be worked around by propagating xport to ctx->xin after the very first lookup and store it in frozen state of the recirculation. So, when lookup is performed due to recirculation, the xport can be retrieved from the frozen state. Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com> CC: Jan Scheurich <jan.scheurich@ericsson.com> --- ofproto/ofproto-dpif-rid.c | 4 +++- ofproto/ofproto-dpif-rid.h | 1 + ofproto/ofproto-dpif-upcall.c | 2 ++ ofproto/ofproto-dpif-xlate.c | 30 ++++++++++++++++++++++++++++++ ofproto/ofproto-dpif-xlate.h | 3 +++ 5 files changed, 39 insertions(+), 1 deletion(-)