Fix all instances of -Waddress-of-packed-member #40909

carlescufi · 2021-12-04T12:09:02Z

This finally closes #16587 by removing all remaining pieces of code that can trigger the compiler warning after #39192 resolved all the ones in the networking code.

Additional analysis of the problem can be found here.

include/toolchain/common.h

cvinayak · 2021-12-04T13:43:30Z

subsys/bluetooth/controller/ll_sw/ull_conn.c

This is intentionally misaligned as the field is inside a packed PDU struct. memcpy is used to fill this address ensuring there is no memfault.

I assume this is true too for line 3759, which is currently asserting?

3759 in this PR? None of the struct typecast use should assert, if they do, then its genuine misalignment/bug.

The assert in line 3759 in ull_conn.c hits, but it's not a struct, it's an int16_t @cvinayak \

I assume this is true too for line 3759, which is currently asserting?

Yes.

subsys/bluetooth/controller/hci/hci.c

cvinayak · 2021-12-05T02:07:15Z

@carlescufi added commits to address required changes related to Broadcast ISO and Connected ISO implementations.

carlescufi · 2021-12-06T10:30:21Z

kernel/mmu.c

@dcpleung @andyross @peter-mitsis any idea why we are hitting this assert if I uncomment it?

I'm not the expert on paging, but per the comment at the top of the struct in kernel/include/mmu.h it's intentionally misaligned for size optimization. z_page_frame is a 5- or 9-byte struct (one pointer and a flags byte) and the intent seems to be to pack them all together, alignment be damned.

OK, I will leave out the assertion then. Thanks!

carlescufi · 2021-12-06T10:30:32Z

kernel/userspace.c

@dcpleung @andyross @peter-mitsis any idea why we are hitting this assert if I uncomment it?

You want to be checking IS_PTR_ALIGNED(dyn, struct dyn_obj), no? It would be pathological, but you could imagine a kernel object aligned on N*8 + 4, with a (8-byte-aligned) dyn_obj field offset 4 or 12 bytes in, and that would be legal.

Beyond that possibility, the question is why this particular kernel object got misaligned. And as this is a dynamic object, that points to a glitch in the allocator? Someone should definitely look at this.

could be that the object is not allocated using the aligned alloc function?

You want to be checking IS_PTR_ALIGNED(dyn, struct dyn_obj),

@andyross
It really is the same, because the struct z_object is the first member of struct dyn_obj. But sure, let me fix that.

could be that the object is not allocated using the aligned alloc function?

@dcpleung I have no idea, could you check?

z_impl_k_object_alloc() is using obj_align_get() to feed alignment to memory allocation, where it should align to sizeof(void *).

Does IS_PTR_ALIGNED(dyn, struct dyn_obj) work?

carlescufi · 2021-12-06T10:36:39Z

@carlescufi added commits to address required changes related to Broadcast ISO and Connected ISO implementations.

Thank you @cvinayak, added that in the Bluetooth commit.

cvinayak

Controller changes look good.

jhedberg · 2021-12-07T11:04:33Z

kernel/mmu.c

I don't think the explicit typecast is needed. Isn't one of the main feature of void pointers that you don't need to do explicit casts to/from them.

Absolutely, will check and remove. Thanks!

Should be done now @jhedberg, thanks for catching this.

I've also removed them from the inverse assignment (void to a data type) since those are not necessary either.

jhedberg · 2021-12-07T11:04:52Z

subsys/bluetooth/controller/hci/hci.c

Same here regarding unnecessary explicit typecast

jhedberg · 2021-12-07T11:05:46Z

subsys/bluetooth/controller/hci/hci.c

And here. I guess it doesn't make sense to to go pointing out every single place in this PR, but if you agree it be good to remove these explicit casts.

After having resolved all of the instances of packed member access, re-enable the warning. Fixes zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

Add a new macro, IS_PTR_ALIGNED() that verifies if a pointer is aligned sufficiently for a particular data type provided as an argument. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

Fix all instances in the controller of -Waddress-of-packed-member by casting through an intermediate variable and verifying the alignment with an assertion. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no> Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

rlubos

LGTM

The following warning is triggered by GCC when -Waddress-of-packed-member is enabled: /home/carles/src/zephyr/zephyr/kernel/mmu.c: In function 'free_page_frame_list_put': /home/carles/src/zephyr/zephyr/kernel/mmu.c:383:42: warning: taking address of packed member of 'struct z_page_frame' may result in an unaligned pointer value [-Waddress-of-packed-member] 383 | sys_slist_append(&free_page_frame_list, &pf->node); This is due to the fact that sys_snode_t node is an unpacked structure inside a packed z_page_frame structure, so that the alignment of the former cannot be ensured if placed inside the latter. Given that alignment of z_page_frame is ensured by the code, silence the compiler by going through an intermediate variable. More info in zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

