-
Notifications
You must be signed in to change notification settings - Fork 8.1k
net: ip: Relax alignment requirements of in(6)_addr/sockaddr_ll #90954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This PR also fixes #88665 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, throughout the network subsystem, these structures
are accessed using UNALIGNED_GET/UNALIGNED_PUT macros to safely handle
unaligned memory accesses. This is often necessary as IP addresses do not
always occur aligned in network packets.
Then why do you need to pack them then? is UBSAN complaining because the values are being accessed byte-by-byte?
The issue isn't with the dereferencing itself, UNALIGNED_GET and UNALIGNED_PUT handle that safely, but rather with the type of pointer being accessed. Without this PR, a pointer of type By applying the packed attribute to the struct, we explicitly lower its alignment requirement to 1 byte. This ensures that even if the pointer is not 4-byte aligned, accessing its members no longer triggers a UBSAN error. |
@carlescufi Is @tpambor s answer satisfactory to you? Would you like some additional changes? Do you have an alternative suggestion? |
@carlescufi What do you think? |
@tpambor sorry for the delay. I am still not quite sold on this idea. You say that the network stack is using |
Basically, we can not have our cake and eat it too. We can
At the moment we try to do both, we try to say, that the alignment requirement of Edit: Of coures one could have both by adding |
Isn't exactly this what was done in this PR: #39192? If you really want to access the Regarding the warnings that you see with UBSAN, could you please provide pointers to the source code lines that trigger them to see exactly how to fix them? |
In the PR description you can find a list with all locations that trigger UBSAN alignment errors, when building twister tests for the network subsystem with UBSAN enabled, e.g. running |
Sorry, my fault, I misread this! I will take a look |
@tpambor @clamattia I went back and tried to understand why UBSAN is throwing these warnings, and I am confused. Most of the warnings involve something similar to: So I am confused. The whole point of |
Under standard C rules, already forming the pointer from a misaligned struct (e.g. doing You can find more information in these threads by gcc https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114217#c7 and clang llvm/llvm-project#83710 |
Remove UNALIGNED_GET/UNALIGNED_PUT macros for struct pointers of types in_addr, in6_addr, sockaddr_*. These structs are now packed, allowing the compiler to handle unaligned accesses automatically. Manual use of UNALIGNED_GET/UNALIGNED_PUT is no longer necessary and has been removed to simplify the code. Signed-off-by: Tim Pambor <tim.pambor@codewrights.de>
|
@carlescufi I think #39192 is still needed as otherwise I rebased and pushed a new commit which now removes UNALIGNED_GET/UNALIGNED_PUT macros for struct pointers of I executed again all networking tests with UBSAN enabled and no alignment related problems were discovered and no
|
Consider creating a separate tracking issue for those. |
They are tracked here, #90882. I will update the tracking issue once this PR is merged. |
Sorry I could not reply earlier and thanks for your patience and ongoing work!
Are you sure? these warnings actually print: |
We've been discussing today with @rlubos and @jukkar about this very topic.
In any case, I am adamant to reject your approach of packing the addr stucts because we should not be using packed structs in a public API, that is not good practice and it is certainly not necessary as I explained. |
@carlescufi Thanks for the explanations. I am not 100% sure I understand how fixing using the approach of #39192 would look like. Taking your example
This approach would mean to do it like this?
to
|
Yes, but when we discussed this with @rlubos and @jukkar we thought it'd be better to instead use this approach:
In fact I can see now that #39192 was fixing it incorrectly in several places, for example: which is incorrect, because you cannot just cast that array to an addr struct, you will have issues later with UBSAN like the ones you found. Instead, the correct way to do this would be:
@rlubos will look into this a bit more next week. Are you on Zephyr's Discord? We could coordinate there, in the #networking channel. |
I agree.
Yes, I'm called there tpambor as well. Regarding performance, I tried three ways to implement this: https://godbolt.org/z/3Gh7P9rjf
Using clang, all variants are optimized and compiled to the same assembly for Cortex-M0. Using GCC, memcpy is not inlined for variant When using Clang, all variants are optimized to identical assembly for Cortex-M0. However, with GCC, memcpy is not inlined in variants For chips that support unaligned access, all variants compiled with Clang, as well as the |
Thanks for the investigation! I will leave to @rlubos, who is currently looking at fixing this throughout the code, to comment on his opinion about the different approaches.
I am not too surprised, it's perhaps the simplest for the compiler to deal with.
Yes, but at a cost. These functions are called per-packet, and adding another indirection (i.e. a second call) may seriously impact performance as you mentioned in your analysis. |
Thanks @tpambor, so far I've been looking into IPv6 side of things, and I've been taking the |
The aforementioned PoC for IPv6: https://github.com/zephyrproject-rtos/zephyr/compare/main...rlubos:zephyr:net/ip-addr-casting-cleanup?expand=1 |
I agree, I looked at your PoC and it looks reasonable. Just to let you know, I re-run the tests and for IPv6 there are two more locations which trigger UBSAN
|
Thanks @tpambor, oddly, I was not able to reproduce those failures locally but there were definitely issues in utils and ICMPv6 code (that must be where
I'll do some extra testing, but unless I encounter some blocker I think I could create a PR tomorrow. |
Nice work, thank you! I encountered one more for IPv4, in
And another one for IPv6 (though not alignment related):
Quick fix:
For testing I used this branch, https://github.com/tpambor/zephyr/commits/fixes-ubsan/, which also has the other pending UBSAN Pull Requests merged, as sometimes behind one runtime error, more runtime errors hide. |
@tpambor Thanks! I've caught those as well, should be fixed now. |
So here's the alternative PR: #92539. Combined with fixes from other PRs it seems to fix all UBSAN complaints in the networking code. |
I will close this PR in favor of #92539. |
The in_addr and in6_addr structures currently have a 4-byte alignment
requirement. However, throughout the network subsystem, these structures
are accessed using UNALIGNED_GET/UNALIGNED_PUT macros to safely handle
unaligned memory accesses. This is often necessary as IP addresses do not
always occur aligned in network packets.
This commit reduces the alignment requirement of in_addr and in6_addr
to 1 byte by marking them as packed, reflecting their actual usage.
The struct sockaddr has a minimum alignment requirement of 2 bytes.
It is commonly cast to more specific socket address types
such as sockaddr_in, sockaddr_ll, etc. To ensure safe casting and avoid
potential unaligned memory accesses, these derived structures must not
have a stricter alignment requirement than sockaddr.
This commit reduces the minimum alignment of sockaddr_ll from 4 bytes to
2 bytes, aligning it with sockaddr and preventing possible issues on
architectures that do not tolerate unaligned accesses.
In addition, compiler warnings are generated for potentially problematic
code like taking a pointer with a larger alignment requirement from one
of the struct members.
This change also resolves issues reported by the Undefined Behavior
Sanitizer (UBSAN), which flagged these unaligned accesses as undefined
behavior.
Errors reported by UBSAN fixed by this PR:
This is a step towards fixing #90882.