Thank you. I pushed the fix to master and branch-1.5.
On Mon, Jan 30, 2012 at 01:13:15PM -0800, Ethan Jackson wrote:
> 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
