2018-07-09 00:41:39

by Okash Khawaja

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

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 <[email protected]>

---
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,



2018-07-09 17:35:56

by Martin KaFai Lau

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

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 <[email protected]>
>
> ---
> 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,
>

2018-07-09 18:34:40

by Martin KaFai Lau

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

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 <[email protected]>
>
> ---
> 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,
>

2018-07-10 08:22:16

by Daniel Borkmann

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

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 <[email protected]>
>>
>> ---
>> 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,
>>


2018-07-10 17:01:47

by David Laight

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

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 <[email protected]>
> >
> > ---
> > 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


2018-07-10 18:18:52

by Okash Khawaja

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

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 <[email protected]>
> >>
> >> ---
> >> 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?

Thanks,
Okash

2018-07-10 18:39:23

by Martin KaFai Lau

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

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 <[email protected]>
> > >
> > > ---
> > > 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.

2018-07-10 18:42:12

by Daniel Borkmann

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

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

2018-07-10 20:03:03

by Daniel Borkmann

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

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 <[email protected]>
>>>>
>>>> ---
>>>> 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.