Handle IPv4 broadcast addresses #10816
Conversation
jukkar
force-pushed
the
bug-10780-10782-ipv4-bcast
branch
from
3354cf8 to
1e6ad51
Compare
Codecov Report
@@ Coverage Diff @@
## master #10816 +/- ##
==========================================
+ Coverage 53.29% 53.32% +0.03%
==========================================
Files 215 215
Lines 25854 25887 +33
Branches 5693 5705 +12
==========================================
+ Hits 13778 13804 +26
- Misses 9761 9763 +2
- Partials 2315 2320 +5
Continue to review full report at Codecov.
|
tbursztyka
left a comment
tbursztyka
left a comment
There was a problem hiding this comment.
let's have consistent names.
subsys/net/ip/net_if.c
Outdated
There was a problem hiding this comment.
s/is_ipv4_broadcast_address/ipv4_is_address_broadcast
include/net/net_ip.h
Outdated
There was a problem hiding this comment.
s/net_is_ipv4_addr_bcast/net_ipv4_is_addr_bcast
There was a problem hiding this comment.
Lets not change this in this case, this function name is following the same format as the other net_is_ipv4_*() functions in that file.
There was a problem hiding this comment.
his function name is following the same format as the other net_is_ipv4_*() functions in that file.
Would be good to rename them all then (in another PR).
include/net/net_ip.h
Outdated
There was a problem hiding this comment.
s/net_if_ipv4_addr_is_bcast/net_if_ipv4_is_addr_bcast
pfalcon
left a comment
pfalcon
left a comment
There was a problem hiding this comment.
Store the netmask in host byte order, and then use that ordering
consistently in the code
Sorry, what do you mean? struct in_addr should contain addresses in the standard way, and if that's network order, it should be like that, or unimaginable mess will ensue.
This is done like this so that we do not
need to convert its value to host byte ordering multiple times.
One good question from rubber ducky is "why do we convert it multiple times?" and "why do we convert it at all?"
Obviously the patch text is misleading in this case, I will rework that in next version.
I am not sure if I follow your whacky comment here. Anyway, I removed various ntohl() calls in the patch in order to make the code more sane. |
Remove extra ntohl() calls when checking IPv4 address against a subnet address. Convert also the IPv4 address to be const as the netmask related functions do not change its value. Signed-off-by: Jukka Rissanen <[email protected]>
Add utility function that returns true if given IPv4 address is a broadcast address. This will be used in later commits to check received packet IPv4 source and destination addresses. Signed-off-by: Jukka Rissanen <[email protected]>
Make sure that IPv4 broadcast address checks work as expected. Signed-off-by: Jukka Rissanen <[email protected]>
jukkar
force-pushed
the
bug-10780-10782-ipv4-bcast
branch
from
1e6ad51 to
8ee10ee
Compare
subsys/net/ip/icmpv4.c
Outdated
There was a problem hiding this comment.
Why do we have 2 duplicate NET_DBG calls here? The only reason I can see is that you do don't want to break the condition in the if in 2 lines. And that's bad when codestyle issues direct us to write "bad" code (and we should increase line length to avoid premature wrapping (and decrease indent size)).
There was a problem hiding this comment.
There are two different if() cases here
- if CONFIG_NET_ICMPV4_ACCEPT_BROADCAST is enabled AND if we do not have icmp echo-request message, then drop the msg
- if CONFIG_NET_ICMPV4_ACCEPT_BROADCAST is not enabled, then drop the message right away
I do not see the major issue here.
There was a problem hiding this comment.
Nothing major, just "2nd-level confusion": "Why that guy didn't write it in natural way:
if (!IS_ENABLED(CONFIG_NET_ICMPV4_ACCEPT_BROADCAST) || icmp_hdr.type != NET_ICMPV4_ECHO_REQUEST) {
NET_DBG("Dropping broadcast pkt");
goto drop;
}
"
"What does that guys miss? What do I miss?"
So, please either explain here (like, point to my mistake), add comment why instead of the natural if you duplicate code chunk twice, or ... just fix it.
There was a problem hiding this comment.
Sure, that can be written like that too. I will send a new version. BTW, next time you can suggest alternative way immediately, no need to ping-pong this around.
There was a problem hiding this comment.
Well, as I'm telling, this is a case of "2nd-order confusion". I know that you're experienced programmer, so start with the assumption that you didn't do that for a reason, which I don't understand.
Well, maybe it's not good approach every time, because it may lead to further levels of confusion.
pfalcon
left a comment
pfalcon
left a comment
There was a problem hiding this comment.
Thanks for the changes. Before approving, I'd like to clarify a case with duplicated logging calls.
If we receive an IPv4 that has broadcast destination address, then
properly handle it.
This means that for
* ICMPv4, if CONFIG_NET_ICMPV4_ACCEPT_BROADCAST is set (this is the
default value) and we receive echo-request then accept the packet.
Drop other ICMPv4 packets.
* TCP, drop the packet
* UDP, accept the packet if the destination address is the broadcast
address 255.255.255.255 or the subnet broadcast address.
Drop the packet if the packets broadcast address is not in our
configured subnet.
In sending side, make sure that we do not route broadcast address
IPv4 packets back to us. Also set Ethernet MAC destination address
properly if destination IPv4 address is broadcast one.
Fixes zephyrproject-rtos#10780
Signed-off-by: Jukka Rissanen <[email protected]>
Source address cannot be broadcast one so check it properly. Fixes zephyrproject-rtos#10782 Signed-off-by: Jukka Rissanen <[email protected]>
jukkar
force-pushed
the
bug-10780-10782-ipv4-bcast
branch
from
8ee10ee to
76c09bd
Compare
|
Updated the icmpv4 code and use simpler checks there. |
pfalcon
left a comment
pfalcon
left a comment
There was a problem hiding this comment.
Thanks!
@tbursztyka : Ping for re-review on your side.
5 participants
No description provided.