ng_icmpv6_echo: fix unexpected parameter problem #3311

miri64 · 2015-07-03T15:08:17Z

Previously it could happen, that the pinging node had send more then one
packet before the reply was received. This would cause the sequence
number to be bigger than expected on receive.

This fixes this problem by introducing a window of expected echo sequence
numbers.

miri64 · 2015-07-03T15:11:13Z

I chose to use id for the payload from now on, since seq now is a little bit more complicated. id should not cause a problem, since the shell command assumes that the shell is blocked anyways, so id does not change during one execution.

miri64 · 2015-07-03T15:41:13Z

@PeterKietzmann noted offline that maybe @emmanuelsearch or @OlegHahm should have a look at this, too. For this reason I also reassign to @OlegHahm.

PeterKietzmann · 2015-07-03T16:07:52Z

I will test this asap. Why I proposed to get an opinion from Oleg or Emmanuel: I think the window will fix the sequence bug. But I don't know if it's legit to do so. Thinking theoretical, not practical, I wonder what happens if you try to send 5mio pings (I know it's not realistic or feasible for other components). The offset will theoretically exceed the windows bounds at any time. So one has to select bounds according to a proposed scenario. This selection feels somehow random to me. You get what I'm trying to say :-) ?

PeterKietzmann · 2015-07-04T08:17:33Z

I can confirm that this PR fixes the unexpected parameters bug. Still I get packet buffer full behaviour. Details in the offline spreadsheet.

miri64 · 2015-07-04T12:57:42Z

Wait a minute… the event loop in this command is: send packet, wait for reply, send packet, wait for reply, …
So how does it happen, that an older packet arrives, when it should be handled before the next packet goes out? My suggestion: there is a duplicate in the message queue, which would also explain a double release on a packet. Maybe related to #1811?

miri64 · 2015-07-04T13:30:23Z

I can confirm that this PR fixes the unexpected parameters bug. Still I get packet buffer full behaviour. Details in the offline spreadsheet.

I see the same btw. Yesterday I saw crashes, when dereferencing 0x80789f2 <_buf+210> (which became 0x7), but since my computer crashed afterwards due to heavy load (which it does from time to time because it's not that new anymore), I'm not sure if this result was valid.

OlegHahm · 2015-07-06T12:41:06Z

id should not cause a problem, since the shell command assumes that the shell is blocked anyways, so id does not change during one execution.

But couldn't this cause a problem if pinging is not performed from the shell?

OlegHahm · 2015-07-06T12:42:31Z

So how does it happen, that an older packet arrives, when it should be handled before the next packet goes out?

This doesn't explain the bug, but shouldn't the stack be able to deal with out-of-order packet reception and duplicates?

miri64 · 2015-07-06T12:53:14Z

But couldn't this cause a problem if pinging is not performed from the shell?

There is no implementation for that… so what do you mean by that.

This doesn't explain the bug, but shouldn't the stack be able to deal with out-of-order packet reception and duplicates?

That's the task of individual layers like TCP. Since we don't use them for ping, the ping application itself is tasked with keeping track of this.

OlegHahm · 2015-07-06T12:56:01Z

There is no implementation for that… so what do you mean by that.

That this is not implemented (yet) in RIOT master, doesn't mean that it will never happen. Someone might even started to do so in her or his application. I'm just wondering if you took this case into consideration.

OlegHahm · 2015-07-06T12:58:01Z

That's the task of individual layers like TCP. Since we don't use them for ping, the ping application itself is tasked with keeping track of this.

I don't understand this. How can any application(???) make sure that no duplicates are generate or re-ordering happen along the route. Or are you saying that the application can/should be able to handle these cases?

miri64 · 2015-07-06T13:14:32Z

That this is not implemented (yet) in RIOT master, doesn't mean that it will never happen. Someone might even started to do so in her or his application. I'm just wondering if you took this case into consideration.

But than that implementation should also handle this cases.

I don't understand this. How can any application(???) make sure that no duplicates are generate or re-ordering happen along the route. Or are you saying that the application can/should be able to handle these cases?

Not any application. A ping application.

I think we are talking past each other here... I'm not really sure what your comments are targeting, since it sound to like you want to put complexities that belong into a ping application into the ICMPv6 implementation of the stack.

OlegHahm · 2015-07-06T13:19:52Z

Ah, maybe. I wasn't really looking at the code and just assuming we were talking about ICMPv6 echo here because of the title while this PR is actually about the ping shell command.

However, just to make sure: the current ping application (aka the ping shell command) is capable of dealing with duplicates and out-of-order reception, right?

miri64 · 2015-07-06T13:32:55Z

However, just to make sure: the current ping application (aka the ping shell command) is capable of dealing with duplicates and out-of-order reception, right?

With this PR, yes.

miri64 · 2015-07-06T13:33:53Z

(at least in a window of 2¹⁶ - 1 packets ;-), to address @PeterKietzmann's concerns)

OlegHahm · 2015-07-07T16:26:32Z

sys/shell/commands/sc_icmpv6_echo.c

            i += 2;
        }
        else {
-            payload[i++] = (uint8_t)(seq & 0xff);
+            payload[i++] = (uint8_t)(id & 0xff);
        }
    }

    if (i > payload_len) {


How can this be true?

Edit: Understood it - but would be worth a comment.

OlegHahm · 2015-07-07T17:02:08Z

ACK

OlegHahm · 2015-07-07T23:11:56Z

Please squash

Previously it could happen, that the pinging node had send more then one packet before the reply was received. This would cause the sequence number to be bigger than expected on receive. This fixes this problem by introducing a window of expected echo sequence numbers.

miri64 · 2015-07-07T23:47:05Z

Squashed.

PeterKietzmann · 2015-07-08T07:10:13Z

As @OlegHahm ACKed gave his ACK and the window fixed some issues, I'll click the button now.

…ted-parameter-problem ng_icmpv6_echo: fix unexpected parameter problem

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) NSTF labels Jul 3, 2015

miri64 assigned PeterKietzmann Jul 3, 2015

miri64 added this to the Release 2015.06 milestone Jul 3, 2015

miri64 assigned OlegHahm and unassigned PeterKietzmann Jul 3, 2015

OlegHahm reviewed Jul 7, 2015
View reviewed changes

OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jul 7, 2015

OlegHahm mentioned this pull request Jul 7, 2015

ng_pktbuf: fix alignment overwrite issue #3316

Merged

miri64 force-pushed the ng_icmpv6_echo/fix/unexpected-parameter-problem branch from 9d00069 to 539dd57 Compare July 7, 2015 23:46

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: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Jul 7, 2015

PeterKietzmann added a commit that referenced this pull request Jul 8, 2015

Merge pull request #3311 from authmillenon/ng_icmpv6_echo/fix/unexpec…

8456c9c

…ted-parameter-problem ng_icmpv6_echo: fix unexpected parameter problem

PeterKietzmann merged commit 8456c9c into RIOT-OS:master Jul 8, 2015

miri64 deleted the ng_icmpv6_echo/fix/unexpected-parameter-problem branch July 9, 2015 10:38

miri64 added the Area: network Area: Networking label Sep 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ng_icmpv6_echo: fix unexpected parameter problem #3311

ng_icmpv6_echo: fix unexpected parameter problem #3311

miri64 commented Jul 3, 2015

miri64 commented Jul 3, 2015

miri64 commented Jul 3, 2015

PeterKietzmann commented Jul 3, 2015

PeterKietzmann commented Jul 4, 2015

miri64 commented Jul 4, 2015

miri64 commented Jul 4, 2015

OlegHahm commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

miri64 commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

miri64 commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

miri64 commented Jul 6, 2015

miri64 commented Jul 6, 2015

OlegHahm Jul 7, 2015

miri64 Jul 7, 2015

OlegHahm commented Jul 7, 2015

OlegHahm commented Jul 7, 2015

miri64 commented Jul 7, 2015

PeterKietzmann commented Jul 8, 2015

ng_icmpv6_echo: fix unexpected parameter problem #3311

ng_icmpv6_echo: fix unexpected parameter problem #3311

Conversation

miri64 commented Jul 3, 2015

miri64 commented Jul 3, 2015

miri64 commented Jul 3, 2015

PeterKietzmann commented Jul 3, 2015

PeterKietzmann commented Jul 4, 2015

miri64 commented Jul 4, 2015

miri64 commented Jul 4, 2015

OlegHahm commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

miri64 commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

miri64 commented Jul 6, 2015

OlegHahm commented Jul 6, 2015

miri64 commented Jul 6, 2015

miri64 commented Jul 6, 2015

OlegHahm Jul 7, 2015

Choose a reason for hiding this comment

miri64 Jul 7, 2015

Choose a reason for hiding this comment

OlegHahm commented Jul 7, 2015

OlegHahm commented Jul 7, 2015

miri64 commented Jul 7, 2015

PeterKietzmann commented Jul 8, 2015