shell_commands: check ICMPv6 echo reply sender #11933

miri64 · 2019-07-30T10:48:16Z

Contribution description

Fix for #11519 by checking the source address when an echo reply is
received by the ping6 command.

Testing procedure

See #11519 for testing procedures

Issues/PRs references

Fix for #11519.

Fixes RIOT-OS#11519 by checking the source address when an echo reply is received by the `ping6` command.

miri64 · 2019-08-05T09:13:40Z

Ping @maribu

maribu · 2019-08-06T15:40:53Z

I do not have the nRF52840DK board at hand. I'll test as soon as possible.

miri64 · 2019-08-06T15:44:15Z

I do not have the nRF52840DK board at hand. I'll test as soon as possible.

Huh? Did I miss something? Why is an nRF52840DK required for that? o.O

maribu · 2019-08-06T15:44:27Z

sys/shell/commands/sc_gnrc_icmpv6_echo.c

@@ -338,7 +338,8 @@ static void _print_reply(_ping_data_t *data, gnrc_pktsnip_t *icmpv6,
        uint16_t recv_seq;

        /* not our ping */
-        if (byteorder_ntohs(icmpv6_hdr->id) != data->id) {
+        if ((byteorder_ntohs(icmpv6_hdr->id) != data->id) &&
+            !ipv6_addr_equal(from, &data->host)) {


Maybe it would make sense to print a short line to indicate that potential mischief happened? E.g. the potential driver issue with either at85rf2xx or the nrf52 802.15.4 I noticed would no longer be detectable if the unexpected pongs are silently dropped.

Mh... The original IP implementation does not seem to care about this + it could be legitimate (e.g. when two threads pinging at the same time for some reason) that a packet that holds this condition arrives here. What is the potential driver issue again?

fjmolinas · 2019-08-09T09:58:22Z

Tested on samr21-xpro and pba-d-01-kw2x

Modify device B to send multiple responses over some extended period of time (e.g. seconds)

This is the brute change I made in the receiver:

diff --git a/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c b/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c
index bc12e2308..ebace70ba 100644
--- a/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c
+++ b/sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c
@@ -27,6 +27,8 @@
 #include "od.h"
 #include "utlist.h"
 
+#include "xtimer.h"
+
 #include "net/gnrc/icmpv6.h"
 #include "net/gnrc/icmpv6/echo.h"
 
@@ -90,6 +92,12 @@ void gnrc_icmpv6_demux(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt)
             DEBUG("icmpv6: handle echo request.\n");
             gnrc_icmpv6_echo_req_handle(netif, (ipv6_hdr_t *)ipv6->data,
                                         (icmpv6_echo_t *)hdr, icmpv6->size);
+            xtimer_usleep(5000);
+            gnrc_icmpv6_echo_req_handle(netif, (ipv6_hdr_t *)ipv6->data,
+                            (icmpv6_echo_t *)hdr, icmpv6->size);
+            xtimer_usleep(5000);
+            gnrc_icmpv6_echo_req_handle(netif, (ipv6_hdr_t *)ipv6->data,
+                            (icmpv6_echo_t *)hdr, icmpv6->size);
             break;
 #endif

On A: ping6 node B more than once

I seem to be getting the same result as in #11519..

2019-08-09 11:48:10,053 - INFO #  ping6 fe80::d1c1:6d4e:ab6a:1336
2019-08-09 11:48:10,062 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-56 dBm time=59.862 ms
2019-08-09 11:48:10,070 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-56 dBm time=67.775 ms (DUP!)
2019-08-09 11:48:10,091 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-57 dBm time=26.938 ms
2019-08-09 11:48:10,100 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-57 dBm time=37.689 ms (DUP!)
2019-08-09 11:48:10,108 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-56 dBm time=47.122 ms (DUP!)
2019-08-09 11:48:10,117 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-57 dBm time=55.603 ms (DUP!)
2019-08-09 11:48:10,125 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-57 dBm time=64.086 ms (DUP!)
2019-08-09 11:48:11,090 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-57 dBm time=27.580 ms
2019-08-09 11:48:11,090 - INFO # 
2019-08-09 11:48:11,096 - INFO # --- fe80::d1c1:6d4e:ab6a:1336 PING statistics ---
2019-08-09 11:48:11,103 - INFO # 2 packets transmitted, 3 packets received, 5 duplicates, 2147483598% packet loss
2019-08-09 11:48:11,107 - INFO # round-trip min/avg/max = 26.938/48.331/67.775 ms

It also somehow seem to be getting confused when pinging to an unreachable address

On A: ping6 node B and immediately afterwards ping6 some unreachable/unassigned IPv6 address

2019-08-09 11:52:53,906 - INFO # RIOT network stack example application
2019-08-09 11:52:53,908 - INFO # All up, running the shell now
> ping6 fe80::d1c1:6d4e:ab6a:1336
2019-08-09 11:52:55,274 - INFO #  ping6 fe80::d1c1:6d4e:ab6a:1336
2019-08-09 11:52:55,314 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-52 dBm time=28.524 ms
2019-08-09 11:52:55,323 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-52 dBm time=39.275 ms (DUP!)
2019-08-09 11:52:55,331 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-52 dBm time=48.726 ms (DUP!)
2019-08-09 11:52:55,340 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-52 dBm time=57.208 ms (DUP!)
2019-08-09 11:52:55,348 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-52 dBm time=65.691 ms (DUP!)
ping6 fe80::d1c1:6d4e:ab6a:13322019-08-09 11:52:56,310 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-52 dBm time=26.619 ms
2019-08-09 11:52:56,320 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-52 dBm time=37.275 ms (DUP!)
2019-08-09 11:52:56,328 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-52 dBm time=46.726 ms (DUP!)
2019-08-09 11:52:56,337 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-52 dBm time=55.208 ms (DUP!)
2019-08-09 11:52:56,345 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-52 dBm time=63.691 ms (DUP!)

2019-08-09 11:52:57,312 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=2 ttl=64 rssi=-48 dBm time=28.531 ms
2019-08-09 11:52:57,312 - INFO # 
2019-08-09 11:52:57,317 - INFO # --- fe80::d1c1:6d4e:ab6a:1336 PING statistics ---
2019-08-09 11:52:57,323 - INFO # 3 packets transmitted, 3 packets received, 8 duplicates, 0% packet loss
2019-08-09 11:52:57,328 - INFO # round-trip min/avg/max = 26.619/45.224/65.691 ms
> 2019-08-09 11:52:57,331 - INFO #  ping6 fe80::d1c1:6d4e:ab6a:1332
2019-08-09 11:52:57,340 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=2 ttl=64 rssi=-47 dBm time=60.216 ms
2019-08-09 11:52:57,349 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=2 ttl=64 rssi=-47 dBm time=68.088 ms (DUP!)
2019-08-09 11:53:00,330 - INFO # 
2019-08-09 11:53:00,334 - INFO # --- fe80::d1c1:6d4e:ab6a:1332 PING statistics ---
2019-08-09 11:53:00,340 - INFO # 3 packets transmitted, 1 packets received, 1 duplicates, 66% packet loss
2019-08-09 11:53:00,344 - INFO # round-trip min/avg/max = 60.216/64.152/68.088 ms

miri64 · 2019-08-09T10:02:45Z

~~The unreachable address problem is a bug separate from the one fixed here. It is addressed in #11938.~~

miri64 · 2019-08-09T10:07:06Z

Sorry... I meant the problem of having late pings come in from a previous ping6 call to the same address. That is what is fixed in #11938 and that is why you get the same output as in #11519 as you reported.

fjmolinas · 2019-08-09T10:20:51Z

Sorry... I meant the problem of having late pings come in from a previous ping6 call to the same address. That is what is fixed in #11938 and that is why you get the same output as in #11519 as you reported.

So if I understand right On A: ping6 node B and immediately afterwards ping6 some unreachable/unassigned IPv6 address should be fixed by this PR, which I wasn't seeing anyway (Or Am I still wrong In my understanding of the issue?)

When testing this PR rebased on top of #11938 the bug was no longer present.

2019-08-09 12:12:20,033 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2019-08-09 12:12:37,121 - INFO # main(): This is RIOT! (Version: 2019.10-devel-160-g7a14-pr-11938)
2019-08-09 12:12:37,124 - INFO # RIOT network stack example application
2019-08-09 12:12:37,127 - INFO # All up, running the shell now
> ping6 fe80::d1c1:6d4e:ab6a:1336
2019-08-09 12:12:39,573 - INFO #  ping6 fe80::d1c1:6d4e:ab6a:1336
2019-08-09 12:12:39,616 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=32.168 ms
2019-08-09 12:12:39,624 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=42.902 ms (DUP!)
2019-08-09 12:12:39,633 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=51.383 ms (DUP!)
2019-08-09 12:12:39,641 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=59.864 ms (DUP!)
ping6 fe80::d1c1:6d4e:ab6a:aaaa
2019-08-09 12:12:40,609 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-50 dBm time=26.627 ms
2019-08-09 12:12:40,618 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-50 dBm time=37.294 ms (DUP!)
2019-08-09 12:12:40,627 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-50 dBm time=46.726 ms (DUP!)
2019-08-09 12:12:40,635 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-50 dBm time=55.208 ms (DUP!)
2019-08-09 12:12:40,644 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-50 dBm time=63.691 ms (DUP!)
2019-08-09 12:12:41,610 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=2 ttl=64 rssi=-49 dBm time=28.532 ms
2019-08-09 12:12:41,610 - INFO # 
2019-08-09 12:12:41,615 - INFO # --- fe80::d1c1:6d4e:ab6a:1336 PING statistics ---
2019-08-09 12:12:41,622 - INFO # 3 packets transmitted, 3 packets received, 7 duplicates, 0% packet loss
2019-08-09 12:12:41,626 - INFO # round-trip min/avg/max = 26.627/44.439/63.691 ms
> 2019-08-09 12:12:41,630 - INFO #  ping6 fe80::d1c1:6d4e:ab6a:aaaa
2019-08-09 12:12:44,628 - INFO # 
2019-08-09 12:12:44,632 - INFO # --- fe80::d1c1:6d4e:ab6a:aaaa PING statistics ---
2019-08-09 12:12:44,637 - INFO # 3 packets transmitted, 0 packets received, 100% packet loss
> ping6 fe80::d1c1:6d4e:ab6a:1336
2019-08-09 12:12:46,797 - INFO #  ping6 fe80::d1c1:6d4e:ab6a:1336
2019-08-09 12:12:46,836 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=28.207 ms
2019-08-09 12:12:46,846 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=38.949 ms (DUP!)
2019-08-09 12:12:46,854 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=48.402 ms (DUP!)
2019-08-09 12:12:46,863 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=56.884 ms (DUP!)
2019-08-09 12:12:46,871 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=0 ttl=64 rssi=-50 dBm time=65.366 ms (DUP!)
ping6 fe80::d1c1:6d4e:ab6a:aaaa
2019-08-09 12:12:47,834 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-49 dBm time=27.277 ms
2019-08-09 12:12:47,843 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-49 dBm time=37.839 ms (DUP!)
2019-08-09 12:12:47,852 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-49 dBm time=47.207 ms (DUP!)
2019-08-09 12:12:47,860 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-49 dBm time=55.689 ms (DUP!)
2019-08-09 12:12:47,869 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=1 ttl=64 rssi=-49 dBm time=64.171 ms (DUP!)
2019-08-09 12:12:48,834 - INFO # 12 bytes from fe80::d1c1:6d4e:ab6a:1336: icmp_seq=2 ttl=64 rssi=-49 dBm time=28.859 ms
2019-08-09 12:12:48,835 - INFO # 
2019-08-09 12:12:48,839 - INFO # --- fe80::d1c1:6d4e:ab6a:1336 PING statistics ---
2019-08-09 12:12:48,846 - INFO # 3 packets transmitted, 3 packets received, 8 duplicates, 0% packet loss
2019-08-09 12:12:48,850 - INFO # round-trip min/avg/max = 27.277/45.350/65.366 ms
> 2019-08-09 12:12:48,854 - INFO #  ping6 fe80::d1c1:6d4e:ab6a:aaaa
2019-08-09 12:12:51,851 - INFO # 
2019-08-09 12:12:51,855 - INFO # --- fe80::d1c1:6d4e:ab6a:aaaa PING statistics ---
2019-08-09 12:12:51,860 - INFO # 3 packets transmitted, 0 packets received, 100% packet loss
2019-08-09 12:12:54,794 - INFO # Exiting Pyterm

miri64 · 2019-08-09T11:06:24Z

So if I understand right On A: ping6 node B and immediately afterwards ping6 some unreachable/unassigned IPv6 address should be fixed by this PR, which I wasn't seeing anyway (Or Am I still wrong In my understanding of the issue?)

Nope, everything right :-)

fjmolinas

Tested this was fixing the Issue in #11519 now that also #11938 got merged. ACK on my side, maybe @maribu wants to verify it too before merging? since he opened the issue.

miri64 · 2019-08-09T13:50:56Z

If the PR improves things in your opinion and don't make things worse, how about we merge now and then @maribu can evaluate if #11519 can be closed as a result of this?

fjmolinas · 2019-08-09T13:52:56Z

If the PR improves things in your opinion and don't make things worse, how about we merge now and then @maribu can evaluate if #11519 can be closed as a result of this?

+1, In that case remove Fixes #11519. from the PR description so it doesn't get closed automatically.

miri64 · 2019-08-09T13:54:53Z

reworded "Fixes" to "Fix for". This way it is ignored by GitHub ;-).

fjmolinas · 2019-08-09T14:27:35Z

GO!

benpicco · 2019-09-10T17:33:24Z

What about broadcast pings?
ping6 ff02::1 should receive all the PONGs.

shell_commands: check ICMPv6 echo reply sender

4a723fe

Fixes RIOT-OS#11519 by checking the source address when an echo reply is received by the `ping6` command.

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 30, 2019

miri64 requested a review from maribu July 30, 2019 10:48

miri64 mentioned this pull request Jul 30, 2019

shell/ping6: Incorrect handling of unexpected pongs #11519

Closed

maribu reviewed Aug 6, 2019

View reviewed changes

fjmolinas approved these changes Aug 9, 2019

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 9, 2019

fjmolinas added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Aug 9, 2019

fjmolinas merged commit 56a6e5d into RIOT-OS:master Aug 9, 2019

miri64 deleted the shell/fix/check-sender branch August 9, 2019 14:28

fjmolinas mentioned this pull request Sep 3, 2019

shell/commands: fix, only accept proper pong response to icmpv6_echo #12159

Merged

benpicco mentioned this pull request Sep 10, 2019

Revert "shell/commands: fix, only accept proper pong response" #12195

Closed

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell_commands: check ICMPv6 echo reply sender #11933

shell_commands: check ICMPv6 echo reply sender #11933

miri64 commented Jul 30, 2019 •

edited

Loading

miri64 commented Aug 5, 2019

maribu commented Aug 6, 2019

miri64 commented Aug 6, 2019

maribu Aug 6, 2019

miri64 Aug 6, 2019

fjmolinas commented Aug 9, 2019

miri64 commented Aug 9, 2019 •

edited

Loading

miri64 commented Aug 9, 2019

fjmolinas commented Aug 9, 2019

miri64 commented Aug 9, 2019

fjmolinas left a comment

miri64 commented Aug 9, 2019

fjmolinas commented Aug 9, 2019

miri64 commented Aug 9, 2019

fjmolinas commented Aug 9, 2019

benpicco commented Sep 10, 2019

shell_commands: check ICMPv6 echo reply sender #11933

shell_commands: check ICMPv6 echo reply sender #11933

Conversation

miri64 commented Jul 30, 2019 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

miri64 commented Aug 5, 2019

maribu commented Aug 6, 2019

miri64 commented Aug 6, 2019

maribu Aug 6, 2019

Choose a reason for hiding this comment

miri64 Aug 6, 2019

Choose a reason for hiding this comment

fjmolinas commented Aug 9, 2019

miri64 commented Aug 9, 2019 • edited Loading

miri64 commented Aug 9, 2019

fjmolinas commented Aug 9, 2019

miri64 commented Aug 9, 2019

fjmolinas left a comment

Choose a reason for hiding this comment

miri64 commented Aug 9, 2019

fjmolinas commented Aug 9, 2019

miri64 commented Aug 9, 2019

fjmolinas commented Aug 9, 2019

benpicco commented Sep 10, 2019

miri64 commented Jul 30, 2019 •

edited

Loading

miri64 commented Aug 9, 2019 •

edited

Loading