linux-kernel - Re: [PATCH 4/4] perf tools: Fix struct comm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 Jul 2018 10:20:55 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>, lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        David Ahern <dsahern@...il.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Kan Liang <kan.liang@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Lukasz Odzioba <lukasz.odzioba@...el.com>,
        Wang Nan <wangnan0@...wei.com>, kernel-team@....com
Subject: Re: [PATCH 4/4] perf tools: Fix struct comm_str removal crash

Hi Arnaldo,

On Thu, Jul 19, 2018 at 03:31:14PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 19, 2018 at 03:28:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jul 19, 2018 at 04:33:45PM +0200, Jiri Olsa escreveu:
> > > +++ b/tools/perf/util/comm.c
> > > @@ -18,11 +18,9 @@ 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;
> > >  }
> > 
> > I don't like changing the semantics of a __get() operation this way, I
> > think it should stay like all the others, i.e. return the object with
> > the desired refcount or return NULL if that is not possible.
> > 
> > Otherwise we'll have to switch gears when debugging refcounts in various
> > objects, that start having slightly different semantics for reference
> > counting.
> > 
> > We should try to find a fix that maintains the semantics of refcounting.
> 
> After looking at the code, this refcount_inc_not_zero returns bool comes
> from the kernel, trying to see how this is used with __get() operations
> there, if at all.

Something like this?

static struct comm_str *comm_str__get(struct comm_str *cs)
{
    if (cs && refcount_inc_not_zero(&cs->refcnt))
        return cs;
    return NULL;
}


Other than that I don't have better idea, so

Acked-by: Namhyung Kim <namhyung@...nel.org>

Thanks,
Namhyung