[ovs-dev] [PATCH] ofproto: Fix use-after-free error when ports disappear.

[Ben Pfaff]

Pushed to master, branch-1.[23456].

On Mon, Apr 23, 2012 at 10:34:02AM -0700, Justin Pettit wrote:
> Looks good to me.  Thanks.
> 
> --Justin
> 
> 
> On Apr 23, 2012, at 9:16 AM, Ben Pfaff wrote:
> 
> > update_port() can delete the port for which it is called, if the underlying
> > network device has been destroyed, so HMAP_FOR_EACH is unsafe in
> > ofproto_run().
> > 
> > Less obviously, update_port() can delete unrelated ports.  For example,
> > suppose that initially device A is port 1 and device B is port 2.  If
> > update_port("A") runs just after this, then it will ofport_remove() both
> > ports, then ofport_install() A as the new port 2.
> > 
> > So this commit first assembles a list of ports to update, then updates them
> > in a separate loop.
> > 
> > Without this commit, running "ovs-dpctl del-dp" while ovs-vswitchd is
> > running consistently causes a crash for me within a few seconds.
> > 
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > ofproto/ofproto.c |   18 ++++++++++++++++--
> > 1 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index f934306..cb46d26 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1027,8 +1027,9 @@ process_port_change(struct ofproto *ofproto, int error, char *devname)
> > int
> > ofproto_run(struct ofproto *p)
> > {
> > +    struct sset changed_netdevs;
> > +    const char *changed_netdev;
> >     struct ofport *ofport;
> > -    char *devname;
> >     int error;
> > 
> >     error = p->ofproto_class->run(p);
> > @@ -1037,18 +1038,31 @@ ofproto_run(struct ofproto *p)
> >     }
> > 
> >     if (p->ofproto_class->port_poll) {
> > +        char *devname;
> > +
> >         while ((error = p->ofproto_class->port_poll(p, &devname)) != EAGAIN) {
> >             process_port_change(p, error, devname);
> >         }
> >     }
> > 
> > +    /* Update OpenFlow port status for any port whose netdev has changed.
> > +     *
> > +     * Refreshing a given 'ofport' can cause an arbitrary ofport to be
> > +     * destroyed, so it's not safe to update ports directly from the
> > +     * HMAP_FOR_EACH loop, or even to use HMAP_FOR_EACH_SAFE.  Instead, we
> > +     * need this two-phase approach. */
> > +    sset_init(&changed_netdevs);
> >     HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
> >         unsigned int change_seq = netdev_change_seq(ofport->netdev);
> >         if (ofport->change_seq != change_seq) {
> >             ofport->change_seq = change_seq;
> > -            update_port(p, netdev_get_name(ofport->netdev));
> > +            sset_add(&changed_netdevs, netdev_get_name(ofport->netdev));
> >         }
> >     }
> > +    SSET_FOR_EACH (changed_netdev, &changed_netdevs) {
> > +        update_port(p, changed_netdev);
> > +    }
> > +    sset_destroy(&changed_netdevs);
> > 
> >     switch (p->state) {
> >     case S_OPENFLOW:
> > -- 
> > 1.7.2.5
> > 
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>