| Message ID | 20211013160902.428340-5-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | btf_dump fixes for s390 | expand |
On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > Currently ptr_is_aligned() takes size, and not alignment, as a > parameter, which may be overly pessimistic e.g. for __i128 on s390, > which must be only 8-byte aligned. Fix by using btf__align_of() where > possible - one notable exception is ptr_sz, for which there is no > corresponding type. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > tools/lib/bpf/btf_dump.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > index 25ce60828e8d..da345520892f 100644 > --- a/tools/lib/bpf/btf_dump.c > +++ b/tools/lib/bpf/btf_dump.c > @@ -1657,9 +1657,9 @@ static int btf_dump_base_type_check_zero(struct btf_dump *d, > return 0; > } > > -static bool ptr_is_aligned(const void *data, int data_sz) > +static bool ptr_is_aligned(const void *data, int alignment) > { > - return ((uintptr_t)data) % data_sz == 0; > + return ((uintptr_t)data) % alignment == 0; btf__align_of() can return 0 on error and this will be div by 0. I think the better approach would be for ptr_is_aligned to accept struct btf *btf and __u32 type_id, call btf__align_of() based on btf and type id, handle 0 case pessimistically (assume not aligned). > } > > static int btf_dump_int_data(struct btf_dump *d, > @@ -1681,7 +1681,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 (!ptr_is_aligned(data, sz)) { > + if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) { > memcpy(buf, data, sz); > data = buf; > } > @@ -1770,7 +1770,7 @@ static int btf_dump_float_data(struct btf_dump *d, > int sz = t->size; > > /* handle unaligned data; copy to local union */ > - if (!ptr_is_aligned(data, sz)) { > + if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) { > memcpy(&fl, data, sz); > flp = &fl; > } > @@ -1953,10 +1953,8 @@ static int btf_dump_get_enum_value(struct btf_dump *d, > __u32 id, > __s64 *value) > { > - int sz = t->size; > - > /* handle unaligned enum value */ > - if (!ptr_is_aligned(data, sz)) { > + if (!ptr_is_aligned(data, btf__align_of(d->btf, id))) { > __u64 val; > int err; > > -- > 2.31.1 >
On Wed, 2021-10-20 at 11:44 -0700, Andrii Nakryiko wrote: > On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > Currently ptr_is_aligned() takes size, and not alignment, as a > > parameter, which may be overly pessimistic e.g. for __i128 on s390, > > which must be only 8-byte aligned. Fix by using btf__align_of() > > where > > possible - one notable exception is ptr_sz, for which there is no > > corresponding type. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > tools/lib/bpf/btf_dump.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > index 25ce60828e8d..da345520892f 100644 > > --- a/tools/lib/bpf/btf_dump.c > > +++ b/tools/lib/bpf/btf_dump.c > > @@ -1657,9 +1657,9 @@ static int > > btf_dump_base_type_check_zero(struct btf_dump *d, > > return 0; > > } > > > > -static bool ptr_is_aligned(const void *data, int data_sz) > > +static bool ptr_is_aligned(const void *data, int alignment) > > { > > - return ((uintptr_t)data) % data_sz == 0; > > + return ((uintptr_t)data) % alignment == 0; > > btf__align_of() can return 0 on error and this will be div by 0. I > think the better approach would be for ptr_is_aligned to accept > struct > btf *btf and __u32 type_id, call btf__align_of() based on btf and > type > id, handle 0 case pessimistically (assume not aligned). I thought about this, but it won't cover the ptr_sz case. Maybe we just need two functions - I'll give it a try tomorrow.
On Wed, Oct 20, 2021 at 4:05 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Wed, 2021-10-20 at 11:44 -0700, Andrii Nakryiko wrote: > > On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich <iii@linux.ibm.com> > > wrote: > > > > > > Currently ptr_is_aligned() takes size, and not alignment, as a > > > parameter, which may be overly pessimistic e.g. for __i128 on s390, > > > which must be only 8-byte aligned. Fix by using btf__align_of() > > > where > > > possible - one notable exception is ptr_sz, for which there is no > > > corresponding type. > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > --- > > > tools/lib/bpf/btf_dump.c | 12 +++++------- > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > index 25ce60828e8d..da345520892f 100644 > > > --- a/tools/lib/bpf/btf_dump.c > > > +++ b/tools/lib/bpf/btf_dump.c > > > @@ -1657,9 +1657,9 @@ static int > > > btf_dump_base_type_check_zero(struct btf_dump *d, > > > return 0; > > > } > > > > > > -static bool ptr_is_aligned(const void *data, int data_sz) > > > +static bool ptr_is_aligned(const void *data, int alignment) > > > { > > > - return ((uintptr_t)data) % data_sz == 0; > > > + return ((uintptr_t)data) % alignment == 0; > > > > btf__align_of() can return 0 on error and this will be div by 0. I > > think the better approach would be for ptr_is_aligned to accept > > struct > > btf *btf and __u32 type_id, call btf__align_of() based on btf and > > type > > id, handle 0 case pessimistically (assume not aligned). > > I thought about this, but it won't cover the ptr_sz case. Maybe we > just need two functions - I'll give it a try tomorrow. > Sorry, what's the ptr_sz case? Is this about btf_ptr_sz() helper somehow?
On Wed, 2021-10-20 at 16:10 -0700, Andrii Nakryiko wrote: > On Wed, Oct 20, 2021 at 4:05 PM Ilya Leoshkevich <iii@linux.ibm.com> > wrote: > > > > On Wed, 2021-10-20 at 11:44 -0700, Andrii Nakryiko wrote: > > > On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich > > > <iii@linux.ibm.com> > > > wrote: > > > > > > > > Currently ptr_is_aligned() takes size, and not alignment, as a > > > > parameter, which may be overly pessimistic e.g. for __i128 on > > > > s390, > > > > which must be only 8-byte aligned. Fix by using btf__align_of() > > > > where > > > > possible - one notable exception is ptr_sz, for which there is no > > > > corresponding type. > > > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > --- > > > > tools/lib/bpf/btf_dump.c | 12 +++++------- > > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > > index 25ce60828e8d..da345520892f 100644 > > > > --- a/tools/lib/bpf/btf_dump.c > > > > +++ b/tools/lib/bpf/btf_dump.c > > > > @@ -1657,9 +1657,9 @@ static int > > > > btf_dump_base_type_check_zero(struct btf_dump *d, > > > > return 0; > > > > } > > > > > > > > -static bool ptr_is_aligned(const void *data, int data_sz) > > > > +static bool ptr_is_aligned(const void *data, int alignment) > > > > { > > > > - return ((uintptr_t)data) % data_sz == 0; > > > > + return ((uintptr_t)data) % alignment == 0; > > > > > > btf__align_of() can return 0 on error and this will be div by 0. I > > > think the better approach would be for ptr_is_aligned to accept > > > struct > > > btf *btf and __u32 type_id, call btf__align_of() based on btf and > > > type > > > id, handle 0 case pessimistically (assume not aligned). > > > > I thought about this, but it won't cover the ptr_sz case. Maybe we > > just need two functions - I'll give it a try tomorrow. > > > > Sorry, what's the ptr_sz case? Is this about btf_ptr_sz() helper > somehow? Almost, I'm referring to this check: static int btf_dump_ptr_data(struct btf_dump *d, const struct btf_type *t, __u32 id, const void *data) { if (ptr_is_aligned(data, d->ptr_sz) && d->ptr_sz == sizeof(void *)) { ...
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index 25ce60828e8d..da345520892f 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -1657,9 +1657,9 @@ static int btf_dump_base_type_check_zero(struct btf_dump *d, return 0; } -static bool ptr_is_aligned(const void *data, int data_sz) +static bool ptr_is_aligned(const void *data, int alignment) { - return ((uintptr_t)data) % data_sz == 0; + return ((uintptr_t)data) % alignment == 0; } static int btf_dump_int_data(struct btf_dump *d, @@ -1681,7 +1681,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 (!ptr_is_aligned(data, sz)) { + if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) { memcpy(buf, data, sz); data = buf; } @@ -1770,7 +1770,7 @@ static int btf_dump_float_data(struct btf_dump *d, int sz = t->size; /* handle unaligned data; copy to local union */ - if (!ptr_is_aligned(data, sz)) { + if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) { memcpy(&fl, data, sz); flp = &fl; } @@ -1953,10 +1953,8 @@ static int btf_dump_get_enum_value(struct btf_dump *d, __u32 id, __s64 *value) { - int sz = t->size; - /* handle unaligned enum value */ - if (!ptr_is_aligned(data, sz)) { + if (!ptr_is_aligned(data, btf__align_of(d->btf, id))) { __u64 val; int err;
Currently ptr_is_aligned() takes size, and not alignment, as a parameter, which may be overly pessimistic e.g. for __i128 on s390, which must be only 8-byte aligned. Fix by using btf__align_of() where possible - one notable exception is ptr_sz, for which there is no corresponding type. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/lib/bpf/btf_dump.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)