Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for multicast IPv4 TTL and IPv6 hop limit #65886

Merged
merged 18 commits into from Dec 4, 2023
Merged

Add support for multicast IPv4 TTL and IPv6 hop limit #65886

merged 18 commits into from Dec 4, 2023

Conversation

jukkar
Copy link
Member

@jukkar jukkar commented Nov 28, 2023 •
edited

Add option support for adjusting the IPv4 multicast time-to-live value. Add also tests for both unicast and multicast TTL settings.
Add also similar IPv6 hop limit option for both unicast and multicast IPv6 packets + tests.
Add checks and tests for IPv4 TTL 0 or IPv6 hop limit 0 when sending packet, the packet should dropped in this case.

Fixes #60299

pdgendt
pdgendt reviewed Nov 28, 2023
View reviewed changes
subsys/net/ip/Kconfig.ipv4 Outdated Show resolved Hide resolved
pdgendt
pdgendt reviewed Nov 28, 2023
View reviewed changes
subsys/net/ip/Kconfig.ipv4 Outdated Show resolved Hide resolved
@jukkar jukkar force-pushed the fix/60299-multicast-ttl branch 3 times, most recently from 33dc811 to 5d01aff Compare November 30, 2023 14:17
@jukkar jukkar marked this pull request as ready for review November 30, 2023 14:17
@jukkar
Copy link
Member Author

jukkar commented Nov 30, 2023

As a general note, I originally planned to implement the multicast TTL/hoplimit support only, but noticed that the IPv4 TTL and IPv6 hoplimit were a bit flaky, so the changes are more extensive. We were also missing tests for unicast versions, those + multicast ones are now implemented.

pdgendt
pdgendt previously approved these changes Nov 30, 2023
View reviewed changes
@jukkar jukkar force-pushed the fix/60299-multicast-ttl branch 2 times, most recently from 21d5a51 to a8294f6 Compare December 1, 2023 08:10
pdgendt
pdgendt previously approved these changes Dec 1, 2023
View reviewed changes
rlubos
rlubos previously approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some doc suggestions to consider, but overall looks good. Shall we mention the API changes in the changelog?

subsys/net/ip/Kconfig.ipv4 Outdated Show resolved Hide resolved
subsys/net/ip/Kconfig.ipv4 Outdated Show resolved Hide resolved
subsys/net/ip/Kconfig.ipv6 Outdated Show resolved Hide resolved
@jukkar
net: context: Add support for adjusting IPv4 multicast ttl
8557ce1 
Add option support for adjusting the IPv4 multicast
time-to-live value.

Fixes zephyrproject-rtos#60299

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
tests: net: udp: Add IPv4 time-to-live multicast tests
fe43481 
Make sure that setting IPv4 multicast TTL works as expected.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
net: Add support for adjusting IPv6 multicast hop limit
581657d 
Add option support for adjusting the IPv6 multicast
multicast hop limit value.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
tests: net: udp: Add IPv6 multicast hop limit tests
8b758bc 
Make sure that setting IPv6 multicast hop limit works as expected.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
net: if: Fix typo in IPv6 hop limit API name
e62b3e6 
The net_if_ipv6_set_hop_limit() API was missing the "_if_"
part in it. Fix this so that the network interface API is
consistent. The old function is deprecated and should not
be used. The old function is left to the code and it calls
the new properly named function.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
net: Add support for adjusting IPv6 unicast hop limit
a709c65 
Add option support for adjusting the IPv6 unicast
hop limit value.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
net: Refactor IP checks just before sending packets
1470c34 
* Check IPv4 TTL or IPv6 hop limit and drop the packet if
  the value is 0
* Check the IP addresses so that we do the loopback check
  at runtime if the packet is destined to loopback interface.
