Tun_metadata can be referened by flow and frozen_state at the same time. When ovs-vswitchd handles TLV table mod message, the involved tun_metadata gets freed. The call trace to free tun_metadata is shown as below:
ofproto_run - handle_openflow - handle_single_part_openflow - handle_tlv_table_mod - tun_metadata_table_mod - tun_metadata_postpone_free Unfortunately, this tun_metadata can be still used by some frozen_state, and later on when frozen_state tries to access its tun_metadata table, ovs-vswitchd crashes. The call trace to access tun_metadata from frozen_state is shown as below: udpif_upcall_handler - recv_upcalls - process_upcall - frozen_metadata_to_flow It is unsafe for frozen_state to reference tun_table because tun_table is protected by RCU while the lifecycle of frozen_state can span several RCU quiesce states. Current code violates OVS's RCU protection mechanism. This patch fixes it by simply stopping frozen_state from referencing tun_table. If frozen_state needs tun_table, the latest valid tun_table can be found through ofproto_get_tun_tab() efficiently. A previous commit seems fixing the samiliar issue: 254878c18874f6 (ofproto-dpif-xlate: Fix segmentation fault caused by tun_table) VMware-BZ: #2526222 Signed-off-by: Yifeng Sun <pkusunyif...@gmail.com> --- v1->v2: Drop the fix based on reference count. It doesn't fit well with RCU mechanism. Thanks William and YiHung for the offline discussion. v2->v3: frozen_metadata_to_flow() looks up and assigns flow's tun_table. Thanks YiHung's suggestion. ofproto/ofproto-dpif-rid.h | 12 +++++++++++- ofproto/ofproto-dpif-upcall.c | 3 ++- ofproto/ofproto-dpif-xlate.c | 9 +++------ 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index e5d02caf28a3..30cd5275f24c 100644 --- a/ofproto/ofproto-dpif-rid.h +++ b/ofproto/ofproto-dpif-rid.h @@ -22,6 +22,7 @@ #include "cmap.h" #include "ofproto-dpif-mirror.h" +#include "ofproto/ofproto-provider.h" #include "openvswitch/list.h" #include "openvswitch/ofp-actions.h" #include "ovs-thread.h" @@ -115,16 +116,25 @@ frozen_metadata_from_flow(struct frozen_metadata *md, { memset(md, 0, sizeof *md); md->tunnel = flow->tunnel; + /* It is unsafe for frozen_state to reference tun_table because + * tun_table is protected by RCU while the lifecycle of frozen_state + * can span several RCU quiesce states. + * + * The latest valid tun_table can be found by ofproto_get_tun_tab() + * efficiently. */ + md->tunnel.metadata.tab = NULL; md->metadata = flow->metadata; memcpy(md->regs, flow->regs, sizeof md->regs); md->in_port = flow->in_port.ofp_port; } static inline void -frozen_metadata_to_flow(const struct frozen_metadata *md, +frozen_metadata_to_flow(struct ofproto *ofproto, + const struct frozen_metadata *md, struct flow *flow) { flow->tunnel = md->tunnel; + flow->tunnel.metadata.tab = ofproto_get_tun_tab(ofproto); flow->metadata = md->metadata; memcpy(flow->regs, md->regs, sizeof flow->regs); flow->in_port.ofp_port = md->in_port; diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 8dfa05b71df4..5e08ef10dad6 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1534,7 +1534,8 @@ process_upcall(struct udpif *udpif, struct upcall *upcall, flow_clear_conntrack(&frozen_flow); } - frozen_metadata_to_flow(&state->metadata, &frozen_flow); + frozen_metadata_to_flow(&upcall->ofproto->up, &state->metadata, + &frozen_flow); flow_get_metadata(&frozen_flow, &am->pin.up.base.flow_metadata); ofproto_dpif_send_async_msg(upcall->ofproto, am); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 042c50a6346c..abce976c6c2f 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -7544,7 +7544,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) /* Restore pipeline metadata. May change flow's in_port and other * metadata to the values that existed when freezing was triggered. */ - frozen_metadata_to_flow(&state->metadata, flow); + frozen_metadata_to_flow(&ctx.xbridge->ofproto->up, + &state->metadata, flow); /* Restore stack, if any. */ if (state->stack) { @@ -7596,14 +7597,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) ctx.error = XLATE_INVALID_TUNNEL_METADATA; goto exit; } - } else if (!flow->tunnel.metadata.tab || xin->frozen_state) { + } else if (!flow->tunnel.metadata.tab) { /* If the original flow did not come in on a tunnel, then it won't have * FLOW_TNL_F_UDPIF set. However, we still need to have a metadata * table in case we generate tunnel actions. */ - /* If the translation is from a frozen state, we use the latest - * TLV map to avoid segmentation fault in case the old TLV map is - * replaced by a new one. - * XXX: It is better to abort translation if the table is changed. */ flow->tunnel.metadata.tab = ofproto_get_tun_tab( &ctx.xbridge->ofproto->up); } -- 2.7.4 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev