[PATCH] perf/core: Fix the mask in perf_output_sample_regs

[PATCH] perf/core: Fix the mask in perf_output_sample_regs

[Madhavan Srinivasan]

When decoding the perf_regs mask in perf_output_sample_regs(),
we loop through the mask using find_first_bit and find_next_bit functions.
While the exisitng code works fine in most of the case,
the logic is broken for 32bit kernel (Big Endian).
When reading u64 mask using (u32 *)(&val)[0], find_*_bit() assumes it gets
lower 32bits of u64 but instead gets upper 32bits which is wrong.
Proposed fix is to swap the words of the u64 to handle this case.
This is _not_ endianness swap.

Suggested-by: Yury Norov <ynorov@caviumnetworks.com>
Cc: Yury Norov <ynorov@caviumnetworks.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
 include/linux/bitmap.h |  2 ++
 kernel/events/core.c   |  4 +++-
 lib/bitmap.c           | 19 +++++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index e9b0b9ab07e5..d95b422db183 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -188,6 +188,8 @@ extern int bitmap_print_to_pagebuf(bool list, char *buf,
 #define small_const_nbits(nbits) \
 	(__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG)
 
+extern void bitmap_from_u64(unsigned long *dst, u64 mask);
+
 static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
 {
 	if (small_const_nbits(nbits))
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9c51ec3f0f44..613fec95ea4c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5205,8 +5205,10 @@ perf_output_sample_regs(struct perf_output_handle *handle,
 			struct pt_regs *regs, u64 mask)
 {
 	int bit;
+	DECLARE_BITMAP(_mask, 64);
 
-	for_each_set_bit(bit, (const unsigned long *) &mask,
+	bitmap_from_u64(_mask, mask);
+	for_each_set_bit(bit, _mask,
 			 sizeof(mask) * BITS_PER_BYTE) {
 		u64 val;
 
diff --git a/lib/bitmap.c b/lib/bitmap.c
index c66da508cbf7..522f1b4c6078 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1170,3 +1170,22 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
 }
 EXPORT_SYMBOL(bitmap_copy_le);
 #endif
+
+/*
+ * bitmap_from_u64 - Check and swap words within u64.
+ *  @mask: source bitmap
+ *  @dst:  destination bitmap
+ *
+ * In 32bit Big Endian kernel, when using (u32 *)(&val)[*]
+ * to read u64 mask, we will get wrong word.
+ * That is "(u32 *)(&val)[0]" gets upper 32 bits,
+ * but expected could be lower 32bits of u64.
+ */
+void bitmap_from_u64(unsigned long *dst, u64 mask)
+{
+	dst[0] = mask & ULONG_MAX;
+
+	if (sizeof(mask) > sizeof(unsigned long))
+		dst[1] = mask >> 32;
+}
+EXPORT_SYMBOL(bitmap_from_u64);
-- 
1.9.1

Re: [PATCH] perf/core: Fix the mask in perf_output_sample_regs

[Yury Norov]

On Sun, Jul 03, 2016 at 11:31:58PM +0530, Madhavan Srinivasan wrote:
> When decoding the perf_regs mask in perf_output_sample_regs(),
> we loop through the mask using find_first_bit and find_next_bit functions.
> While the exisitng code works fine in most of the case,
> the logic is broken for 32bit kernel (Big Endian).
> When reading u64 mask using (u32 *)(&val)[0], find_*_bit() assumes it gets
> lower 32bits of u64 but instead gets upper 32bits which is wrong.
> Proposed fix is to swap the words of the u64 to handle this case.
> This is _not_ endianness swap.


In fact, it's broken for 32-bit LE as well if mask is zero and next
word on stack is not zero. The rest is OK.

Reviewed-by: Yury Norov <ynorov@caviumnetworks.com>

> 
> Suggested-by: Yury Norov <ynorov@caviumnetworks.com>
> Cc: Yury Norov <ynorov@caviumnetworks.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
>  include/linux/bitmap.h |  2 ++
>  kernel/events/core.c   |  4 +++-
>  lib/bitmap.c           | 19 +++++++++++++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index e9b0b9ab07e5..d95b422db183 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -188,6 +188,8 @@ extern int bitmap_print_to_pagebuf(bool list, char *buf,
>  #define small_const_nbits(nbits) \
>  	(__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG)
>  
> +extern void bitmap_from_u64(unsigned long *dst, u64 mask);
> +
>  static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
>  {
>  	if (small_const_nbits(nbits))
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9c51ec3f0f44..613fec95ea4c 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5205,8 +5205,10 @@ perf_output_sample_regs(struct perf_output_handle *handle,
>  			struct pt_regs *regs, u64 mask)
>  {
>  	int bit;
> +	DECLARE_BITMAP(_mask, 64);
>  
> -	for_each_set_bit(bit, (const unsigned long *) &mask,
> +	bitmap_from_u64(_mask, mask);
> +	for_each_set_bit(bit, _mask,
>  			 sizeof(mask) * BITS_PER_BYTE) {
>  		u64 val;
>  
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index c66da508cbf7..522f1b4c6078 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -1170,3 +1170,22 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
>  }
>  EXPORT_SYMBOL(bitmap_copy_le);
>  #endif
> +
> +/*
> + * bitmap_from_u64 - Check and swap words within u64.
> + *  @mask: source bitmap
> + *  @dst:  destination bitmap
> + *
> + * In 32bit Big Endian kernel, when using (u32 *)(&val)[*]
> + * to read u64 mask, we will get wrong word.
> + * That is "(u32 *)(&val)[0]" gets upper 32 bits,
> + * but expected could be lower 32bits of u64.
> + */
> +void bitmap_from_u64(unsigned long *dst, u64 mask)
> +{
> +	dst[0] = mask & ULONG_MAX;
> +
> +	if (sizeof(mask) > sizeof(unsigned long))
> +		dst[1] = mask >> 32;
> +}
> +EXPORT_SYMBOL(bitmap_from_u64);
> -- 
> 1.9.1

Re: [PATCH] perf/core: Fix the mask in perf_output_sample_regs

[Arnaldo Carvalho de Melo]

Em Mon, Jul 04, 2016 at 10:19:06AM +0300, Yury Norov escreveu:
> On Sun, Jul 03, 2016 at 11:31:58PM +0530, Madhavan Srinivasan wrote:
> > When decoding the perf_regs mask in perf_output_sample_regs(),
> > we loop through the mask using find_first_bit and find_next_bit functions.
> > While the exisitng code works fine in most of the case,
> > the logic is broken for 32bit kernel (Big Endian).
> > When reading u64 mask using (u32 *)(&val)[0], find_*_bit() assumes it gets
> > lower 32bits of u64 but instead gets upper 32bits which is wrong.
> > Proposed fix is to swap the words of the u64 to handle this case.
> > This is _not_ endianness swap.
> 
> 
> In fact, it's broken for 32-bit LE as well if mask is zero and next
> word on stack is not zero. The rest is OK.
> 
> Reviewed-by: Yury Norov <ynorov@caviumnetworks.com>

Waiting a bit for more acks, as IIRC several other people had comments
on this patch, ok?

- Arnaldo
 
> > 
> > Suggested-by: Yury Norov <ynorov@caviumnetworks.com>
> > Cc: Yury Norov <ynorov@caviumnetworks.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> > ---
> >  include/linux/bitmap.h |  2 ++
> >  kernel/events/core.c   |  4 +++-
> >  lib/bitmap.c           | 19 +++++++++++++++++++
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index e9b0b9ab07e5..d95b422db183 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -188,6 +188,8 @@ extern int bitmap_print_to_pagebuf(bool list, char *buf,
> >  #define small_const_nbits(nbits) \
> >  	(__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG)
> >  
> > +extern void bitmap_from_u64(unsigned long *dst, u64 mask);
> > +
> >  static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
> >  {
> >  	if (small_const_nbits(nbits))
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 9c51ec3f0f44..613fec95ea4c 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -5205,8 +5205,10 @@ perf_output_sample_regs(struct perf_output_handle *handle,
> >  			struct pt_regs *regs, u64 mask)
> >  {
> >  	int bit;
> > +	DECLARE_BITMAP(_mask, 64);
> >  
> > -	for_each_set_bit(bit, (const unsigned long *) &mask,
> > +	bitmap_from_u64(_mask, mask);
> > +	for_each_set_bit(bit, _mask,
> >  			 sizeof(mask) * BITS_PER_BYTE) {
> >  		u64 val;
> >  
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index c66da508cbf7..522f1b4c6078 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -1170,3 +1170,22 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
> >  }
> >  EXPORT_SYMBOL(bitmap_copy_le);
> >  #endif
> > +
> > +/*
> > + * bitmap_from_u64 - Check and swap words within u64.
> > + *  @mask: source bitmap
> > + *  @dst:  destination bitmap
> > + *
> > + * In 32bit Big Endian kernel, when using (u32 *)(&val)[*]
> > + * to read u64 mask, we will get wrong word.
> > + * That is "(u32 *)(&val)[0]" gets upper 32 bits,
> > + * but expected could be lower 32bits of u64.
> > + */
> > +void bitmap_from_u64(unsigned long *dst, u64 mask)
> > +{
> > +	dst[0] = mask & ULONG_MAX;
> > +
> > +	if (sizeof(mask) > sizeof(unsigned long))
> > +		dst[1] = mask >> 32;
> > +}
> > +EXPORT_SYMBOL(bitmap_from_u64);
> > -- 
> > 1.9.1

Re: [PATCH] perf/core: Fix the mask in perf_output_sample_regs

[Peter Zijlstra]

Sorry, found it in my inbox while clearing out backlog..

On Sun, Jul 03, 2016 at 11:31:58PM +0530, Madhavan Srinivasan wrote:
> When decoding the perf_regs mask in perf_output_sample_regs(),
> we loop through the mask using find_first_bit and find_next_bit functions.
> While the exisitng code works fine in most of the case,
> the logic is broken for 32bit kernel (Big Endian).
> When reading u64 mask using (u32 *)(&val)[0], find_*_bit() assumes it gets
> lower 32bits of u64 but instead gets upper 32bits which is wrong.
> Proposed fix is to swap the words of the u64 to handle this case.

> This is _not_ endianness swap.

But it looks an awful lot like it..

> +++ b/kernel/events/core.c
> @@ -5205,8 +5205,10 @@ perf_output_sample_regs(struct perf_output_handle *handle,
>  			struct pt_regs *regs, u64 mask)
>  {
>  	int bit;
> +	DECLARE_BITMAP(_mask, 64);
>  
> -	for_each_set_bit(bit, (const unsigned long *) &mask,
> +	bitmap_from_u64(_mask, mask);
> +	for_each_set_bit(bit, _mask,
>  			 sizeof(mask) * BITS_PER_BYTE) {
>  		u64 val;

> +++ b/lib/bitmap.c

> +void bitmap_from_u64(unsigned long *dst, u64 mask)
> +{
> +	dst[0] = mask & ULONG_MAX;
> +
> +	if (sizeof(mask) > sizeof(unsigned long))
> +		dst[1] = mask >> 32;
> +}
> +EXPORT_SYMBOL(bitmap_from_u64);

Looks small enough for an inline.

Alternatively you can go all the way and add bitmap_from_u64array(), but
that seems massive overkill.

Tedious stuff.. I can't come up with anything prettier :/

Re: [PATCH] perf/core: Fix the mask in perf_output_sample_regs

[Madhavan Srinivasan]

On Thursday 11 August 2016 05:57 PM, Peter Zijlstra wrote:
> Sorry, found it in my inbox while clearing out backlog..
>
> On Sun, Jul 03, 2016 at 11:31:58PM +0530, Madhavan Srinivasan wrote:
>> When decoding the perf_regs mask in perf_output_sample_regs(),
>> we loop through the mask using find_first_bit and find_next_bit functions.
>> While the exisitng code works fine in most of the case,
>> the logic is broken for 32bit kernel (Big Endian).
>> When reading u64 mask using (u32 *)(&val)[0], find_*_bit() assumes it gets
>> lower 32bits of u64 but instead gets upper 32bits which is wrong.
>> Proposed fix is to swap the words of the u64 to handle this case.
>> This is _not_ endianness swap.
> But it looks an awful lot like it..
Hit this issue when testing my perf_arch_regs patchset. Yep exactly
the reason for adding that comment in the commit message.


>
>> +++ b/kernel/events/core.c
>> @@ -5205,8 +5205,10 @@ perf_output_sample_regs(struct perf_output_handle *handle,
>>   			struct pt_regs *regs, u64 mask)
>>   {
>>   	int bit;
>> +	DECLARE_BITMAP(_mask, 64);
>>   
>> -	for_each_set_bit(bit, (const unsigned long *) &mask,
>> +	bitmap_from_u64(_mask, mask);
>> +	for_each_set_bit(bit, _mask,
>>   			 sizeof(mask) * BITS_PER_BYTE) {
>>   		u64 val;
>> +++ b/lib/bitmap.c
>> +void bitmap_from_u64(unsigned long *dst, u64 mask)
>> +{
>> +	dst[0] = mask & ULONG_MAX;
>> +
>> +	if (sizeof(mask) > sizeof(unsigned long))
>> +		dst[1] = mask >> 32;
>> +}
>> +EXPORT_SYMBOL(bitmap_from_u64);
> Looks small enough for an inline.
>
> Alternatively you can go all the way and add bitmap_from_u64array(), but
> that seems massive overkill.

Ok will make it inline and resend.

Maddy

>
> Tedious stuff.. I can't come up with anything prettier :/
>