Thanks for tracking this down. Looks good to me.
Ethan
On Mon, Jan 30, 2012 at 13:10, Ben Pfaff <b...@nicira.com> wrote:
> When handle_flow_miss() saw that subfacet did not have any actions, then
> the associated packet would get freed early, in the loop that constructs
> the set of batched operations. However, there would still be a "flow_put"
> operation that referenced the key that shares the same memory block as the
> packet. The memory allocator would overwrite the first few bytes of this
> block, causing bizarre errors in the flow_put.
>
> This commit changes the memory release strategy to be less error-prone, by
> deferring all freeing of packets to the end of the function. With this
> change, every packet gets freed in the same place, instead of having some
> packets freed in one place and other packets freed in another.
>
> Here is the valgrind report that pinpoints the problem:
>
> Invalid read of size 4
> at 0x4026838: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
> by 0x80E9B52: dpif_linux_flow_to_ofpbuf (dpif-linux.c:1714)
> by 0x80E9C77: dpif_linux_operate (dpif-linux.c:883)
> by 0x80AFB5A: dpif_operate (dpif.c:994)
> by 0x809A03B: handle_upcalls (ofproto-dpif.c:2758)
> by 0x809A23A: run_fast (ofproto-dpif.c:757)
> by 0x808C04E: ofproto_run_fast (ofproto.c:963)
> by 0x806DFB6: bridge_run_fast (bridge.c:1811)
> by 0x8074B59: main (ovs-vswitchd.c:98)
> Address 0x4427948 is 80 bytes inside a block of size 2,048 free'd
> at 0x402421C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
> by 0x80CD865: ofpbuf_delete (ofpbuf.c:187)
> by 0x80CD8AA: ofpbuf_list_delete (ofpbuf.c:531)
> by 0x8099F06: handle_upcalls (ofproto-dpif.c:2747)
> by 0x809A23A: run_fast (ofproto-dpif.c:757)
> by 0x808C04E: ofproto_run_fast (ofproto.c:963)
> by 0x806DFB6: bridge_run_fast (bridge.c:1811)
> by 0x8074B59: main (ovs-vswitchd.c:98)
>
> Bug #9346.
> Reported-by: Alan Shieh <ash...@nicira.com>
> Reported-by: Ethan Jackson <et...@nicira.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 14 +++++++-------
> 1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 344f9d4..51d3f3f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2579,7 +2579,6 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct
> flow_miss *miss,
> continue;
> }
>
> - list_remove(&packet->list_node);
> if (flow->vlan_tci != subfacet->initial_tci) {
> /* This packet was received on a VLAN splinter port. We added
> * a VLAN to the packet to make the packet resemble the flow,
> @@ -2742,14 +2741,10 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto,
> struct dpif_upcall *upcalls,
> /* Process each element in the to-do list, constructing the set of
> * operations to batch. */
> n_ops = 0;
> - HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> + HMAP_FOR_EACH (miss, hmap_node, &todo) {
> handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
> - ofpbuf_list_delete(&miss->packets);
> - hmap_remove(&todo, &miss->hmap_node);
> - free(miss);
> }
> assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
> - hmap_destroy(&todo);
>
> /* Execute batch. */
> for (i = 0; i < n_ops; i++) {
> @@ -2768,7 +2763,6 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto,
> struct dpif_upcall *upcalls,
> if (op->subfacet->actions != execute->actions) {
> free((struct nlattr *) execute->actions);
> }
> - ofpbuf_delete((struct ofpbuf *) execute->packet);
> break;
>
> case DPIF_OP_FLOW_PUT:
> @@ -2778,6 +2772,12 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto,
> struct dpif_upcall *upcalls,
> break;
> }
> }
> + HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> + ofpbuf_list_delete(&miss->packets);
> + hmap_remove(&todo, &miss->hmap_node);
> + free(miss);
> + }
> + hmap_destroy(&todo);
> }
>
> static void
> --
> 1.7.2.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
