Refactor/consolidate net pkt ieee802154 options #51266
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
requested review from
andy-liu-telink,
cfriedt,
jciupis,
nandojve,
nashif,
pfalcon,
rlubos and
tbursztyka
as code owners
ghost
mentioned this pull request
nandojve
requested changes
Oct 14, 2022
Member
nandojve
left a comment
nandojve
left a comment
There was a problem hiding this comment.
Hi @fgrandel ,
I looked inside 802.14.2 only and have doubts about 2 points.
Author
|
@nandojve Thanks for your review! Could you have a look at my fix and answer to your comment to see whether this makes sense now? |
nandojve
requested changes
Oct 14, 2022
cfriedt
previously approved these changes
Oct 15, 2022
rlubos
reviewed
Oct 17, 2022
Contributor
rlubos
left a comment
rlubos
left a comment
There was a problem hiding this comment.
Looks good, just some nits.
added 8 commits
October 17, 2022 15:50
This change just fixes some insignificant typos. Signed-off-by: Florian Grandel <[email protected]>
Several IEEE 802154 drivers allocated RX packets from the TX pool. This may seem like a minor problem at first sight but it may become problematic if the pool is used to distinguish package types as is the case in some code paths, e.g. for packet priority or determination of the packet buffer pool. This bug also has the potential of starving the TX pool capacity which even may make devices vulnerable to DoS attacks as sending may be prohibited by addressing enough RX packets to a device to let it run out of TX capacity. Fixes: #51261 Signed-off-by: Florian Grandel <[email protected]>
Most IEEE 802.15.4 drivers do not support promiscuous mode, some do. There is a dedicated L2 flag to signal this capability to clients. Unfortunately the IEEE 802.15.4 L2 stack does not announce this flag even for drivers that correctly expose it in their HW capabilities. Some clients (notably the OpenThread L2) even uses promiscuous mode without checking whether the driver actually supports it. This change lets the vanilla IEEE 802.15.4 L2 check the driver's HW capabilities to announce promiscuous mode on its 'get_flags()' interface if supported. The OpenThread L2 uses a constant (potentially incorrect) response to 'get_flags()'. Fixing the OpenThread L2 is out of scope of this change. This change just introduces TODO messages to the OpenThread code so that the OpenThread team may fix the issue (or delete the TODO if they deem it irrelevant). Fixes: #51263 Signed-off-by: Florian Grandel <[email protected]>
Nordic's IEEE 802.15.4 radio driver adapter layer had a few raw accesses to net packet attributes. Packet attributes should never be accessed directly, though, but only through the dedicated accessor methods provided by the net core. This change replaces raw accesses to packet attributes by their respective wrapper functions. This also is a necessary precondition to the isolation and encapsulation of IEEE 802.15.4 specific packet attributes which will be introduced in a later commit of this change set. Fixes: #51264 Signed-off-by: Florian Grandel <[email protected]>
The net packet structure contains pointers to link-layer source and destination addresses. Usually, these structures do not point to separately allocated memory but directly into the packet's data buffer. In case of a deep package clone (which includes copying the buffer) the copy of the ll addresses continued to point into the old package (contrary to a rather misleading inline comment). This was proven by an additional failing unit test assertion. As the original package may be unreferenced while the cloned package is still being accessed, the ll address pointers of the cloned package may become invalid. The fix consists of two parts: * First it is determined whether a given ll address actually points into the buffer and if so at which logical cursor offset it is located. * If the address points into the package buffer then the cursor API is used to determine the corresponding physical memory location in the cloned package. The ll address of the cloned package is then patched to point to the cloned buffer. Additional assertions were introduced to the existing unit test to ensure that the newly generated address points to the correct content of the cloned package. The solution is implemented in a generic way so that the previously redundant implementations were consolidated into a single one. The code includes a check that ensures that the ll address check and manipulation will be skipped in case of shallow package copies. The change also addresses problems related to the "overwrite" flag of the package: * Package cloning assumes the overwrite flag to be set. Otherwise it will not work correctly. This was not ensured inside the clone method. * Package cloning manipulates the overwrite flag of the cloned package but does not reset it to represent the same state as the original package. The change introduces a fix and unit test assertions for both problems. Fixes: #51265 Signed-off-by: Florian Grandel <[email protected]>
The IEEE 802.15.4 L2 now sets the ll protocol in the packet to a specific value. This corresponds to the respective solution in Linux and is required to validate access to IEEE 802.15.4 specific attributes of the packet. Later change sets will rely on this value to ensure that IEEE 802.15.4 specific package content can only be accessed on IEEE 802.15.4 packages. Signed-off-by: Florian Grandel <[email protected]>
This change removes unnecessary padding from the net_pkt struct in preparation for restructuring it in a later commit. Depending on the number of activated Kconfig options, this saves between 4 and 8 bytes per packet. The change also removes anonymous unions inside of bitfields: * The assumption, that anonymous unions inside bitfields save space is wrong. They rather enforce an additional byte aligment boundary with padding before or after on most compilers (try -Wpadded to prove it). Removing the unions therefore counter-intuitively removes padding. * Some of the fields inside the union may actually not be orthogonal if several extensions are enabled at the same time. They may overwrite each other when activated at the same time. The change therefore not only improves packing of net_pkt but also makes net_pkt easier to understand and maintain by removing potentially non-orthogonal uses of bits within bitfields. The union of RX/TX-only attributes has been maintained, though, as it DOES save space and is easy to maintain. Maintainability has further been improved by introducing additional inline comments and anonymous structs that make the orthogonality of RX- and TX-attributes even more obvious. Signed-off-by: Florian Grandel <[email protected]>
This change implements part two of the program laid out in the TSCH RFC, see #50336#issuecomment-1250250154 : > Consolidate IEEE 802.15.4 options in net_pkt This change improves decoupling of generic net core code from IEEE 802.15.4 internals. It also simplifies IEEE 802.15.4 attribute cloning and thereby makes it easier to maintain and less error prone (and probably even faster as individual bits are no longer copied over separately). This enables us to extend and design IEEE 802.15.4 L2 attributes inside the package in isolation from the net core code which will no longer have to be changed when introducing changes or additions to the flags. This flexibility will be built upon in later change sets to model the IEEE 802.15.4 attributes closer to the spec. The solution is inspired by Linux's sk_buff->cb attribute which addresses the same concern as the attribute introduced in this change set: https://elixir.bootlin.com/linux/v6.0.1/source/include/linux/skbuff.h#L871 As the inline comment says: The cb attribute can be made a union or even a uint8[something] in the future, if further L2s need a control block, too. Right now such full indirection would make the code overly abstract, so I chose to compromise with maintainability in mind. Care has been taken to ensure that this changes does not introduce additional padding into the net package. To maintain zero-padding, future changes to the net packet struct will have to ensure that the IEEE 802.15.4 struct is 4-byte aligned (iff the IEEE 802.15.4 struct continues with max uint32_t scalar members) which is no deviation from the previous implementation. Signed-off-by: Florian Grandel <[email protected]>
Author
|
@rlubos Fixed the things you remarked. I'm a bit unsure about the copyright stuff. Is that what you meant? |
nandojve
approved these changes
Oct 17, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses part 2 of the initial development approach outlined in #50336 (comment) and confirmed in #50336 (comment):
The detailed approach and rationale for this change is outlined in the linked RFC.
The PR also fixes several bugs found while working on this change. Bugs are fixed in separate commits and have been reported as separate issues so they can be backported if you wish so:
Fixes: #51261
Fixes: #51263
Fixes: #51264
Fixes: #51265