Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf-next] libbpf: clarify/fix unaligned data issues for btf typed dump
@ 2021-07-16 21:43 Alan Maguire
  2021-07-16 22:05 ` Andrii Nakryiko
  0 siblings, 1 reply; 2+ messages in thread
From: Alan Maguire @ 2021-07-16 21:43 UTC (permalink / raw)
  To: ast, daniel, andrii
  Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, morbo,
	shuah, bpf, netdev, linux-kselftest, linux-kernel, Alan Maguire

If data is packed, data structures can store it outside of usual
boundaries.  For example a 4-byte int can be stored on a unaligned
boundary in a case like this:

struct s {
	char f1;
	int f2;
} __attribute((packed));

...the int is stored at an offset of one byte.  Some platforms have
problems dereferencing data that is not aligned with its size, and
code exists to handle most cases of this for BTF typed data display.
However pointer display was missed, and a simple macro to test if
"data_is_unaligned(data, data_sz)" would help clarify this code.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/btf_dump.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 929cf93..9dfe9c1 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1654,6 +1654,8 @@ static int btf_dump_base_type_check_zero(struct btf_dump *d,
 	return 0;
 }
 
+#define data_is_unaligned(data, data_sz)	(((uintptr_t)data) % data_sz)
+
 static int btf_dump_int_data(struct btf_dump *d,
 			     const struct btf_type *t,
 			     __u32 type_id,
@@ -1672,7 +1674,7 @@ static int btf_dump_int_data(struct btf_dump *d,
 	/* handle packed int data - accesses of integers not aligned on
 	 * int boundaries can cause problems on some platforms.
 	 */
-	if (((uintptr_t)data) % sz)
+	if (data_is_unaligned(data, sz))
 		return btf_dump_bitfield_data(d, t, data, 0, 0);
 
 	switch (sz) {
@@ -1739,7 +1741,7 @@ static int btf_dump_float_data(struct btf_dump *d,
 	int sz = t->size;
 
 	/* handle unaligned data; copy to local union */
-	if (((uintptr_t)data) % sz) {
+	if (data_is_unaligned(data, sz)) {
 		memcpy(&fl, data, sz);
 		flp = &fl;
 	}
@@ -1897,7 +1899,10 @@ static int btf_dump_ptr_data(struct btf_dump *d,
 			      __u32 id,
 			      const void *data)
 {
-	btf_dump_type_values(d, "%p", *(void **)data);
+	void *ptrval;
+
+	memcpy(&ptrval, data, d->ptr_sz);
+	btf_dump_type_values(d, "%p", ptrval);
 	return 0;
 }
 
@@ -1910,7 +1915,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d,
 	int sz = t->size;
 
 	/* handle unaligned enum value */
-	if (((uintptr_t)data) % sz) {
+	if (data_is_unaligned(data, sz)) {
 		*value = (__s64)btf_dump_bitfield_get_data(d, t, data, 0, 0);
 		return 0;
 	}
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH bpf-next] libbpf: clarify/fix unaligned data issues for btf typed dump
  2021-07-16 21:43 [PATCH bpf-next] libbpf: clarify/fix unaligned data issues for btf typed dump Alan Maguire
@ 2021-07-16 22:05 ` Andrii Nakryiko
  0 siblings, 0 replies; 2+ messages in thread
From: Andrii Nakryiko @ 2021-07-16 22:05 UTC (permalink / raw)
  To: Alan Maguire
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Song Liu, Yonghong Song, john fastabend, KP Singh, Bill Wendling,
	Shuah Khan, bpf, Networking, open list:KERNEL SELFTEST FRAMEWORK,
	open list

On Fri, Jul 16, 2021 at 2:44 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> If data is packed, data structures can store it outside of usual
> boundaries.  For example a 4-byte int can be stored on a unaligned
> boundary in a case like this:
>
> struct s {
>         char f1;
>         int f2;
> } __attribute((packed));
>
> ...the int is stored at an offset of one byte.  Some platforms have
> problems dereferencing data that is not aligned with its size, and
> code exists to handle most cases of this for BTF typed data display.
> However pointer display was missed, and a simple macro to test if
> "data_is_unaligned(data, data_sz)" would help clarify this code.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/btf_dump.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 929cf93..9dfe9c1 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -1654,6 +1654,8 @@ static int btf_dump_base_type_check_zero(struct btf_dump *d,
>         return 0;
>  }
>
> +#define data_is_unaligned(data, data_sz)       (((uintptr_t)data) % data_sz)
> +

there is no need for macro, please use static function. And
ptr_is_aligned() is probably a better form:

if (!ptr_is_aligned(data, sz)) {
    /* handle uncommon case */
}

ptr_is_aligned() can be probably reused more readily in some other places later.

>  static int btf_dump_int_data(struct btf_dump *d,
>                              const struct btf_type *t,
>                              __u32 type_id,
> @@ -1672,7 +1674,7 @@ static int btf_dump_int_data(struct btf_dump *d,
>         /* handle packed int data - accesses of integers not aligned on
>          * int boundaries can cause problems on some platforms.
>          */
> -       if (((uintptr_t)data) % sz)
> +       if (data_is_unaligned(data, sz))
>                 return btf_dump_bitfield_data(d, t, data, 0, 0);
>
>         switch (sz) {
> @@ -1739,7 +1741,7 @@ static int btf_dump_float_data(struct btf_dump *d,
>         int sz = t->size;
>
>         /* handle unaligned data; copy to local union */
> -       if (((uintptr_t)data) % sz) {
> +       if (data_is_unaligned(data, sz)) {
>                 memcpy(&fl, data, sz);
>                 flp = &fl;
>         }
> @@ -1897,7 +1899,10 @@ static int btf_dump_ptr_data(struct btf_dump *d,
>                               __u32 id,
>                               const void *data)
>  {
> -       btf_dump_type_values(d, "%p", *(void **)data);
> +       void *ptrval;

sizeof(void *) could be 4 on the host system and 8 in BTF. If you want
to preserve the speed, I'd do something like:

if (ptr_is_aligned(data, sizeof(void *)) && sizeof(void *) == d->ptr_sz) {
    btf_dump_type_values(d, "%p", *(void **)data);
} else {
    /* fetch pointer value as unaligned integer */
    if (d->ptr_sz == 4)
        printf("0x%x")
    else
        printf("0x%llx")
}

Maybe there is some cleaner way. But that should work, no?

> +
> +       memcpy(&ptrval, data, d->ptr_sz);
> +       btf_dump_type_values(d, "%p", ptrval);
>         return 0;
>  }
>
> @@ -1910,7 +1915,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d,
>         int sz = t->size;
>
>         /* handle unaligned enum value */
> -       if (((uintptr_t)data) % sz) {
> +       if (data_is_unaligned(data, sz)) {
>                 *value = (__s64)btf_dump_bitfield_get_data(d, t, data, 0, 0);
>                 return 0;
>         }
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-07-16 22:05 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-16 21:43 [PATCH bpf-next] libbpf: clarify/fix unaligned data issues for btf typed dump Alan Maguire
2021-07-16 22:05 ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).