The warning below appears once -Waddress-of-packed-mem is enabled: /home/carles/src/zephyr/zephyr/kernel/userspace.c: In function 'unref_check': /home/carles/src/zephyr/zephyr/kernel/userspace.c:471:28: warning: converting a packed 'struct z_object' pointer (alignment 4) to a 'struct dyn_obj' pointer (alignment 16) may result in an unaligned pointer value [-Waddress-of-packed-mem ber] 471 | CONTAINER_OF(ko, struct dyn_obj, kobj); To avoid the warning, use an intermediate void * variable. More info in zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

The warning below appears once -Waddress-of-packed-mem is enabled: /home/carles/src/zephyr/zephyr/arch/x86/core/acpi.c: In function 'z_acpi_find_table': /home/carles/src/zephyr/zephyr/arch/x86/core/acpi.c:190:24: warning: taking address of packed member of 'struct acpi_xsdt' may result in an unaligned pointer value [-Waddress-of-packed-member] 190 | for (uint64_t *tp = &xsdt->table_ptrs[0]; tp < end; tp++) { To avoid the warning, use an intermediate void * variable. More info in zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

The warning below appears once -Waddress-of-packed-mem is enabled: /__w/zephyr/zephyr/drivers/gpio/gpio_sx1509b.c: In function 'port_write': /__w/zephyr/zephyr/drivers/gpio/gpio_sx1509b.c:456:19: error: taking address of packed member of 'struct sx1509b_pin_state' may result in an unaligned pointer value [-Werror=address-of-packed-member] 456 | uint16_t *outp = &drv_data->pin_state.data; To avoid the warning, use an intermediate void * variable. More info in zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

The warning below appears once -Waddress-of-packed-mem is enabled: /__w/zephyr/zephyr/tests/kernel/mem_protect/userspace/src/main.c: In function 'test_main': /__w/zephyr/zephyr/tests/kernel/mem_protect/userspace/src/main.c:1024:17: error: converting a packed 'k_thread_stack_t' {aka 'struct z_thread_stack_element'} pointer (alignment 1) to a 'struct z_x86_thread_stack_header' pointer (alignment 4096) may result in an unaligned pointer value [-Werror=address-of-packed-member] 1024 | hdr = ((struct z_x86_thread_stack_header *)ztest_thread_stack); To avoid the warning, use an intermediate void * variable. More info in zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

The warning below appears once -Waddress-of-packed-mem is enabled: /__w/zephyr/zephyr/tests/lib/mpsc_pbuf/src/main.c: In function 'item_put_data_overwrite': /__w/zephyr/zephyr/tests/lib/mpsc_pbuf/src/main.c:497:3: error: converting a packed 'struct test_data_ext' pointer (alignment 1) to a 'uint32_t' {aka 'const unsigned int'} pointer (alignment 4) may result in an unaligned pointer value [-Werror=address-of-packed-member] 497 | mpsc_pbuf_put_data(&buffer, (uint32_t *)&item, len); To avoid the warning, as well as several others related to the same problem, use an intermediate void * variable. More info in zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

The warning below appears once -Waddress-of-packed-mem is enabled: /__w/zephyr/zephyr/drivers/sensor/bmi160/bmi160.c: In function bmi160_gyr_channel_get: /__w/zephyr/zephyr/drivers/sensor/bmi160/bmi160.c:795:23: error: taking address of packed member of struct <anonymous> may result in an unaligned pointer value [-Werror=address-of-packed-member] 795 | data->sample.gyr, val); | ~~~~~~~~~~~~^~~~ /__w/zephyr/zephyr/drivers/sensor/bmi160/bmi160.c: In function bmi160_acc_channel_get: /__w/zephyr/zephyr/drivers/sensor/bmi160/bmi160.c:807:23: error: taking address of packed member of struct <anonymous> may result in an unaligned pointer value [-Werror=address-of-packed-member] 807 | data->sample.acc, val); | ~~~~~~~~~~~~^~~~ To avoid the warning, make the struct non-packed, since it is not necessary in this case given that a union already guarantees that the pointer to the union points to each member of the union equally.. More info in zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

andyross · 2021-12-09T16:37:29Z

kernel/userspace.c

+
+	struct dyn_obj *dyn = CONTAINER_OF(vko, struct dyn_obj, kobj);
+	/* TODO: check why this assert hits */
+	/*__ASSERT(IS_PTR_ALIGNED(dyn, struct dyn_obj), "unaligned z_object");*/


