Re: [PATCH 1/4] perf tools: Fix struct comm_str removal crash

[Namhyung Kim]

Hi Jiri,

On Mon, Jul 16, 2018 at 12:29:34PM +0200, Jiri Olsa wrote:
> On Sun, Jul 15, 2018 at 10:08:27PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > > Because thread 2 first decrements the refcnt and only after then it
> > > removes the struct comm_str from the list, the thread 1 can find this
> > > object on the list with refcnt equls to 0 and hit the assert.
> > > 
> > > This patch fixes the thread 2 path, by removing the struct comm_str
> > > FIRST from the list and only AFTER calling comm_str__put on it. This
> > > way the thread 1 finds only valid objects on the list.
> > 
> > I'm not sure we can unconditionally remove the comm_str from the tree.
> > It should be removed only if refcount is going to zero IMHO.
> > Otherwise it could end up having multiple comm_str entry for a same
> > name.
> 
> right, but it wouldn't crash ;-)
> 
> how about attached change, that actualy deals with the refcnt
> race I'm running the tests now, seems ok so far

I think we can keep if the refcount is back to non-zero.  What about this?
(not tested..)


static struct comm_str *comm_str__get(cs)
{
    if (cs)
        refcount_inc_no_warn(&cs->refcnt);  // should be added
    return cs;
}

static void comm_str__put(cs)
{
    if (cs && refcount_dec_and_test(&cs->refcnt)) {
        down_write(&comm_str_lock);
        /* might race with comm_str__findnew() */
        if (!refcount_read(&cs->refcnt)) {
            rb_erase(&cs->rb_node, &comm_str_root);
            zfree(&cs->str);
            free(cs);
        }
        up_write(&comm_str_lock);
    }
}


Thanks,
Namhyung


> 
> 
> ---
> diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
> index 7798a2cc8a86..592b03548021 100644
> --- a/tools/perf/util/comm.c
> +++ b/tools/perf/util/comm.c
> @@ -18,22 +18,27 @@ struct comm_str {
>  static struct rb_root comm_str_root;
>  static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
>  
> -static struct comm_str *comm_str__get(struct comm_str *cs)
> +static bool comm_str__get(struct comm_str *cs)
>  {
> -	if (cs)
> -		refcount_inc(&cs->refcnt);
> -	return cs;
> +	return cs ? refcount_inc_not_zero(&cs->refcnt) : false;
>  }
>  
> -static void comm_str__put(struct comm_str *cs)
> +static int comm_str__put(struct comm_str *cs, bool lock)
>  {
> -	if (cs && refcount_dec_and_test(&cs->refcnt)) {
> +	if (!cs || !refcount_dec_and_test(&cs->refcnt))
> +		return 0;
> +
> +	if (lock)
>  		down_write(&comm_str_lock);
> -		rb_erase(&cs->rb_node, &comm_str_root);
> +
> +	rb_erase(&cs->rb_node, &comm_str_root);
> +
> +	if (lock)
>  		up_write(&comm_str_lock);
> -		zfree(&cs->str);
> -		free(cs);
> -	}
> +
> +	zfree(&cs->str);
> +	free(cs);
> +	return 1;
>  }
>  
>  static struct comm_str *comm_str__alloc(const char *str)
> @@ -67,9 +72,22 @@ struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
>  		parent = *p;
>  		iter = rb_entry(parent, struct comm_str, rb_node);
>  
> -		cmp = strcmp(str, iter->str);
> -		if (!cmp)
> -			return comm_str__get(iter);
> +		/*
> +		 * If we race with comm_str__put, iter->refcnt == 0
> +		 * and it will be removed within comm_str__put
> +		 * thread shortly, ignore it in this search.
> +		 */
> +		if (comm_str__get(iter)) {
> +			cmp = strcmp(str, iter->str);
> +			if (!cmp)
> +				return iter;
> +			/*
> +			 * If we actualy had to remove the item, restart
> +			 * the search to have the clean tree search.
> +			 */
> +			if (comm_str__put(iter, false))
> +				return __comm_str__findnew(str, root);
> +		}
>  
>  		if (cmp < 0)
>  			p = &(*p)->rb_left;
> @@ -125,7 +143,7 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp, bool exec)
>  	if (!new)
>  		return -ENOMEM;
>  
> -	comm_str__put(old);
> +	comm_str__put(old, true);
>  	comm->comm_str = new;
>  	comm->start = timestamp;
>  	if (exec)
> @@ -136,7 +154,7 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp, bool exec)
>  
>  void comm__free(struct comm *comm)
>  {
> -	comm_str__put(comm->comm_str);
> +	comm_str__put(comm->comm_str, true);
>  	free(comm);
>  }
>