sys/shell/commands/gnrc_icmpv6_echo: test for ICMPv6 reply corruption

[benpicco]

Contribution description

The Linux ping utility has the nice feature that fills the ICMPv6 request payload with a pattern payload_index & 0xFF.
Then the ICMPv6 response payload is checked to verify that the pattern is still intact.

This way corrupted messages can be detected.
In the past that revealed some 6lo-fragmentation bugs in Linux when corrupted replies arrived.

This feature is also useful for RIOT, so implement it in RIOTs ping command.

Testing procedure

ping should still work with varying lengths.
If packet corruption happened on the data path, this should now be reported.

Issues/PRs references

[miri64]

Mh, is that feature optional on Linux? Because, as documented,

RIOT/sys/shell/commands/sc_gnrc_icmpv6_echo.c

Lines 15 to 19 in e63927b

	* This implementation oriented itself on the [version by Mike
	* Muuss](http://ftp.arl.army.mil/~mike/ping.html) which was published under
	* public domain. The state-handling and duplicate detection was inspired by the
	* ping version of [inetutils](://www.gnu.org/software/inetutils/), which was
	* published under GPLv3

the implementation is partly based on the Linux implementation.

[miri64]

(So my actual question is: should we make this optional using a command-line flag?)

[miri64]

At any rate this most likely will need an adaptation of the corresponding ShellInteraction

RIOT/dist/pythonlibs/riotctrl_shell/gnrc.py

Lines 485 to 499 in 5f7a7bc

	class GNRCICMPv6Echo(ShellInteraction):
	@ShellInteraction.check_term
	def ping6(self, hostname, count=3, interval=1000, packet_size=4,
	hop_limit=None, timeout=1000, async_=False):
	cmd = "ping6 {hostname} -c {count} -i {interval} " \
	"-s {packet_size} -W {timeout}" \
	.format(hostname=hostname, count=count, interval=interval,
	packet_size=packet_size, timeout=timeout)
	if hop_limit is not None:
	cmd += " -h {hop_limit}".format(hop_limit=hop_limit)
	# wait a second longer than all pings
	cmd_timeout = ((timeout / 1000) * count) + 1
	return self.cmd(cmd, timeout=cmd_timeout, async_=async_)

And its corresponding parser

RIOT/dist/pythonlibs/riotctrl_shell/gnrc.py

Lines 122 to 137 in 5f7a7bc

	res = {}
	for line in cmd_output.splitlines():
	if "stats" not in res: # If final stats were not found yet
	m = self.c_reply.match(line)
	if m is not None:
	self._add_reply(res, m.groupdict())
	continue
	m = self.c_stats.match(line)
	if m is not None:
	self._set_stats(res, m.groupdict())
	continue
	else:
	m = self.c_rtts.match(line)
	if m is not None:
	self._set_rtts(res, m.groupdict())
	return res

To deal with the new behavior.

[benpicco]

What would we gain by making this optional? The Linux ping utility will always do the corruption check.
For ShellInteraction, would I just add reply["corrupt"] = 1 in case the error line is printed?

[miri64]

What would we gain by making this optional? The Linux ping utility will always do the corruption check.

You wouldn't need to adapt the riotctrl stuff and not break the tests ;-).

For ShellInteraction, would I just add reply["corrupt"] = 1 in case the error line is printed?

The ShellInteraction itself probably doesn't need any adoption, since as far as I can see you don't change the command parameters. The parser, however, needs adoption. Since you provide offset information and stuff like that, I would prefer, if that information would be provided in the reply dictionary as well. E.g.

reply["corrupt"] = {"bytes": bytes, "offset": offset}

[benpicco]

I did a thing.
I don't know how to test this though, so it's untested.

(the reply can either be truncated or corrupted. A truncated reply is not checked for corruption as it's already damaged)

[benpicco]

How can I test riotctrl_shell/gnrc.py?

[miri64]

Type tox when you are in the dist/pythonlibs/riotctrl_shell/ directory (you might need to install tox first).

[miri64]

(see also the tools-test / python-tests Github Actions Workflow).

[benpicco]

Thank you for the hint with tox! Now I figured out what's going on here.
I re-wrote the error output to be in-line so there is only a single line of output / reply for easier parsing.

[miri64]

Thank you for the hint with tox! Now I figured out what's going on here.
I re-wrote the error output to be in-line so there is only a single line of output / reply for easier parsing.

Now only the flake8 linter is complaining :-).

[benpicco]

Heh, I thought we bumped the limit to 100 characters, but only for C code it seems.

[miri64]

Heh, I thought we bumped the limit to 100 characters, but only for C code it seems.

For C code the coding conventions also still say 80 chars as soft target.

[miri64]

(but we actually aim for PEP8 compliance in the pythonlibs (not in the test scripts though))

[miri64]

now is now much later fetched. Can this be a problem? Ideally this should come directly after the receive and maybe provided as a parameter to _check_payload().

[benpicco]

I moved the time capture to the _handle_reply() call so we get the time right after msg_receive() returned.

[miri64]

Using ping6 -c 100 -i 100 ... I get similar results for both

master

--- fe80::dc1a:a8ff:fe09:45b3 PING statistics ---
100 packets transmitted, 100 packets received, 0% packet loss
round-trip min/avg/max = 0.158/0.438/0.781 ms

This PR

--- fe80::dc1a:a8ff:fe09:45b3 PING statistics ---
100 packets transmitted, 100 packets received, 0% packet loss
round-trip min/avg/max = 0.135/0.449/1.470 ms

Tox for dist/pythonlibs/riotctrl_shell still passes. Did not bother to test the output for corrupted and truncated replies. Maybe something with scapy can be done there.

[miri64]

ACK

[miri64]

Just to be absolutely sure there is not a problem introduced with riotctrl_shell, I also started a release-test Github Action release-tests Github Action (edit: link changed to build correct commit) that uses that python library. If it succeeds (or fails with unrelated errors) we are good to go, I think.

[miri64]

Just to be absolutely sure there is not a problem introduced with riotctrl_shell, I also started a release-test Github Action release-tests Github Action (edit: link changed to build correct commit) that uses that python library. If it succeeds (or fails with unrelated errors) we are good to go, I think.

Only the usual suspects (LoRaWAN and a test where the nodes are picked way to far apart) fail. So gooo!