Just to repeat my comment from elsewhere: this points to some suspicious behavior in the kernel dynamic object allocator. We should probably open a bug to make sure it gets looked at. Given the architecture, it's not illegal to misalign structs, but we shouldn't be doing it without a good reason.

(If I had to make a wild guess, it was written for i386 and 4-byte-aligns everything, so misses half the time for structs with 64 bit pointers.)

Indeed, see here: #41062

carlescufi requested review from andyross, cvinayak, dcpleung, kruithofa, nashif, tejlmand and thoh-ot as code owners December 4, 2021 12:09

carlescufi requested a review from rlubos December 4, 2021 12:09

github-actions bot added area: API Changes to public APIs area: Bluetooth area: Bluetooth Controller area: Build System area: Kernel labels Dec 4, 2021

carlescufi requested review from jhedberg and removed request for kruithofa December 4, 2021 12:09

carlescufi force-pushed the fix-16587 branch from 2797c2e to aa83b8d Compare December 4, 2021 12:14

carlescufi commented Dec 4, 2021

View reviewed changes

include/toolchain/common.h Outdated Show resolved Hide resolved

cvinayak requested changes Dec 4, 2021

View reviewed changes

carlescufi force-pushed the fix-16587 branch 2 times, most recently from 0ab22b5 to a2f5da6 Compare December 4, 2021 16:26

carlescufi force-pushed the fix-16587 branch 2 times, most recently from db3c954 to dd54411 Compare December 5, 2021 23:00

carlescufi requested a review from cvinayak December 6, 2021 10:29

carlescufi commented Dec 6, 2021

View reviewed changes

cvinayak approved these changes Dec 7, 2021

View reviewed changes

jhedberg reviewed Dec 7, 2021

View reviewed changes

carlescufi force-pushed the fix-16587 branch from dd54411 to 8a670a0 Compare December 7, 2021 12:04

carlescufi removed request for SebastianBoe, Vudentz, alwa-nordic, asbjornsabo, enjiamai, kruithofa, laurenmurphyx64, mtpr-ot and ppryga-nordic December 8, 2021 16:04

carlescufi force-pushed the fix-16587 branch from 26f943a to f50c981 Compare December 8, 2021 16:50

carlescufi added 3 commits December 8, 2021 21:01

gcc: Enable address-of-packed-member warnings

1d61ebc

After having resolved all of the instances of packed member access, re-enable the warning. Fixes zephyrproject-rtos#16587. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

include: Add a macro to check pointer alignment

909060d

Add a new macro, IS_PTR_ALIGNED() that verifies if a pointer is aligned sufficiently for a particular data type provided as an argument. Signed-off-by: Carles Cufi <carles.cufi@nordicsemi.no>

carlescufi force-pushed the fix-16587 branch from f50c981 to bd4a3f1 Compare December 8, 2021 20:01

nashif added the DNM This PR should not be merged (Do Not Merge) label Dec 8, 2021

rlubos approved these changes Dec 9, 2021

View reviewed changes

carlescufi force-pushed the fix-16587 branch from bd4a3f1 to e31ba1c Compare December 9, 2021 10:17

carlescufi added 7 commits December 9, 2021 11:31

carlescufi force-pushed the fix-16587 branch from e31ba1c to 3e41e10 Compare December 9, 2021 10:31

andyross approved these changes Dec 9, 2021

View reviewed changes

carlescufi merged commit 2bc9cb4 into zephyrproject-rtos:main Dec 10, 2021

carlescufi mentioned this pull request Dec 10, 2021

kernel: userspace: Potential misaligned access #41062

Closed

henrikbrixandersen mentioned this pull request Dec 16, 2021

Fix alignment warnings in mcan #41265

Closed

carlescufi mentioned this pull request Jun 25, 2025

net: ip: Relax alignment requirements of in(6)_addr/sockaddr_ll #90954

Closed

Fix all instances of -Waddress-of-packed-member #40909

Fix all instances of -Waddress-of-packed-member #40909

Uh oh!

Conversation

carlescufi commented Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cvinayak commented Dec 5, 2021

Uh oh!

carlescufi Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Dec 6, 2021

Uh oh!

cvinayak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlubos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

carlescufi commented Dec 4, 2021 •

edited

Loading

carlescufi Dec 4, 2021 •

edited

Loading

carlescufi Dec 6, 2021 •

edited

Loading

carlescufi Dec 6, 2021 •

edited

Loading

carlescufi Dec 9, 2021 •

edited

Loading