linux-kernel - Re: [PATCH tip 1/1] perf_counter tools: Add locking to perf top

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] [day] [month] [year] [list]

Date:	Sat, 30 May 2009 12:31:26 +0200
From:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
To:	Arnaldo Carvalho de Melo <acme@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Mike Galbraith <efault@....de>,
	Paul Mackerras <paulus@...ba.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH tip 1/1] perf_counter tools: Add locking to perf top

On Fri, 2009-05-29 at 17:33 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, May 29, 2009 at 10:22:17PM +0200, Peter Zijlstra escreveu:
> > On Fri, 2009-05-29 at 17:03 -0300, Arnaldo Carvalho de Melo wrote:
> > >         /* Sort the active symbols */
> > > -       list_for_each_entry_safe(syme, n, &active_symbols, node) {
> > > -               if (syme->count[0] != 0) {
> > > +       pthread_mutex_lock(&active_symbols_lock);
> > > +       syme = list_entry(active_symbols.next, struct sym_entry, node);
> > > +       pthread_mutex_unlock(&active_symbols_lock);
> > > +
> > > +       list_for_each_entry_safe_from(syme, n, &active_symbols, node) {
> > > +               syme->snap_count = syme->count[0];
> > > +               if (syme->snap_count != 0) {
> > > +                       syme->weight = sym_weight(syme);
> > 
> > That looks wrong, you basically do a fancy cast while holding the lock,
> > then you overwrite the variable doing a list iteration without holding
> > the lock.
> > 
> > If list_add and list_del are under a lock, the iteration should be too.
> 
> Look closer :)
> 
> 1) List insertion is only done at the head and by the other thread, thus
> the lock above. The other thread will only mess with the above
> syme->node.prev when inserting a new head, never with .next.
> 
> 2) List deletion is only done after taking the lock, and on the above
> thread.
> 
> Only problem probably is to access syme->count[0], that on some
> architectures may not be atomic.

If you want to do that, you need to use the rcu list primitives,
otherwise insertion vs iteration isn't safe. Memory barriers and
ordering still matter in userspace.

And whichever way you turn that list_entry() under mutex, that's utter
rubbish, it doesn't protect anything.

As it stands, this code is more broken than it was before for the simple
reason that it might fool people into believing its ok.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/