[PATCH bpf 0/1] bpf: btf: Fix endianness of bitfields

[PATCH bpf 0/1] bpf: btf: Fix endianness of bitfields

[Okash Khawaja]

Hi,

This patch fixes endianness bug in btf_int_bits_seq_show(). Jakub
Kicinski pointed out in a separate patch review for bpftool ("bpf: btf:
add btf print functionality") that parsing of bitfield might not work
on big endian machine. Similar parsing is performed in
btf_int_bits_seq_show() and this patch fixes it.

Thanks,
Okash

[PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[Okash Khawaja]

[-- Attachment #1: fix-btf-bitfields.patch --]
[-- Type: text/plain, Size: 2568 bytes --]

When extracting bitfield from a number, btf_int_bits_seq_show() builds
a mask and accesses least significant byte of the number in a way
specific to little-endian. This patch fixes that by checking endianness
of the machine and then shifting left and right the unneeded bits.

Thanks to Martin Lau for the help in navigating potential pitfalls when
dealing with endianess and for the final solution.

Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
Signed-off-by: Okash Khawaja <osk@fb.com>

---
 kernel/bpf/btf.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -162,6 +162,8 @@
 #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
 #define BITS_ROUNDUP_BYTES(bits) \
 	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+const int one = 1;
+#define is_big_endian() ((*(char *)&one) == 0)
 
 #define BTF_INFO_MASK 0x0f00ffff
 #define BTF_INT_MASK 0x0fffffff
@@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
 				  void *data, u8 bits_offset,
 				  struct seq_file *m)
 {
+	u8 left_shift_bits, right_shift_bits;
 	u32 int_data = btf_type_int(t);
 	u16 nr_bits = BTF_INT_BITS(int_data);
 	u16 total_bits_offset;
 	u16 nr_copy_bytes;
 	u16 nr_copy_bits;
-	u8 nr_upper_bits;
-	union {
-		u64 u64_num;
-		u8  u8_nums[8];
-	} print_num;
+	u64 print_num;
 
 	total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
 	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
@@ -1008,21 +1007,20 @@ static void btf_int_bits_seq_show(const
 	nr_copy_bits = nr_bits + bits_offset;
 	nr_copy_bytes = BITS_ROUNDUP_BYTES(nr_copy_bits);
 
-	print_num.u64_num = 0;
-	memcpy(&print_num.u64_num, data, nr_copy_bytes);
-
-	/* Ditch the higher order bits */
-	nr_upper_bits = BITS_PER_BYTE_MASKED(nr_copy_bits);
-	if (nr_upper_bits) {
-		/* We need to mask out some bits of the upper byte. */
-		u8 mask = (1 << nr_upper_bits) - 1;
-
-		print_num.u8_nums[nr_copy_bytes - 1] &= mask;
+	print_num = 0;
+	memcpy(&print_num, data, nr_copy_bytes);
+	if (is_big_endian()) {
+		left_shift_bits = bits_offset;
+		right_shift_bits = BITS_PER_U64 - nr_bits;
+	} else {
+		left_shift_bits = BITS_PER_U64 - nr_copy_bits;
+		right_shift_bits = BITS_PER_U64 - nr_bits;
 	}
 
-	print_num.u64_num >>= bits_offset;
+	print_num <<= left_shift_bits;
+	print_num >>= right_shift_bits;
 
-	seq_printf(m, "0x%llx", print_num.u64_num);
+	seq_printf(m, "0x%llx", print_num);
 }
 
 static void btf_int_seq_show(const struct btf *btf, const struct btf_type *t,

Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[Martin KaFai Lau]

On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> When extracting bitfield from a number, btf_int_bits_seq_show() builds
> a mask and accesses least significant byte of the number in a way
> specific to little-endian. This patch fixes that by checking endianness
> of the machine and then shifting left and right the unneeded bits.
> 
> Thanks to Martin Lau for the help in navigating potential pitfalls when
> dealing with endianess and for the final solution.
> 
> Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> Signed-off-by: Okash Khawaja <osk@fb.com>
> 
> ---
>  kernel/bpf/btf.c |   32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -162,6 +162,8 @@
>  #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
>  #define BITS_ROUNDUP_BYTES(bits) \
>  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> +const int one = 1;
> +#define is_big_endian() ((*(char *)&one) == 0)
Can the __BIG_ENDIAN be reused here?

>  
>  #define BTF_INFO_MASK 0x0f00ffff
>  #define BTF_INT_MASK 0x0fffffff
> @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
>  				  void *data, u8 bits_offset,
>  				  struct seq_file *m)
>  {
> +	u8 left_shift_bits, right_shift_bits;
>  	u32 int_data = btf_type_int(t);
>  	u16 nr_bits = BTF_INT_BITS(int_data);
>  	u16 total_bits_offset;
>  	u16 nr_copy_bytes;
>  	u16 nr_copy_bits;
> -	u8 nr_upper_bits;
> -	union {
> -		u64 u64_num;
> -		u8  u8_nums[8];
> -	} print_num;
> +	u64 print_num;
>  
>  	total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
>  	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
> @@ -1008,21 +1007,20 @@ static void btf_int_bits_seq_show(const
>  	nr_copy_bits = nr_bits + bits_offset;
>  	nr_copy_bytes = BITS_ROUNDUP_BYTES(nr_copy_bits);
>  
> -	print_num.u64_num = 0;
> -	memcpy(&print_num.u64_num, data, nr_copy_bytes);
> -
> -	/* Ditch the higher order bits */
> -	nr_upper_bits = BITS_PER_BYTE_MASKED(nr_copy_bits);
> -	if (nr_upper_bits) {
> -		/* We need to mask out some bits of the upper byte. */
> -		u8 mask = (1 << nr_upper_bits) - 1;
> -
> -		print_num.u8_nums[nr_copy_bytes - 1] &= mask;
> +	print_num = 0;
> +	memcpy(&print_num, data, nr_copy_bytes);
> +	if (is_big_endian()) {
> +		left_shift_bits = bits_offset;
> +		right_shift_bits = BITS_PER_U64 - nr_bits;
> +	} else {
> +		left_shift_bits = BITS_PER_U64 - nr_copy_bits;
> +		right_shift_bits = BITS_PER_U64 - nr_bits;
>  	}
>  
> -	print_num.u64_num >>= bits_offset;
> +	print_num <<= left_shift_bits;
> +	print_num >>= right_shift_bits;
>  
> -	seq_printf(m, "0x%llx", print_num.u64_num);
> +	seq_printf(m, "0x%llx", print_num);
>  }
>  
>  static void btf_int_seq_show(const struct btf *btf, const struct btf_type *t,
> 

Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[Martin KaFai Lau]

On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> When extracting bitfield from a number, btf_int_bits_seq_show() builds
> a mask and accesses least significant byte of the number in a way
> specific to little-endian. This patch fixes that by checking endianness
> of the machine and then shifting left and right the unneeded bits.
> 
> Thanks to Martin Lau for the help in navigating potential pitfalls when
> dealing with endianess and for the final solution.
> 
> Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> Signed-off-by: Okash Khawaja <osk@fb.com>
> 
> ---
>  kernel/bpf/btf.c |   32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -162,6 +162,8 @@
>  #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
>  #define BITS_ROUNDUP_BYTES(bits) \
>  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> +const int one = 1;
> +#define is_big_endian() ((*(char *)&one) == 0)
>  
>  #define BTF_INFO_MASK 0x0f00ffff
>  #define BTF_INT_MASK 0x0fffffff
> @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
>  				  void *data, u8 bits_offset,
>  				  struct seq_file *m)
>  {
> +	u8 left_shift_bits, right_shift_bits;
Nit.
Although only max 64 bit int is allowed now (ensured by btf_int_check_meta),
it is better to use u16 such that it will be consistent to BTF_INT_BITS.

>  	u32 int_data = btf_type_int(t);
>  	u16 nr_bits = BTF_INT_BITS(int_data);
>  	u16 total_bits_offset;
>  	u16 nr_copy_bytes;
>  	u16 nr_copy_bits;
> -	u8 nr_upper_bits;
> -	union {
> -		u64 u64_num;
> -		u8  u8_nums[8];
> -	} print_num;
> +	u64 print_num;
>  
>  	total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
>  	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
> @@ -1008,21 +1007,20 @@ static void btf_int_bits_seq_show(const
>  	nr_copy_bits = nr_bits + bits_offset;
>  	nr_copy_bytes = BITS_ROUNDUP_BYTES(nr_copy_bits);
>  
> -	print_num.u64_num = 0;
> -	memcpy(&print_num.u64_num, data, nr_copy_bytes);
> -
> -	/* Ditch the higher order bits */
> -	nr_upper_bits = BITS_PER_BYTE_MASKED(nr_copy_bits);
> -	if (nr_upper_bits) {
> -		/* We need to mask out some bits of the upper byte. */
> -		u8 mask = (1 << nr_upper_bits) - 1;
> -
> -		print_num.u8_nums[nr_copy_bytes - 1] &= mask;
> +	print_num = 0;
> +	memcpy(&print_num, data, nr_copy_bytes);
> +	if (is_big_endian()) {
> +		left_shift_bits = bits_offset;
> +		right_shift_bits = BITS_PER_U64 - nr_bits;
> +	} else {
> +		left_shift_bits = BITS_PER_U64 - nr_copy_bits;
> +		right_shift_bits = BITS_PER_U64 - nr_bits;
Nit.
right_shift_bits is the same for both cases.  Lets simplify it.

>  	}
>  
> -	print_num.u64_num >>= bits_offset;
> +	print_num <<= left_shift_bits;
> +	print_num >>= right_shift_bits;
>  
> -	seq_printf(m, "0x%llx", print_num.u64_num);
> +	seq_printf(m, "0x%llx", print_num);
>  }
>  
>  static void btf_int_seq_show(const struct btf *btf, const struct btf_type *t,
> 

Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[Daniel Borkmann]

On 07/09/2018 08:32 PM, Martin KaFai Lau wrote:
> On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
>> When extracting bitfield from a number, btf_int_bits_seq_show() builds
>> a mask and accesses least significant byte of the number in a way
>> specific to little-endian. This patch fixes that by checking endianness
>> of the machine and then shifting left and right the unneeded bits.
>>
>> Thanks to Martin Lau for the help in navigating potential pitfalls when
>> dealing with endianess and for the final solution.
>>
>> Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
>> Signed-off-by: Okash Khawaja <osk@fb.com>
>>
>> ---
>>  kernel/bpf/btf.c |   32 +++++++++++++++-----------------
>>  1 file changed, 15 insertions(+), 17 deletions(-)
>>
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -162,6 +162,8 @@
>>  #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
>>  #define BITS_ROUNDUP_BYTES(bits) \
>>  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
>> +const int one = 1;
>> +#define is_big_endian() ((*(char *)&one) == 0)

Also here, in the kernel archs provide proper definitions.

>>  #define BTF_INFO_MASK 0x0f00ffff
>>  #define BTF_INT_MASK 0x0fffffff
>> @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
>>  				  void *data, u8 bits_offset,
>>  				  struct seq_file *m)
>>  {
>> +	u8 left_shift_bits, right_shift_bits;
> Nit.
> Although only max 64 bit int is allowed now (ensured by btf_int_check_meta),
> it is better to use u16 such that it will be consistent to BTF_INT_BITS.
> 
>>  	u32 int_data = btf_type_int(t);
>>  	u16 nr_bits = BTF_INT_BITS(int_data);
>>  	u16 total_bits_offset;
>>  	u16 nr_copy_bytes;
>>  	u16 nr_copy_bits;
>> -	u8 nr_upper_bits;
>> -	union {
>> -		u64 u64_num;
>> -		u8  u8_nums[8];
>> -	} print_num;
>> +	u64 print_num;
>>  
>>  	total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
>>  	data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
>> @@ -1008,21 +1007,20 @@ static void btf_int_bits_seq_show(const
>>  	nr_copy_bits = nr_bits + bits_offset;
>>  	nr_copy_bytes = BITS_ROUNDUP_BYTES(nr_copy_bits);
>>  
>> -	print_num.u64_num = 0;
>> -	memcpy(&print_num.u64_num, data, nr_copy_bytes);
>> -
>> -	/* Ditch the higher order bits */
>> -	nr_upper_bits = BITS_PER_BYTE_MASKED(nr_copy_bits);
>> -	if (nr_upper_bits) {
>> -		/* We need to mask out some bits of the upper byte. */
>> -		u8 mask = (1 << nr_upper_bits) - 1;
>> -
>> -		print_num.u8_nums[nr_copy_bytes - 1] &= mask;
>> +	print_num = 0;
>> +	memcpy(&print_num, data, nr_copy_bytes);
>> +	if (is_big_endian()) {
>> +		left_shift_bits = bits_offset;
>> +		right_shift_bits = BITS_PER_U64 - nr_bits;
>> +	} else {
>> +		left_shift_bits = BITS_PER_U64 - nr_copy_bits;
>> +		right_shift_bits = BITS_PER_U64 - nr_bits;
> Nit.
> right_shift_bits is the same for both cases.  Lets simplify it.
> 
>>  	}
>>  
>> -	print_num.u64_num >>= bits_offset;
>> +	print_num <<= left_shift_bits;
>> +	print_num >>= right_shift_bits;
>>  
>> -	seq_printf(m, "0x%llx", print_num.u64_num);
>> +	seq_printf(m, "0x%llx", print_num);
>>  }
>>  
>>  static void btf_int_seq_show(const struct btf *btf, const struct btf_type *t,
>>

RE: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[David Laight]

From: Martin KaFai Lau
> Sent: 09 July 2018 19:33
> On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> > When extracting bitfield from a number, btf_int_bits_seq_show() builds
> > a mask and accesses least significant byte of the number in a way
> > specific to little-endian. This patch fixes that by checking endianness
> > of the machine and then shifting left and right the unneeded bits.
> >
> > Thanks to Martin Lau for the help in navigating potential pitfalls when
> > dealing with endianess and for the final solution.
> >
> > Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> > Signed-off-by: Okash Khawaja <osk@fb.com>
> >
> > ---
> >  kernel/bpf/btf.c |   32 +++++++++++++++-----------------
> >  1 file changed, 15 insertions(+), 17 deletions(-)
> >
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -162,6 +162,8 @@
> >  #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> >  #define BITS_ROUNDUP_BYTES(bits) \
> >  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> > +const int one = 1;
> > +#define is_big_endian() ((*(char *)&one) == 0)
> >
> >  #define BTF_INFO_MASK 0x0f00ffff
> >  #define BTF_INT_MASK 0x0fffffff
> > @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
> >  				  void *data, u8 bits_offset,
> >  				  struct seq_file *m)
> >  {
> > +	u8 left_shift_bits, right_shift_bits;
> Nit.
> Although only max 64 bit int is allowed now (ensured by btf_int_check_meta),
> it is better to use u16 such that it will be consistent to BTF_INT_BITS.

Double-nit.

Use 'int' or 'unsigned int'.
Sub-word arithmetic will require extra instructions on almost everything
except x86.

	David

Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[Martin KaFai Lau]

On Tue, Jul 10, 2018 at 04:35:04PM +0000, David Laight wrote:
> From: Martin KaFai Lau
> > Sent: 09 July 2018 19:33
> > On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> > > When extracting bitfield from a number, btf_int_bits_seq_show() builds
> > > a mask and accesses least significant byte of the number in a way
> > > specific to little-endian. This patch fixes that by checking endianness
> > > of the machine and then shifting left and right the unneeded bits.
> > >
> > > Thanks to Martin Lau for the help in navigating potential pitfalls when
> > > dealing with endianess and for the final solution.
> > >
> > > Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> > > Signed-off-by: Okash Khawaja <osk@fb.com>
> > >
> > > ---
> > >  kernel/bpf/btf.c |   32 +++++++++++++++-----------------
> > >  1 file changed, 15 insertions(+), 17 deletions(-)
> > >
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -162,6 +162,8 @@
> > >  #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> > >  #define BITS_ROUNDUP_BYTES(bits) \
> > >  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> > > +const int one = 1;
> > > +#define is_big_endian() ((*(char *)&one) == 0)
> > >
> > >  #define BTF_INFO_MASK 0x0f00ffff
> > >  #define BTF_INT_MASK 0x0fffffff
> > > @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
> > >  				  void *data, u8 bits_offset,
> > >  				  struct seq_file *m)
> > >  {
> > > +	u8 left_shift_bits, right_shift_bits;
> > Nit.
> > Although only max 64 bit int is allowed now (ensured by btf_int_check_meta),
> > it is better to use u16 such that it will be consistent to BTF_INT_BITS.
> 
> Double-nit.
> 
> Use 'int' or 'unsigned int'.
> Sub-word arithmetic will require extra instructions on almost everything
> except x86.
I would prefer to keep it as u16 which is the max width that is allowed for
this field in the wire format.  Keeping the usage consistent can avoid
accidentally incorrect offsetting or writing wrong data out in other
cases.

Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[Daniel Borkmann]

On 07/10/2018 07:18 PM, Martin KaFai Lau wrote:
[...]
> I would prefer to keep it as u16 which is the max width that is allowed for
> this field in the wire format.  Keeping the usage consistent can avoid
> accidentally incorrect offsetting or writing wrong data out in other
> cases.

+1

Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian

[Daniel Borkmann]

On 07/10/2018 07:49 PM, Okash Khawaja wrote:
> On Tue, Jul 10, 2018 at 10:21:02AM +0200, Daniel Borkmann wrote:
>> On 07/09/2018 08:32 PM, Martin KaFai Lau wrote:
>>> On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
>>>> When extracting bitfield from a number, btf_int_bits_seq_show() builds
>>>> a mask and accesses least significant byte of the number in a way
>>>> specific to little-endian. This patch fixes that by checking endianness
>>>> of the machine and then shifting left and right the unneeded bits.
>>>>
>>>> Thanks to Martin Lau for the help in navigating potential pitfalls when
>>>> dealing with endianess and for the final solution.
>>>>
>>>> Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
>>>> Signed-off-by: Okash Khawaja <osk@fb.com>
>>>>
>>>> ---
>>>>  kernel/bpf/btf.c |   32 +++++++++++++++-----------------
>>>>  1 file changed, 15 insertions(+), 17 deletions(-)
>>>>
>>>> --- a/kernel/bpf/btf.c
>>>> +++ b/kernel/bpf/btf.c
>>>> @@ -162,6 +162,8 @@
>>>>  #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
>>>>  #define BITS_ROUNDUP_BYTES(bits) \
>>>>  	(BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
>>>> +const int one = 1;
>>>> +#define is_big_endian() ((*(char *)&one) == 0)
>>
>> Also here, in the kernel archs provide proper definitions.
> Is this the __BIG_ENDIAN #define or are there better ways to check that?

Given this deals with bitfields, should be __{BIG,LITTLE}_ENDIAN_BITFIELD.