From a62699954a9f57c8407ba10d08636c79166f56ed Mon Sep 17 00:00:00 2001 From: Dave Barach Date: Wed, 7 Jun 2017 08:18:49 -0400 Subject: VPP-873: fix vector expansion bug in dispatch_pending_node The main interior graph-node dispatch loop had a longstanding dangling vector element reference: for (i = 0; i < _vec_len (nm->pending_frames); i++) cpu_time_now = dispatch_pending_node (vm, nm->pending_frames + i, cpu_time_now); Passing a pointer to a vector element (nm->pending_frames + i) has considerable comedic potential if there's any chance that the vector could expand. dispatch_pending_node() calls dispatch_node(), and indirectly any interior graph node dispatch function. If that node happens to expand nm->pending_frames by filling in a new frame, nm->pending_frames can expand. After calling the node dispatch function, dispatch_node() does the following: nf = vec_elt_at_index (nm->next_frames, p->next_frame_index); If nm->pending_frames expands during dispatch function execution, p is a dangling reference to freed memory. By luck, the TCP stack managed to allocate a fresh frame which included "old-p," which caused p->next_frame_index to be filled with the new-frame poison pattern 0xfefefefe. This has been broken from day 1, summer 2007, first use of the third-generation vector processing library. Change-Id: Ideb6363bb060c4e8bf9b901882c318bd83853121 Signed-off-by: Dave Barach --- src/vlib/main.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/src/vlib/main.c b/src/vlib/main.c index 0e6d66cd..14f680e6 100644 --- a/src/vlib/main.c +++ b/src/vlib/main.c @@ -1112,14 +1112,18 @@ dispatch_node (vlib_main_t * vm, } static u64 -dispatch_pending_node (vlib_main_t * vm, - vlib_pending_frame_t * p, u64 last_time_stamp) +dispatch_pending_node (vlib_main_t * vm, uword pending_frame_index, + u64 last_time_stamp) { vlib_node_main_t *nm = &vm->node_main; vlib_frame_t *f; vlib_next_frame_t *nf, nf_dummy; vlib_node_runtime_t *n; u32 restore_frame_index; + vlib_pending_frame_t *p; + + /* See comment below about dangling references to nm->pending_frames */ + p = nm->pending_frames + pending_frame_index; n = vec_elt_at_index (nm->nodes_by_type[VLIB_NODE_TYPE_INTERNAL], p->node_runtime_index); @@ -1169,18 +1173,29 @@ dispatch_pending_node (vlib_main_t * vm, /* Frame is ready to be used again, so restore it. */ if (restore_frame_index != ~0) { - /* we musn't restore a frame that is flagged to be freed. This shouldn't - happen since frames to be freed post dispatch are those used - when the to-node frame becomes full i.e. they form a sort of queue of - frames to a single node. If we get here then the to-node frame and the - pending frame *were* the same, and so we removed the to-node frame. - Therefore this frame is no longer part of the queue for that node - and hence it cannot be it's overspill. + /* + * We musn't restore a frame that is flagged to be freed. This + * shouldn't happen since frames to be freed post dispatch are + * those used when the to-node frame becomes full i.e. they form a + * sort of queue of frames to a single node. If we get here then + * the to-node frame and the pending frame *were* the same, and so + * we removed the to-node frame. Therefore this frame is no + * longer part of the queue for that node and hence it cannot be + * it's overspill. */ ASSERT (!(f->flags & VLIB_FRAME_FREE_AFTER_DISPATCH)); - /* p->next_frame_index can change during node dispatch if node - function decides to change graph hook up. */ + /* + * NB: dispatching node n can result in the creation and scheduling + * of new frames, and hence in the reallocation of nm->pending_frames. + * Recompute p, or no supper. This was broken for more than 10 years. + */ + p = nm->pending_frames + pending_frame_index; + + /* + * p->next_frame_index can change during node dispatch if node + * function decides to change graph hook up. + */ nf = vec_elt_at_index (nm->next_frames, p->next_frame_index); nf->flags |= VLIB_FRAME_IS_ALLOCATED; @@ -1607,8 +1622,7 @@ vlib_main_or_worker_loop (vlib_main_t * vm, int is_main) Process pending vector until there is nothing left. All pending vectors will be processed from input -> output. */ for (i = 0; i < _vec_len (nm->pending_frames); i++) - cpu_time_now = dispatch_pending_node (vm, nm->pending_frames + i, - cpu_time_now); + cpu_time_now = dispatch_pending_node (vm, i, cpu_time_now); /* Reset pending vector for next iteration. */ _vec_len (nm->pending_frames) = 0; -- cgit 1.2.3-korg