[PATCH 0/2] perf/x86/intel/bts: Small fixes

[PATCH 0/2] perf/x86/intel/bts: Small fixes

[Alexander Shishkin]

Hi Peter and Ingo,

Here are two small fixes that resulted from running perf_fuzzer on a !KPTI
kernel. One is a misguided and unannotated warning and another is a sketchy
use of page_private(). The choice between deleting the BTS driver and
fixing it is not obvious, though. It may theoretically still have users.

Alexander Shishkin (2):
  perf/x86/intel/bts: Remove a silly warning
  perf/x86/intel/bts: Fix the use of page_private()

 arch/x86/events/intel/bts.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

-- 
2.24.0

[PATCH 1/2] perf/x86/intel/bts: Remove a silly warning

[Alexander Shishkin]

Commit

  8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")

brought in a warning with the BTS buffer initialization that doesn't make
sense, but is easily tripped with (assuming KPTI is disabled):

instantly throwing:

> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 326 at arch/x86/events/intel/bts.c:86 bts_buffer_setup_aux+0x117/0x3d0
> Modules linked in:
> CPU: 2 PID: 326 Comm: perf Not tainted 5.4.0-rc8-00291-gceb9e77324fa #904
> RIP: 0010:bts_buffer_setup_aux+0x117/0x3d0
> Call Trace:
>  rb_alloc_aux+0x339/0x550
>  perf_mmap+0x607/0xc70
>  mmap_region+0x76b/0xbd0
...

There is no comment or record anywhere that would explain the train of
thought that went into this warning, it probably tried to make sure that
the high order allocations indeed happened in the ring buffer code.

The rest of the driver handles multiple order-0 pages in the buffer just
fine. Therefore, get rid the warning. This was accidentally discovered by
Vince's perf_fuzzer.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Fixes: 8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")
Cc: Vince Weaver <vincent.weaver@maine.edu>
---
 arch/x86/events/intel/bts.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 38de4a7f6752..d53b4fb86d87 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -83,8 +83,6 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
 	/* count all the high order buffers */
 	for (pg = 0, nbuf = 0; pg < nr_pages;) {
 		page = virt_to_page(pages[pg]);
-		if (WARN_ON_ONCE(!PagePrivate(page) && nr_pages > 1))
-			return NULL;
 		pg += 1 << page_private(page);
 		nbuf++;
 	}
-- 
2.24.0

[PATCH 2/2] perf/x86/intel/bts: Fix the use of page_private()

[Alexander Shishkin]

Commit

  8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")

uses page_private(page) without checking the PagePrivate(page) first,
which seems like a potential bug, considering that page->private aliases
with other stuff in struct page.