* Update the statistics properly for dropped packets.
* Do not update sent packets if we drop packets.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
tests: net: udp: Add IPv4 TTL 0 test
aecd8dc 
Make sure packet is dropped if TTL 0 packet is tried to
send.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
tests: net: udp: Add IPv6 hop limit 0 test
6bdf1f5 
Make sure packet is dropped if IPv6 hop limit 0 packet is
tried to send.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
tests: net: udp: Verify that packet is dropped from stats
601bb81 
Make sure that statistics is properly update for dropped
packets when IPv4 TTL is 0 or IPv6 hop limit is 0.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
net: dns: Do not pass 0 as TTL or hop limit
0b649d3 
We specifically set TTL/hoplimit to 1 for LLMNR,
but only want to set it if in that specific case.
We must not pass TTL/hoplimit value 0 as that would
cause the packet to be dropped.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
net: dns: responders: Set the multicast TTL or hoplimit
7522e4b 
We are creating a multicast address in mDNS or LLMNR
responder so set the TTL or hoplimit using the multicast
variant API.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
tests: net: dns-sd: Fix IPv4 TTL and IPv6 hop limit checking
712cfba 
We need to use multicast TTL/hoplimit instead of unicast one
in the test.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar jukkar dismissed stale reviews from rlubos and pdgendt via fbe3c71 December 1, 2023 12:55
@jukkar
Copy link
Member Author

jukkar commented Dec 1, 2023

Minor tweaks to kconfig file. Added also info to migration guide.

@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Dec 1, 2023
pdgendt
pdgendt previously approved these changes Dec 1, 2023
View reviewed changes
@jukkar jukkar force-pushed the fix/60299-multicast-ttl branch 2 times, most recently from e5d05fd to 5e710a9 Compare December 1, 2023 12:59
@jukkar
doc: migration-guide: 3.6: IPv4 TTL and IPv6 hoplimit changes
0da1561 
Add information about IPv4 TTL and IPv6 hoplimit changes
for unicast and multicast packets.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
doc: release: 3.6: IPv4 TTL and IPv6 hoplimit changes
bb027e5 
Add information about IPv4 TTL and IPv6 hoplimit changes
for unicast and multicast packets.

Signed-off-by: Jukka Rissanen <jukka.rissanen@nordicsemi.no>
@jukkar
Copy link
Member Author

jukkar commented Dec 1, 2023

Release and migration note changes in this version.

kartben
kartben reviewed Dec 1, 2023
View reviewed changes
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed missing Doxygen comments -- not making this blocking as I don't necessarily want to slow you guys down as I trust you'll address this in a follow-up PR, but would still be nice to have I think, especially since the networking headers are usually pretty well documented, so would be good to keep the track record ;-)

https://builds.zephyrproject.io/zephyr/pr/65886/api-coverage/zephyr/net/index.html

include/zephyr/net/net_context.h
@@ -703,6 +709,17 @@ static inline void net_context_set_ipv4_ttl(struct net_context *context,
context->ipv4_ttl = ttl;
}

static inline uint8_t net_context_get_ipv4_mcast_ttl(struct net_context *context)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are quite a few of these net_context_get|set_* functions (existing ones, and the ones you added) that are missing doxygen comments. Could you please add them? Should these be considered internal (I have no idea myself :)) you may consider not having the doxygen documentation (although it's always nice to have imo), and simply enclose them in @cond INTERNAL_HIDDEN sections.

Sorry that I didn't notice earlier as I really just paid attention to the migration guide changes when the bot added me.

include/zephyr/net/net_context.h
Comment on lines +351 to +358
struct {
uint8_t ipv6_hop_limit;
uint8_t ipv6_mcast_hop_limit;
};
struct {
uint8_t ipv4_ttl;
uint8_t ipv4_mcast_ttl;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to have doxygen comments here as well (again, unless this is something you want to hide from the public API).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will submit a follow up pr adding more comments. Thx!

@jukkar jukkar mentioned this pull request Dec 4, 2023
Merged
@henrikbrixandersen henrikbrixandersen merged commit ae8c328 into zephyrproject-rtos:main Dec 4, 2023
21 checks passed
@jukkar jukkar deleted the fix/60299-multicast-ttl branch December 4, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kartben kartben kartben left review comments

@pdgendt pdgendt pdgendt approved these changes

@rlubos rlubos rlubos approved these changes

@ssharks ssharks Awaiting requested review from ssharks

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

Assignees

@rlubos rlubos

Labels
area: Networking area: Sockets Networking sockets Release Notes To be mentioned in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set time-to-live for Multicasts
6 participants
@jukkar @kartben @pdgendt @rlubos @henrikbrixandersen @zephyrbot