[ovs-dev] [branch 2.1] ipfix, sflow: Fix a race.

Alex Wang alexw at nicira.com
Fri Jul 17 00:05:49 UTC 2015


On Thu, Jul 16, 2015 at 4:03 PM, Jarno Rajahalme <jrajahalme at nicira.com>
wrote:

> Alex,
>
> Not a review, but a comment: Isn’t it unnecessary to use atomics when the
> refcount is protected with a mutex?
>


That's very true, now atomics is unnecessary.



> Also, in general, a thread should not release the last reference if other
> threads can still find the object. Why does this happen here?
>


Thx Jarno for questioning this, after discussion offline, I realized that
this patch does not fix the issue reported.

Please ignore this patch,


>   Jarno
>
> > On Jul 16, 2015, at 3:48 PM, Alex Wang <alexw at nicira.com> wrote:
> >
> > In ovs 2.1, the unref functions for ipfix and sflow are called
> > in ofproto/ofproto-dpif-upcall.c without the protection of
> > any lock.  If the unprotected call of unref function removes
> > the last reference, and at the same time, other thread is
> > trying to ref the same struct, the race could cause abortion
> > of ovs.
> >
> > To fix this issue, this commit protects the ref and unref
> > functions using the global mutex of the ipfix and sflow
> > modules respectively.
> >
> > Reported-by: Xiao Ma <cumtb_maxiao at 163.com>
> > Signed-off-by: Alex Wang <alexw at nicira.com>
> > ---
> > ofproto/ofproto-dpif-ipfix.c |    6 ++++--
> > ofproto/ofproto-dpif-sflow.c |   14 +++++---------
> > 2 files changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index a9cc73e..8deace8 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -647,11 +647,13 @@ struct dpif_ipfix *
> > dpif_ipfix_ref(const struct dpif_ipfix *di_)
> > {
> >     struct dpif_ipfix *di = CONST_CAST(struct dpif_ipfix *, di_);
> > +    ovs_mutex_lock(&mutex);
> >     if (di) {
> >         int orig;
> >         atomic_add(&di->ref_cnt, 1, &orig);
> >         ovs_assert(orig > 0);
> >     }
> > +    ovs_mutex_unlock(&mutex);
> >     return di;
> > }
> >
> > @@ -689,16 +691,16 @@ dpif_ipfix_unref(struct dpif_ipfix *di)
> OVS_EXCLUDED(mutex)
> >         return;
> >     }
> >
> > +    ovs_mutex_lock(&mutex);
> >     atomic_sub(&di->ref_cnt, 1, &orig);
> >     ovs_assert(orig > 0);
> >     if (orig == 1) {
> > -        ovs_mutex_lock(&mutex);
> >         dpif_ipfix_clear(di);
> >         dpif_ipfix_bridge_exporter_destroy(&di->bridge_exporter);
> >         hmap_destroy(&di->flow_exporter_map);
> >         free(di);
> > -        ovs_mutex_unlock(&mutex);
> >     }
> > +    ovs_mutex_unlock(&mutex);
> > }
> >
> > static void
> > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> > index 158887f..07a5c4b 100644
> > --- a/ofproto/ofproto-dpif-sflow.c
> > +++ b/ofproto/ofproto-dpif-sflow.c
> > @@ -292,14 +292,6 @@ dpif_sflow_clear__(struct dpif_sflow *ds)
> OVS_REQUIRES(mutex)
> >     ds->probability = 0;
> > }
> >
> > -void
> > -dpif_sflow_clear(struct dpif_sflow *ds) OVS_EXCLUDED(mutex)
> > -{
> > -    ovs_mutex_lock(&mutex);
> > -    dpif_sflow_clear__(ds);
> > -    ovs_mutex_unlock(&mutex);
> > -}
> > -
> > bool
> > dpif_sflow_is_enabled(const struct dpif_sflow *ds) OVS_EXCLUDED(mutex)
> > {
> > @@ -336,11 +328,13 @@ struct dpif_sflow *
> > dpif_sflow_ref(const struct dpif_sflow *ds_)
> > {
> >     struct dpif_sflow *ds = CONST_CAST(struct dpif_sflow *, ds_);
> > +    ovs_mutex_lock(&mutex);
> >     if (ds) {
> >         int orig;
> >         atomic_add(&ds->ref_cnt, 1, &orig);
> >         ovs_assert(orig > 0);
> >     }
> > +    ovs_mutex_unlock(&mutex);
> >     return ds;
> > }
> >
> > @@ -366,19 +360,21 @@ dpif_sflow_unref(struct dpif_sflow *ds)
> OVS_EXCLUDED(mutex)
> >         return;
> >     }
> >
> > +    ovs_mutex_lock(&mutex);
> >     atomic_sub(&ds->ref_cnt, 1, &orig);
> >     ovs_assert(orig > 0);
> >     if (orig == 1) {
> >         struct dpif_sflow_port *dsp, *next;
> >
> >         route_table_unregister();
> > -        dpif_sflow_clear(ds);
> > +        dpif_sflow_clear__(ds);
> >         HMAP_FOR_EACH_SAFE (dsp, next, hmap_node, &ds->ports) {
> >             dpif_sflow_del_port__(ds, dsp);
> >         }
> >         hmap_destroy(&ds->ports);
> >         free(ds);
> >     }
> > +    ovs_mutex_unlock(&mutex);
> > }
> >
> > static void
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>



More information about the dev mailing list