[PATCH] s390: Fix perf event init

[PATCH] s390: Fix perf event init

[Paul Burton]

Commit c311c797998c ("cpumask: make "nr_cpumask_bits" unsigned")
modified cpumsf_pmu_event_init() to cast the struct perf_event cpu field
to an unsigned integer before it is compared with nr_cpumask_bits. This
is broken because the cpu field may be -1 for events which follow a
process rather than being affine to a particular CPU. When this is the
case the cast to an unsigned int results in a value equal to ULONG_MAX,
which is always greater than nr_cpumask_bits so we always fail
cpumsf_pmu_event_init() and return -ENODEV.

The check against nr_cpumask_bits seems nonsensical anyway, so this
patch simply removes it. The cpu field is going to either be 0 or a
valid CPU number. Comparing it with nr_cpumask_bits is effectively
checking that it's a valid cpu number, but it seems safe to rely on the
core perf events code to ensure that's the case.

The end result is that this fixes use of perf when not constraining
events to a particular CPU, and fixes the "perf list hw" command which
fails to list any events without this.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Fixes: c311c797998c ("cpumask: make "nr_cpumask_bits" unsigned")
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: stable <stable@vger.kernel.org> # v4.12+

---
This patch is purely speculative, based upon fixing perf on MIPS
following the offending commit c311c797998c ("cpumask: make
"nr_cpumask_bits" unsigned") which made the same change to both MIPS &
s390. I leave it to s390 developers to verify whether this affects them
in the same way as it affected MIPS thus whether this fix is correct
there too.

Thanks,
    Paul

 arch/s390/kernel/perf_cpum_sf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index c1bf75ffb875..cc570143f245 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -823,8 +823,7 @@ static int cpumsf_pmu_event_init(struct perf_event *event)
 	}
 
 	/* Check online status of the CPU to which the event is pinned */
