On Thu, Jul 16, 2015 at 4:03 PM, Jarno Rajahalme <jrajaha...@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 <al...@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_max...@163.com>
> > Signed-off-by: Alex Wang <al...@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@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
