Alex,
Not a review, but a comment: Isn’t it unnecessary to use atomics when the
refcount is protected with a mutex?
Also, in general, a thread should not release the last reference if other
threads can still find the object. Why does this happen here?
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