-	if ((unsigned int)event->cpu >= nr_cpumask_bits ||
-	    (event->cpu >= 0 && !cpu_online(event->cpu)))
+	if (event->cpu >= 0 && !cpu_online(event->cpu))
 		return -ENODEV;
 
 	/* Force reset of idle/hv excludes regardless of what the
-- 
2.14.1

Re: [PATCH] s390: Fix perf event init

[Martin Schwidefsky]

Hi Paul,

On Tue, 19 Sep 2017 22:07:57 -0700
Paul Burton <paul.burton@imgtec.com> wrote:

> Commit c311c797998c ("cpumask: make "nr_cpumask_bits" unsigned")
> modified cpumsf_pmu_event_init() to cast the struct perf_event cpu field
> to an unsigned integer before it is compared with nr_cpumask_bits. This
> is broken because the cpu field may be -1 for events which follow a
> process rather than being affine to a particular CPU. When this is the
> case the cast to an unsigned int results in a value equal to ULONG_MAX,
> which is always greater than nr_cpumask_bits so we always fail
> cpumsf_pmu_event_init() and return -ENODEV.
> 
> The check against nr_cpumask_bits seems nonsensical anyway, so this
> patch simply removes it. The cpu field is going to either be 0 or a
> valid CPU number. Comparing it with nr_cpumask_bits is effectively
> checking that it's a valid cpu number, but it seems safe to rely on the
> core perf events code to ensure that's the case.
> 
> The end result is that this fixes use of perf when not constraining
> events to a particular CPU, and fixes the "perf list hw" command which
> fails to list any events without this.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Fixes: c311c797998c ("cpumask: make "nr_cpumask_bits" unsigned")
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: linux-s390@vger.kernel.org
> Cc: stable <stable@vger.kernel.org> # v4.12+
> 
> ---
> This patch is purely speculative, based upon fixing perf on MIPS
> following the offending commit c311c797998c ("cpumask: make
> "nr_cpumask_bits" unsigned") which made the same change to both MIPS &
> s390. I leave it to s390 developers to verify whether this affects them
> in the same way as it affected MIPS thus whether this fix is correct
> there too.

Thanks for the patch, there is indeed an issue with nr_cpumask_bits.
But we already have a slightly different fix for this queued on the
fixes branch of s390/linux:

https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=fixes&id=fc3100d64f0ae383ae8d845989103da06d62763b

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH] s390: Fix perf event init

[Heiko Carstens]

On Wed, Sep 20, 2017 at 08:51:00AM +0200, Martin Schwidefsky wrote:
> Hi Paul,
> 
> On Tue, 19 Sep 2017 22:07:57 -0700
> Paul Burton <paul.burton@imgtec.com> wrote:
> 
> > Commit c311c797998c ("cpumask: make "nr_cpumask_bits" unsigned")
> > modified cpumsf_pmu_event_init() to cast the struct perf_event cpu field
> > to an unsigned integer before it is compared with nr_cpumask_bits. This
> > is broken because the cpu field may be -1 for events which follow a
> > process rather than being affine to a particular CPU. When this is the
> > case the cast to an unsigned int results in a value equal to ULONG_MAX,
> > which is always greater than nr_cpumask_bits so we always fail
> > cpumsf_pmu_event_init() and return -ENODEV.
> > 
> > The check against nr_cpumask_bits seems nonsensical anyway, so this
> > patch simply removes it. The cpu field is going to either be 0 or a

I assume you meant to write "...either be -1..."?

> > valid CPU number. Comparing it with nr_cpumask_bits is effectively
> > checking that it's a valid cpu number, but it seems safe to rely on the
> > core perf events code to ensure that's the case.

Looks like you are right and the nr_cpumask_bits check is not needed. The
sanity check is done at the beginning of perf_event_alloc() and everything
else can rely on a sane cpu number (-1 or within bounds of nr_cpumask_bits).

> Thanks for the patch, there is indeed an issue with nr_cpumask_bits.
> But we already have a slightly different fix for this queued on the
> fixes branch of s390/linux:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=fixes&id=fc3100d64f0ae383ae8d845989103da06d62763b

So we should either use your patch or remove the superfluous check with an
addon patch. Martin's call ;)

Re: [PATCH] s390: Fix perf event init

[Paul Burton]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

Hi Heiko,

On Wednesday, 20 September 2017 05:15:17 PDT Heiko Carstens wrote:
> On Wed, Sep 20, 2017 at 08:51:00AM +0200, Martin Schwidefsky wrote:
> > Hi Paul,
> > 
> > On Tue, 19 Sep 2017 22:07:57 -0700
> > 
> > Paul Burton <paul.burton@imgtec.com> wrote:
> > > Commit c311c797998c ("cpumask: make "nr_cpumask_bits" unsigned")
> > > modified cpumsf_pmu_event_init() to cast the struct perf_event cpu field
> > > to an unsigned integer before it is compared with nr_cpumask_bits. This
> > > is broken because the cpu field may be -1 for events which follow a
> > > process rather than being affine to a particular CPU. When this is the
> > > case the cast to an unsigned int results in a value equal to ULONG_MAX,
> > > which is always greater than nr_cpumask_bits so we always fail
> > > cpumsf_pmu_event_init() and return -ENODEV.
> > > 
> > > The check against nr_cpumask_bits seems nonsensical anyway, so this
> > > patch simply removes it. The cpu field is going to either be 0 or a
> 
> I assume you meant to write "...either be -1..."?

Indeed I did... and I wrote -1 in the MIPS patch which I then copied this 
from, so I'm not sure how it became 0. Oops!

> > > valid CPU number. Comparing it with nr_cpumask_bits is effectively
> > > checking that it's a valid cpu number, but it seems safe to rely on the
> > > core perf events code to ensure that's the case.
> 
> Looks like you are right and the nr_cpumask_bits check is not needed. The
> sanity check is done at the beginning of perf_event_alloc() and everything
> else can rely on a sane cpu number (-1 or within bounds of nr_cpumask_bits).
> > Thanks for the patch, there is indeed an issue with nr_cpumask_bits.
> > But we already have a slightly different fix for this queued on the
> > fixes branch of s390/linux:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=f
> > ixes&id=fc3100d64f0ae383ae8d845989103da06d62763b
> So we should either use your patch or remove the superfluous check with an
> addon patch. Martin's call ;)

Glad the patch is of use either way.

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] s390: Fix perf event init

[Alexey Dobriyan]

On Wed, Sep 20, 2017 at 09:04:38AM -0700, Paul Burton wrote:
> Hi Heiko,
> 
> On Wednesday, 20 September 2017 05:15:17 PDT Heiko Carstens wrote:
> > On Wed, Sep 20, 2017 at 08:51:00AM +0200, Martin Schwidefsky wrote:
> > > Hi Paul,
> > > 
> > > On Tue, 19 Sep 2017 22:07:57 -0700
> > > 
> > > Paul Burton <paul.burton@imgtec.com> wrote:
> > > > Commit c311c797998c ("cpumask: make "nr_cpumask_bits" unsigned")
> > > > modified cpumsf_pmu_event_init() to cast the struct perf_event cpu field
> > > > to an unsigned integer before it is compared with nr_cpumask_bits. This
> > > > is broken because the cpu field may be -1 for events which follow a
> > > > process rather than being affine to a particular CPU. When this is the
> > > > case the cast to an unsigned int results in a value equal to ULONG_MAX,
> > > > which is always greater than nr_cpumask_bits so we always fail
> > > > cpumsf_pmu_event_init() and return -ENODEV.
> > > > 
> > > > The check against nr_cpumask_bits seems nonsensical anyway, so this
> > > > patch simply removes it. The cpu field is going to either be 0 or a
> > 
> > I assume you meant to write "...either be -1..."?
> 
> Indeed I did... and I wrote -1 in the MIPS patch which I then copied this 
> from, so I'm not sure how it became 0. Oops!
> 
> > > > valid CPU number. Comparing it with nr_cpumask_bits is effectively
> > > > checking that it's a valid cpu number, but it seems safe to rely on the
> > > > core perf events code to ensure that's the case.
> > 
> > Looks like you are right and the nr_cpumask_bits check is not needed. The
> > sanity check is done at the beginning of perf_event_alloc() and everything
> > else can rely on a sane cpu number (-1 or within bounds of nr_cpumask_bits).
> > > Thanks for the patch, there is indeed an issue with nr_cpumask_bits.
> > > But we already have a slightly different fix for this queued on the
> > > fixes branch of s390/linux:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=f
> > > ixes&id=fc3100d64f0ae383ae8d845989103da06d62763b
> > So we should either use your patch or remove the superfluous check with an
> > addon patch. Martin's call ;)
> 
> Glad the patch is of use either way.

First, sorry for the breakage.

Second, if you write code like this:

	if (event->cpu >= 0) {
		if ((unsigned int)event->cpu >= nr_cpumask_bits)
			return -ENODEV;

then cast is unnecessary as comparison is unsigned and non-negative
values will remain themselves.