Fix this by checking PagePrivate() first.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Fixes: 8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")
---
 arch/x86/events/intel/bts.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index d53b4fb86d87..9e4da1c5a129 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -63,9 +63,17 @@ struct bts_buffer {
 
 static struct pmu bts_pmu;
 
+static int buf_nr_pages(struct page *page)
+{
+	if (!PagePrivate(page))
+		return 1;
+
+	return 1 << page_private(page);
+}
+
 static size_t buf_size(struct page *page)
 {
-	return 1 << (PAGE_SHIFT + page_private(page));
+	return 1 << (PAGE_SHIFT + buf_nr_pages(page));
 }
 
 static void *
@@ -83,7 +91,7 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
 	/* count all the high order buffers */
 	for (pg = 0, nbuf = 0; pg < nr_pages;) {
 		page = virt_to_page(pages[pg]);
-		pg += 1 << page_private(page);
+		pg += buf_nr_pages(page);
 		nbuf++;
 	}
 
@@ -107,7 +115,7 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
 		unsigned int __nr_pages;
 
 		page = virt_to_page(pages[pg]);
-		__nr_pages = PagePrivate(page) ? 1 << page_private(page) : 1;
+		__nr_pages = buf_nr_pages(page);
 		buf->buf[nbuf].page = page;
 		buf->buf[nbuf].offset = offset;
 		buf->buf[nbuf].displacement = (pad ? BTS_RECORD_SIZE - pad : 0);
-- 
2.24.0

Re: [PATCH 1/2] perf/x86/intel/bts: Remove a silly warning

[Peter Zijlstra]

On Thu, Dec 05, 2019 at 05:28:52PM +0300, Alexander Shishkin wrote:
> There is no comment or record anywhere that would explain the train of
> thought that went into this warning, it probably tried to make sure that
> the high order allocations indeed happened in the ring buffer code.
> 

> --- a/arch/x86/events/intel/bts.c
> +++ b/arch/x86/events/intel/bts.c
> @@ -83,8 +83,6 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
>  	/* count all the high order buffers */
>  	for (pg = 0, nbuf = 0; pg < nr_pages;) {
>  		page = virt_to_page(pages[pg]);
> -		if (WARN_ON_ONCE(!PagePrivate(page) && nr_pages > 1))
> -			return NULL;
>  		pg += 1 << page_private(page);

I'm thinking that because ^^^^ uses page_private(), it wants to make
sure PagePrivate().

I haven't checked the current rules, but using page_private() without
PagePrivate() seems dodgy.

Also consider:

+               __nr_pages = PagePrivate(page) ? 1 << page_private(page) : 1;

>  		nbuf++;
>  	}
> -- 
> 2.24.0
> 

Re: [PATCH 2/2] perf/x86/intel/bts: Fix the use of page_private()

[Peter Zijlstra]

On Thu, Dec 05, 2019 at 05:28:53PM +0300, Alexander Shishkin wrote:
> Commit
> 
>   8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")
> 
> uses page_private(page) without checking the PagePrivate(page) first,

Well, arguably it did check it, but you didn't like that WARN ;-)

> which seems like a potential bug, considering that page->private aliases
> with other stuff in struct page.
> 
> Fix this by checking PagePrivate() first.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Fixes: 8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")
> ---
>  arch/x86/events/intel/bts.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
> index d53b4fb86d87..9e4da1c5a129 100644
> --- a/arch/x86/events/intel/bts.c
> +++ b/arch/x86/events/intel/bts.c
> @@ -63,9 +63,17 @@ struct bts_buffer {
>  
>  static struct pmu bts_pmu;
>  
> +static int buf_nr_pages(struct page *page)
> +{
> +	if (!PagePrivate(page))
> +		return 1;
> +
> +	return 1 << page_private(page);
> +}

Yes, that seems like a sensible helper.

Re: [PATCH 0/2] perf/x86/intel/bts: Small fixes

[Peter Zijlstra]

On Thu, Dec 05, 2019 at 05:28:51PM +0300, Alexander Shishkin wrote:
> Hi Peter and Ingo,
> 
> Here are two small fixes that resulted from running perf_fuzzer on a !KPTI
> kernel. One is a misguided and unannotated warning and another is a sketchy
> use of page_private(). The choice between deleting the BTS driver and
> fixing it is not obvious, though. It may theoretically still have users.
> 
> Alexander Shishkin (2):
>   perf/x86/intel/bts: Remove a silly warning
>   perf/x86/intel/bts: Fix the use of page_private()

I'll squash the pair, that makes more sense to me.

Re: [PATCH 2/2] perf/x86/intel/bts: Fix the use of page_private()

[Peter Zijlstra]

On Thu, Dec 05, 2019 at 05:28:53PM +0300, Alexander Shishkin wrote:
> Commit
> 
>   8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")
> 
> uses page_private(page) without checking the PagePrivate(page) first,
> which seems like a potential bug, considering that page->private aliases
> with other stuff in struct page.
> 
> Fix this by checking PagePrivate() first.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Fixes: 8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")
> ---
>  arch/x86/events/intel/bts.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
> index d53b4fb86d87..9e4da1c5a129 100644
> --- a/arch/x86/events/intel/bts.c
> +++ b/arch/x86/events/intel/bts.c
> @@ -63,9 +63,17 @@ struct bts_buffer {
>  
>  static struct pmu bts_pmu;
>  
> +static int buf_nr_pages(struct page *page)
> +{
> +	if (!PagePrivate(page))
> +		return 1;
> +
> +	return 1 << page_private(page);
> +}
> +
>  static size_t buf_size(struct page *page)
>  {
> -	return 1 << (PAGE_SHIFT + page_private(page));
> +	return 1 << (PAGE_SHIFT + buf_nr_pages(page));

Hurmph, shouldn't that be:

	return buf_nr_pages(page) * PAGE_SIZE;

?

Re: [PATCH 2/2] perf/x86/intel/bts: Fix the use of page_private()

[Alexander Shishkin]

Peter Zijlstra <peterz@infradead.org> writes:

>>  static size_t buf_size(struct page *page)
>>  {
>> -	return 1 << (PAGE_SHIFT + page_private(page));
>> +	return 1 << (PAGE_SHIFT + buf_nr_pages(page));
>
> Hurmph, shouldn't that be:
>
> 	return buf_nr_pages(page) * PAGE_SIZE;
>
> ?

True, that one's broken.

Re: [PATCH 0/2] perf/x86/intel/bts: Small fixes

[Alexander Shishkin]

Peter Zijlstra <peterz@infradead.org> writes:

> On Thu, Dec 05, 2019 at 05:28:51PM +0300, Alexander Shishkin wrote:
>> Hi Peter and Ingo,
>> 
>> Here are two small fixes that resulted from running perf_fuzzer on a !KPTI
>> kernel. One is a misguided and unannotated warning and another is a sketchy
>> use of page_private(). The choice between deleting the BTS driver and
>> fixing it is not obvious, though. It may theoretically still have users.
>> 
>> Alexander Shishkin (2):
>>   perf/x86/intel/bts: Remove a silly warning
>>   perf/x86/intel/bts: Fix the use of page_private()
>
> I'll squash the pair, that makes more sense to me.

Thanks!

[tip: perf/urgent] perf/x86/intel/bts: Fix the use of page_private()

[tip-bot2 for Alexander Shishkin]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     ff61541cc6c1962957758ba433c574b76f588d23
Gitweb:        https://git.kernel.org/tip/ff61541cc6c1962957758ba433c574b76f588d23
Author:        Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate:    Thu, 05 Dec 2019 17:28:52 +03:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 17 Dec 2019 13:32:46 +01:00

perf/x86/intel/bts: Fix the use of page_private()

Commit

  8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")

brought in a warning with the BTS buffer initialization
that is easily tripped with (assuming KPTI is disabled):

instantly throwing:

> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 326 at arch/x86/events/intel/bts.c:86 bts_buffer_setup_aux+0x117/0x3d0
> Modules linked in:
> CPU: 2 PID: 326 Comm: perf Not tainted 5.4.0-rc8-00291-gceb9e77324fa #904
> RIP: 0010:bts_buffer_setup_aux+0x117/0x3d0
> Call Trace:
>  rb_alloc_aux+0x339/0x550
>  perf_mmap+0x607/0xc70
>  mmap_region+0x76b/0xbd0
...

It appears to assume (for lost raisins) that PagePrivate() is set,
while later it actually tests for PagePrivate() before using
page_private().

Make it consistent and always check PagePrivate() before using
page_private().

Fixes: 8062382c8dbe2 ("perf/x86/intel/bts: Add BTS PMU driver")
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Link: https://lkml.kernel.org/r/20191205142853.28894-2-alexander.shishkin@linux.intel.com
---
 arch/x86/events/intel/bts.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 38de4a7..6a3b599 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -63,9 +63,17 @@ struct bts_buffer {
 
 static struct pmu bts_pmu;
 
+static int buf_nr_pages(struct page *page)
+{
+	if (!PagePrivate(page))
+		return 1;
+
+	return 1 << page_private(page);
+}
+
 static size_t buf_size(struct page *page)
 {
-	return 1 << (PAGE_SHIFT + page_private(page));
+	return buf_nr_pages(page) * PAGE_SIZE;
 }
 
 static void *
@@ -83,9 +91,7 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
 	/* count all the high order buffers */
 	for (pg = 0, nbuf = 0; pg < nr_pages;) {
 		page = virt_to_page(pages[pg]);
-		if (WARN_ON_ONCE(!PagePrivate(page) && nr_pages > 1))
-			return NULL;
-		pg += 1 << page_private(page);
+		pg += buf_nr_pages(page);
 		nbuf++;
 	}
 
@@ -109,7 +115,7 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
 		unsigned int __nr_pages;
 
 		page = virt_to_page(pages[pg]);
-		__nr_pages = PagePrivate(page) ? 1 << page_private(page) : 1;
+		__nr_pages = buf_nr_pages(page);
 		buf->buf[nbuf].page = page;
 		buf->buf[nbuf].offset = offset;
 		buf->buf[nbuf].displacement = (pad ? BTS_RECORD_SIZE - pad : 0);