linux-kernel - Re: perf events ring buffer memory barrier on powerpc

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]

Message-ID: <20131023141948.GB3566@localhost.localdomain>
Date:	Wed, 23 Oct 2013 15:19:51 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Michael Neuling <mikey@...ling.org>,
	Peter Zijlstra <peterz@...radead.org>
Cc:	benh@...nel.crashing.org, anton@...ba.org,
	linux-kernel@...r.kernel.org,
	Linux PPC dev <linuxppc-dev@...abs.org>,
	Victor Kaplansky <VICTORK@...ibm.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	michael@...erman.id.au
Subject: Re: perf events ring buffer memory barrier on powerpc

On Wed, Oct 23, 2013 at 10:54:54AM +1100, Michael Neuling wrote:
> Frederic,
> 
> In the perf ring buffer code we have this in perf_output_get_handle():
> 
> 	if (!local_dec_and_test(&rb->nest))
> 		goto out;
> 
> 	/*
> 	 * Publish the known good head. Rely on the full barrier implied
> 	 * by atomic_dec_and_test() order the rb->head read and this
> 	 * write.
> 	 */
> 	rb->user_page->data_head = head;
> 
> The comment says atomic_dec_and_test() but the code is
> local_dec_and_test().
> 
> On powerpc, local_dec_and_test() doesn't have a memory barrier but
> atomic_dec_and_test() does.  Is the comment wrong, or is
> local_dec_and_test() suppose to imply a memory barrier too and we have
> it wrongly implemented in powerpc?
> 
> My guess is that local_dec_and_test() is correct but we to add an
> explicit memory barrier like below:
> 
> (Kudos to Victor Kaplansky for finding this)
> 
> Mikey
> 
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index cd55144..95768c6 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -87,10 +87,10 @@ again:
>  		goto out;
>  
>  	/*
> -	 * Publish the known good head. Rely on the full barrier implied
> -	 * by atomic_dec_and_test() order the rb->head read and this
> -	 * write.
> +	 * Publish the known good head. We need a memory barrier to order the
> +	 * order the rb->head read and this write.
>  	 */
> +	smp_mb ();
>  	rb->user_page->data_head = head;
>  
>  	/*


I'm adding Peter in Cc since he wrote that code.
I agree that local_dec_and_test() doesn't need to imply an smp barrier.
All it has to provide as a guarantee is the atomicity against local concurrent
operations (interrupts, preemption, ...).

Now I'm a bit confused about this barrier.

I think we want this ordering:

    Kernel                             User

   READ rb->user_page->data_tail       READ rb->user_page->data_head
   smp_mb()                            smp_mb()
   WRITE rb data                       READ rb  data
   smp_mb()                            smp_mb()
   rb->user_page->data_head            WRITE rb->user_page->data_tail

So yeah we want a berrier between the data published and the user data_head.
But this ordering concerns wider layout than just rb->head and rb->user_page->data_head

And BTW I can see an smp_rmb() after we read rb->user_page->data_tail. This is probably the
first kernel barrier in my above example. (not sure if rmb() alone is enough though).
--
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/