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@xxxxxxxxxx> 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 ;)