Kernel/Net: Minimal amount of IPv6 for ping replies #25009
Conversation
This enum is neither IPv4-specific, nor does it have to do anything with the network layer, as the name may imply. The enum is moved to a new header containing common IPv4/IPv6 definitions.
Commit ad73ade needlessly introduced an IPv4 directory. If we were to keep it, sharing common headers between IPv4 and IPv6 would be much messier, and may potentially increase code duplication. This change renames the IPv4 directory to IP to aid with later IPv6 porting efforts.
Kernel/Net/IP/IPv6.h
Outdated
| void* payload() { return this + 1; } | ||
| void const* payload() const { return this + 1; } |
There was a problem hiding this comment.
For this please prefer a traling u8 m_playload[]; member
There was a problem hiding this comment.
Also maybe use Bytes/ReadOnlyBytes here
There was a problem hiding this comment.
I've remade this (and all other relevant structs in Kernel/Net) to use m_payload; I'm not sure about returning a Span from here, as it would result in a non-insignificant refactor, as those functions are called (and directly cast) in a lot of places.
Redoing everything here to use Spans may be a good idea for a future refactor, but I would prefer to keep it out of this (already quite large) MR.
| }; | ||
|
|
||
| // https://datatracker.ietf.org/doc/html/rfc8200#section-3 | ||
| class [[gnu::packed]] IPv6PacketHeader { |
There was a problem hiding this comment.
Not sure if this "needs to be" a class or if a struct with mostly non m_ members would be sufficient
(Maintainers choice)
Also dislike this needlessly being packed, especially with the apparently weird packed specific behavior of bitfields
There was a problem hiding this comment.
Hmm. I'm not sure how to handle this in a way where this wouldn't be packed? We're using that to form a packet, albeit not directly (it's always included in another packed struct)
As for class vs struct - looking at the IPv4 implementation (and some other code in Kernel/Net/), it's quite common for the main header to be a class with methods to call. Same with it being packed, btw.
There was a problem hiding this comment.
Okay, I've checked just now - removing [[gnu::packed]] from this (and adjacent structures in Kernel/Net) does work, even down to EthernetFrameHeader, I can foresee a scenario where it does break in an unexpected way. So I would vouch for leaving it as-is.
There was a problem hiding this comment.
The struct thing is from what we do in the HW parts of the code, to my knowledge,
and as long as you dont try to do anything with member references [[gnu::packed]] should be fine for now I guess
There was a problem hiding this comment.
should be fine for now I guess
Dropping it and opening up the possibility to padding seems like a massive footgun, why "for now"?
There was a problem hiding this comment.
Not sure if this "needs to be" a class or if a struct with mostly non m_ members would be sufficient
there is no ambiguity here, the coding style guide says to prefer classes for things with methods and this has a bunch of those for bitwise ops
There was a problem hiding this comment.
Dropping it and opening up the possibility to padding seems like a massive footgun, why "for now"?
gnu::packed lowers the alignment to 1, which causes bytewise accesses to be generated for some ARM and RISC-V systems. This isn't as bad as for MMIO, but it causes worse codegen. Padding bytes should already be caught by AssertSizes.
There was a problem hiding this comment.
2 more things to note:
Most specs are nice enough to follow common alignment rules and pack accordingly.
BigEndian/NetworkOrdered is for some reason [[gnu::packed]]
There was a problem hiding this comment.
i'll remove the [[gnu::packed]] from all relevant places around Kernel/Net (except for the card-relevant driver code) and do some testing. Likely later tonight.
There was a problem hiding this comment.
getting rid of [[gnu::packed]] in all related structures under Kernel/Net that don't directly touch the hardware brought me to the following crashes:
[Network Task(9:9)]: KUBSAN: reference binding to misaligned address 0x000000200d909036 of type 'const IC
MPv6NeighborSolicitation'
[Network Task(9:9)]: KUBSAN: at /home/domi/serenity/Kernel/Net/NetworkTask.cpp, line 294, column: 25
[Network Task(9:9)]: Kernel + 0x00000000004354c4 print_location(AK::UBSanitizer::SourceLocation const&)
+0xfc
[Network Task(9:9)]: Kernel + 0x00000000004369c4 __ubsan_handle_type_mismatch_v1 +0x31c
[Network Task(9:9)]: Kernel + 0x00000000003eb7aa Kernel::NetworkTask_main(void*) +0x2f02
[Network Task(9:9)]: UB is configured to be deadly, halting the system.
That's a bit_cast on L294 in NetworkTask.cpp.
ICMP (IPv4) was working OK, in both directions. However, TCP connections were failing to get established (so the alignment broke structs, or at least broke checksumming). I suspect that those are not the only issues, but I didn't conduct further testing.
Hence, I'm holding my ground with regards to keeping the packed structs as they currently exist. We could work around some of those UBs, and fix other things that break, but this opens us up to a lot of platform-dependent behavior which may never get hit when testing on x86.
sdomi
force-pushed
the
aaaaaaaad
branch
12 times, most recently
from
c810410
to
32f897a
Compare
Up until now, the internet_checksum function lived in IPv4.h. Due to its use in IPv6, a better place for it nowadays would be in IP.h. Due to how it gets used in IPv6 scenarios, it makes sense to extend it to run over multiple smaller structs instead of one big continous chunk of memory. This change converts internet_checksum into a class with methods "add" and "finish", which process data and return the final result, respectively. This commit also fixes proper hash computation for payloads that have an odd length. Furthermore, the function was refactored to use a ReadonlyBytes object instead of separate data and size. Co-Authored-By: Wanda <[email protected]>
This commit introduces the first bit of IPv6 support into SerenityOS. An IPv6 packet header structure allows NetworkTask to recognize and log correctly shaped IPv6 packets, but nothing is done with those yet. A new logging flag, IPV6_DEBUG, allows debugging of IPv6 internals. Co-Authored-By: sdomi <[email protected]>
This change also migrates the existing fill_in_ipv4_header from C-style casts to proper C++ casts.
github-actions
bot
added
the
👀 pr-needs-review
This commit adds the minimum amount of code to make Serenity reply to ICMPv6 ping packets. This includes basic Network Discovery Protocol support, as well as a basic ICMPv6 Echo Request/Response handler. This change also introduces a new debug flag, ICMPV6_DEBUG. Co-Authored-By: kleines Filmröllchen <[email protected]>
Up until now, protocol handlers for IPv4 and ARP had to rediscover the adapter based on trying to match the address to all interfaces. This approach is relatively slow, introduces more code duplication, and doesn't work with multiple IPs per interface (as is the case with IPv6). This change introduces directly passing the adapter to all handlers that need it, cutting out the retrieval process. Furthermore, we discard the packet much faster if the IP doesn't match.
According to ARP.h, address lengths default to size of their underlying types (IPv4Address and MACAddress respectively). We never modify those values, and in reality, ARP never carries anything else. Hence, those checks resolved to comparing sizeof to itself, which probably gets optimized out of the compiler anyways. This change removes the checks that will never fail, plus changes the function definition to pass adapter directly, matching other handlers.
Unifying with other structures, this introduces m_payload for all relevant structures, except the TCP header (which has a different concept of payload than the rest). This allows us to return a pointer directly instead of doing pointer arithmetic to point to the end of the structure.
This (draft) MR introduces enough code to parse IPv6 packets, and respond to ICMPv6 Neighbor Solicitations and ICMPv6 Echo Requests - aka the minimal amount of stuff to make Serenity respond to pings.
The effects aren't terribly spectacular, but if you run the VM with a tap interface, you can now ping the autoconfigured IPv6 address:
Beyond code needed for ICMP, we also introduce a small refactor of protocol handlers: before, we had to recover the adapter by trying to match the destination IP against IPs of all known adapters; Now, we just pass the adapter to the functions directly, cutting out the matching process. In the future, this will aid us with getting rid of the one-address-per-adapter limitation, which will be crucial for full IPv6 support.
cc: @kleinesfilmroellchen