[PATCH] perf: fix ring_buffer perf_output_space() boundary calculation

[PATCH] perf: fix ring_buffer perf_output_space() boundary calculation

[Stephane Eranian]

This patch fixes a flaw in perf_output_space(). In case the size
of the space needed is bigger than the actual buffer size, there
may be situations where the function would return true (i.e., there
is space) when it should not. head > offset due to rounding of the
masking logic.

The problem can be tested by activating BTS on Intel processors.
A BTS record can be as big as 16 pages. The following command
fails:

$ perf record -m 4 -c 1 -e branches:u my_test_program

You will get a buffer corruption with this. Perf report won't be
able to parse the perf.data.

The fix is to first check that the requested space is smaller than the
buffer size. If so, then the masking logic will work fine. If not, then
there is no chance the record can be saved and it will be gracefully handled
by upper code layers.

This patch also fixes the writable test. If the buffer is not writable, the
function should return false because obviously nothing can be written in
the buffer.

Signed-off-by: Stephane Eranian <eranian@google.com>
---

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 23cb34f..73147b9 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -18,12 +18,19 @@
 static bool perf_output_space(struct ring_buffer *rb, unsigned long tail,
 			      unsigned long offset, unsigned long head)
 {
-	unsigned long mask;
+	unsigned long sz = perf_data_size(rb);
+	unsigned long mask = sz - 1;
 
 	if (!rb->writable)
-		return true;
+		return false;
 
-	mask = perf_data_size(rb) - 1;
+	/*
+	 * verify that payload is not bigger than buffer
+	 * otherwise masking logic may fail to detect
+	 * the "not enough space" condition
+	 */
+	if ((head - offset) > sz)
+		return false;
 
 	offset = (offset - tail) & mask;
 	head   = (head   - tail) & mask;

Re: [PATCH] perf: fix ring_buffer perf_output_space() boundary calculation

[Peter Zijlstra]

On Mon, 2013-03-18 at 13:48 +0100, Stephane Eranian wrote:
>         if (!rb->writable)
> -               return true;
> +               return false;


writable means user writable (VM_WRITE); the difference is that a
!VM_WRITE buffer will simply over-write its own tail whereas a VM_WRITE
buffer will drop events.

So returning true for !VM_WRITE makes sense, there's always space.

Re: [PATCH] perf: fix ring_buffer perf_output_space() boundary calculation

[Stephane Eranian]

On Mon, Mar 18, 2013 at 1:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2013-03-18 at 13:48 +0100, Stephane Eranian wrote:
>>         if (!rb->writable)
>> -               return true;
>> +               return false;
>
>
> writable means user writable (VM_WRITE); the difference is that a
> !VM_WRITE buffer will simply over-write its own tail whereas a VM_WRITE
> buffer will drop events.
>
> So returning true for !VM_WRITE makes sense, there's always space.
>
Ok, that was not so clear to me. I think this if() statment deserves a comment.
I will add that in V2.

Re: [PATCH] perf: fix ring_buffer perf_output_space() boundary calculation

[Peter Zijlstra]

On Mon, 2013-03-18 at 14:03 +0100, Stephane Eranian wrote:
> On Mon, Mar 18, 2013 at 1:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, 2013-03-18 at 13:48 +0100, Stephane Eranian wrote:
> >>         if (!rb->writable)
> >> -               return true;
> >> +               return false;
> >
> >
> > writable means user writable (VM_WRITE); the difference is that a
> > !VM_WRITE buffer will simply over-write its own tail whereas a VM_WRITE
> > buffer will drop events.
> >
> > So returning true for !VM_WRITE makes sense, there's always space.
> >
> Ok, that was not so clear to me. I think this if() statment deserves a comment.
> I will add that in V2.

Thanks; I suppose renaming the entire ->writable thing might be better
though. Something like ->overwrite or so.

Re: [PATCH] perf: fix ring_buffer perf_output_space() boundary calculation

[Stephane Eranian]

On Mon, Mar 18, 2013 at 2:11 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2013-03-18 at 14:03 +0100, Stephane Eranian wrote:
>> On Mon, Mar 18, 2013 at 1:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Mon, 2013-03-18 at 13:48 +0100, Stephane Eranian wrote:
>> >>         if (!rb->writable)
>> >> -               return true;
>> >> +               return false;
>> >
>> >
>> > writable means user writable (VM_WRITE); the difference is that a
>> > !VM_WRITE buffer will simply over-write its own tail whereas a VM_WRITE
>> > buffer will drop events.
>> >
>> > So returning true for !VM_WRITE makes sense, there's always space.
>> >
>> Ok, that was not so clear to me. I think this if() statment deserves a comment.
>> I will add that in V2.
>
> Thanks; I suppose renaming the entire ->writable thing might be better
> though. Something like ->overwrite or so.
>
Yes. And if you do this you invert the logic of the field.
overwrite=1 -> !VM_WRITE
overwrite=0 -> VM_WRITE