ieee802154: Set intra-PAN flag for packets where src/dst PANs match#5685
ieee802154: Set intra-PAN flag for packets where src/dst PANs match#5685aeneby wants to merge 1 commit intoRIOT-OS:masterfrom
Conversation
|
Fixes #5684 |
OlegHahm
added
Type: bug
|
ACK for the change. Didn't test it until now. IMO this is not an urgent bugfix. Thus I'd like to wait until feature-un-freeze. |
|
Fine by me. Thanks. |
PeterKietzmann
added
the
CI: ready for build
|
What's Murdock complaining about? |
PeterKietzmann
added
CI: ready for build
|
I think the source for lwip was down at that time. Just restarted Murdock |
| uint8_t type = (flags & IEEE802154_FCF_TYPE_MASK); | ||
| uint8_t bcast = (flags & IEEE802154_BCAST); | ||
|
|
||
| if (dst_pan.u16 == src_pan.u16) { |
There was a problem hiding this comment.
to be on the safe side: could you also clear the bit in case the pans are different?
|
@PeterKietzmann please wait to merge. I'd like to give some more thought to @thomaseichinger's comment first, when I get a chance. |
According to section 7.5.6.1 of 802.15.4-2003, packets destined for the same PAN from which they were sent must have their intra-PAN flag set, and the source PAN omitted from the transmitted frame.
|
While I haven't managed to see the 802.15.4-2015 spec yet, there is this. Although it's only a submission draft, I have reason to believe (based on looking at what Wireshark has implemented, for example) that this - or something very close to it - is what ended up in the final specification. Perhaps someone in a position to do so can verify:
The PR has been updated to reflect this description; if there are no objections I'm happy for it to be merged as-is. |
|
@aeneby, @miri64, @thomaseichinger I'm sorry I was off during latest discussions. After following your conversation (also in #5759) it is not completely clear to me which OSes/stacks will be affected in interoperability when adopting this PR to the latest specification which -again- seems to enforce the elision of the src PAN, in case it equals the dst PAN. Could someone please summarize that? |
|
@PeterKietzmann as long as a stack/OS implements IEEE 802.15.4 correctly interoperability isn't effected. The problem with lwIP's interop is a recognized bug |
|
The question was, which stack/OS actually does it correctly.It is just lwip which does not implement it correctly? |
|
I believe Linux (4.7) will completely ignore packets with two identical PAN IDs. |
|
@PeterKietzmann Management summary as far as I get it: (Did I get you confused even more 😊) |
|
@thomaseichinger thanks and yes :-). Is there any reason against proceeding as follows:
|
|
Seems fine to me if others agree too. |
|
Because I think its unnecessary (because in the end only drivers use this function). It is (codewise and API-wise) simpler to just correct a potential developer's mistake and every byte we can save should be saved. We are working on constrained devices after all. Even if for some reason the device allows for the setting of that flag (should be 2 instructions, if I'm not mistaking) it's less code than checking the flag before the call and then again after (5 instructions, I think). |
|
I disagree. Yes it's an extra comparison, but that's just a quirk of having a modular stack. Correcting the PAN_COMP flag automatically in |
|
What is gnrc_netdev2->send() going to set the PAN_COMP flag to, if we remove this from the driver state? |
|
It would leave it unset. |
That seems arbitrary, and IMO reinforces the fact we should be setting the PAN compression flag before I apologise for taking so long with this PR, but I'm still trying to get my head around the unit tests. For example, |
Can we roll back for a minute? I think we are talking past each other here. If we remove the PAN_COMP flag from the driver state it does not know anything about PAN compression (because it does not exist for the driver), so naturally it would leave it unset. I don't see how this is arbitrary.
If you think they check an invalid state then correct them. I did not wrote them with regard to spec correctness, but API correctness. The API allows for this configuration so it needs to be tested. |
If the flag is zero this doesn't mean "unset", it means "set to zero". This is an arbitrary decision to take if the code doesn't know/care what the value should actually be.
Yes but I don't understand what the test result should be, given that we're sometimes correcting things and sometimes not. |
|
Actually, is it even currently possible for GNRC to send intra-PAN packets? I see that the |
IIRC I implemented (at least on the NDP side) that if you set an address like this |
@miri64 could you please point me to an example of this? Does this mean that the way to get a PAN ID in as part of the netif header would be to concatenate it with the src address? |
I couldn't find an actual example. Maybe I dropped it because non of the drivers did not supported it and it made things only more complicated (RFC 6775 doesn't even mention PAN IDs). The idea was early on that IEEE 802.15.4 devices would have 3 address types:
Yes, at least for short addresses (long addresses would need a PAN coordinator still): as examplified above, they would be prepended. |
|
This somehow breaks multicast messages with SRC_PAN == DST_PAN where it does not set the IEEE802154_FCF_PAN_COMP flag, what I found is that for multicast message dst_len = 0 when calling The result is that multicast messages such as RS have equal SRC_PAN and DST_PAN but the flag is not set, which makes Linux drop those RS packets. Unfortunately I run |
|
This is really ugly in a way, currently it works with Master-branch and As both of you @miri64 and @aeneby proposed I think it would be best to not set this flag in the driver and it shouldn't be modified by the user. It must be checked and set where the hdr is build and the SRC_PAN and DST_PAN are written (or compressed). |
|
btw. which makes it even more confusing and difficult to look up is that we have multiple
|
AFAIK, the current consensus is to remove the |
| uint8_t bcast = (flags & IEEE802154_BCAST); | ||
|
|
||
| if (dst_pan.u16 == src_pan.u16 && | ||
| (src_len != 0 && dst_len != 0)) { |
There was a problem hiding this comment.
the problem with this comparison of dst_len != 0 is that for multicast messages such as RS the length actually is 0 see 1. The result is the for RS messages we have SRC_PAN==DST_PAN but dst_len=0 and the flag will be unset which makes Linux drop those RS and never send an RA.
This could be fixed, for instance in 2, by not not passing netif_hdr->dst_l2addr_lendirectly but rather set this like with src_len. That is, if the packet is broadcast or multicast (see 3) then set dst_len = 2.
There was a problem hiding this comment.
the problem with this comparison of dst_len != 0 is that for multicast messages such as RS the length actually is 0 see 1. The result is the for RS messages we have SRC_PAN==DST_PAN but dst_len=0 and the will not be unset which makes Linux drops those RS and never send an RA.
Just an explanation for this behavior: since netif_hdr_t is L2 agnostic and the format of a multicast address can be very L2 specific, multicast is just handled by setting a special multicast flag and not setting the destination address in the netif_hdr_t. The L2 then sets the multicast address to its liking according to e.g. the IPv6 destination address (or in IEEE 802.15.4 by just using the broadcast address).
There was a problem hiding this comment.
The L2 then sets the multicast address to its liking according to e.g. the IPv6 destination address
where does this happen in the standard case of 6lo over 802154?
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
and what's the reasoning behind unset the broadcast flag here?
There was a problem hiding this comment.
@miri64 I don't understand why src address and length are set in _send here but the destination is not set the same way when it is multicast? I mean the code in gnrc_netdev2_ieee802154.c already knows about L2 so it should be able to set the address like here, or not?
Because the driver is not "supposed" to know the length of the broadcast address. That's this function's job.
and what's the reasoning behind unset the broadcast flag here?
The flag is not part of the IEEE 802.15.4 specs so adding it to the MAC header seems like a bad idea ;-).
There was a problem hiding this comment.
@miri64, this is no driver code (I thought)?! I was talking about these 2 files
- /sys/net/link_layer/ieee802154/ieee802154.c
- /sys/net/gnrc/link_layer/netdev2/gnrc_netdev2_ieee802154.c
both are not driver, but src is set in the GNRC (2.) part but dst if broad/multicast is set in the other file (2.). I argue for consistency, i.e., set broadcast address in GNRC too, and further get rid of this broadcast flag setting and later unsetting again.
There was a problem hiding this comment.
I have to confess with "Because the driver is not 'supposed' to know […]" I was mixing terms myself. I meant "the glue code" gnrc_netdev2_ieee802154 ^^".
I argue for consistency, i.e., set broadcast address in GNRC too, and further get rid of this broadcast flag setting and later unsetting again.
Might indeed be a good idea!
They are not the same thing. |
|
@miri64 thx for clarification! |
Yes! I'm working on an alternative PR now to finally clear (and clean) things up! ;-). It was used by |
|
I --HAW Hamburg, that is-- now have several |
See #5897 |
To clarify this: I ran into this error in #5897, too. It was happening with your PR due to the broadcast behavior @smlng reported here (PAN were equal, but broadcast flag was set => dst_len == 0 => PAN compression wasn't applied). Since my PAN compression handling looked a little bit different then, I just included the change into #5897 instead of basing that PR on this one. |
PAN compression wasn't applied => header was longer than expected => return address of test function was overridden => stack smashing detected ;-). |
7 participants
According to section 7.5.6.1 of 802.15.4-2003, packets destined for
the same PAN from which they were sent must have their intra-PAN
flag set, and the source PAN omitted from the transmitted frame.