Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 17 May 2019 10:47:12 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Yabin Cui <yabinc@google.com>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] perf/ring_buffer: Fix exposing a temporarily
 decreased data_head.
Message-ID: <20190517084712.GL2623@hirez.programming.kicks-ass.net>
References: <20190515003059.23920-1-yabinc@google.com>
 <20190516184010.167903-1-yabinc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190516184010.167903-1-yabinc@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 16, 2019 at 11:40:10AM -0700, Yabin Cui wrote:
> In perf_output_put_handle(), an IRQ/NMI can happen in below location and
> write records to the same ring buffer:
> 	...
> 	local_dec_and_test(&rb->nest)
> 	...                          <-- an IRQ/NMI can happen here
> 	rb->user_page->data_head = head;
> 	...
> 
> In this case, a value A is written to data_head in the IRQ, then a value
> B is written to data_head after the IRQ. And A > B. As a result,
> data_head is temporarily decreased from A to B. And a reader may see
> data_head < data_tail if it read the buffer frequently enough, which
> creates unexpected behaviors.
> 
> This can be fixed by moving dec(&rb->nest) to after updating data_head,
> which prevents the IRQ/NMI above from updating data_head.
> 
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> 
> v1 -> v2: change rb->nest from local_t to unsigned int, and add barriers.
> 
> ---
>  kernel/events/internal.h    |  2 +-
>  kernel/events/ring_buffer.c | 24 ++++++++++++++++++------
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/events/internal.h b/kernel/events/internal.h
> index 79c47076700a..0a8c003b9bcf 100644
> --- a/kernel/events/internal.h
> +++ b/kernel/events/internal.h
> @@ -24,7 +24,7 @@ struct ring_buffer {
>  	atomic_t			poll;		/* POLL_ for wakeups */
>  
>  	local_t				head;		/* write position    */
> -	local_t				nest;		/* nested writers    */
> +	unsigned int			nest;		/* nested writers    */
>  	local_t				events;		/* event limit       */
>  	local_t				wakeup;		/* wakeup stamp      */
>  	local_t				lost;		/* nr records lost   */
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 674b35383491..c677beb01fb1 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -38,7 +38,8 @@ static void perf_output_get_handle(struct perf_output_handle *handle)
>  	struct ring_buffer *rb = handle->rb;
>  
>  	preempt_disable();
> -	local_inc(&rb->nest);
> +	rb->nest++;
> +	barrier();

Urgh; almost but not quite. You just lost the 'volatile' qualifier and
now the compiler can mess things up for you.

>  	handle->wakeup = local_read(&rb->wakeup);
>  }

What I'm going to do is split this into two patches, one fixes the
problem and marked for backport, and one changing away from local_t.