Merge lp:~martin-lp/hipl/update_ack_handling into lp:hipl

[David Martin]

This is a resubmitted proposal:
addressed issues are fixed in revisions 6048-6052 in this branch.

Old proposal description:
This is a relatively small change but it addresses dealing with update
acknowledgments and it would be fatal to introduce bugs here.

The situation: previously only acknowledgments for the most recent sent
Update packet were accepted. This is a problem when both hosts initiate
an update at the same time.

The typical update sequence between host1 <-> host2 looks like this:
U1 (SEQ x) ->
U2 (SEQ y, ACK x) <-
U3 (ACK y) ->

If both hosts initiate simultaneously things go wrong:
U1 (SEQ x) ->
U1 (SEQ y) <-
U2 (SEQ x+1, ACK y) ->
U2 (SEQ y+1, ACK x) <-

if hosts only store the most recent sent update packet SEQ it breaks now
as host1 gets an ACK for x but expects x+1 (as this is the most recent).
As a consequence the packets get dropped and the update fails.

This branch introduces a lower boundary for outgoing Update packet IDs.
Valid received ACKs are the outstanding ones in
[lower_boundary, most recent sent].

Please check if I missed anything or if there is a flaw in the logic.
Actual tests or reproduction of the problem can be done with
tcpdump + wireshark or just by debug output.

- initiate BEX between two hosts (eg. by ping)
- run tools/hipconf manual-update at the same time on both hosts

[René Hummen (rene-hummen)]

review needs-fixing

I just have a few comments inline.

On 10.08.2011, at 13:16, David Martin wrote:
> David Martin has proposed merging lp:~martin-lp/hipl/update_ack_handling into lp:hipl.
>
> Requested reviews:
> HIPL core team (hipl-core)
>
> For more details, see:
> https://code.launchpad.net/~martin-lp/hipl/update_ack_handling/+merge/71027

> === modified file 'modules/update/hipd/update.c'
> --- modules/update/hipd/update.c 2011-07-18 16:41:13 +0000
> +++ modules/update/hipd/update.c 2011-08-10 11:16:05 +0000

> @@ -157,6 +158,41 @@
> }
>
> /**
> + * Verify the validity of the ACK ID of a received Update packet and update the
> + * range in which Update packets will be accepted accordingly.
> + *
> + * @param state The update state of the respective HA.
> + * @param ack_peer_update_id The received ACK ID.
> + *
> + * @return true if received ACK is valid
> + * false else
> + */
> +static bool check_and_update_ack_id_bounds(struct update_state *const state,
> + uint32_t ack_peer_update_id)

const uint32_t ack_peer_update_id would be more const correct.

> +{
> + HIP_ASSERT(state);
> +
> + /* two cases:
> + * oldest not yet acknowledged SEQ <= latest sent Update SEQ
> + * => received ACK must be in those bounds to be valid
> + * oldest not yet acknowledged SEQ > latest sent Update SEQ (counter overflow)
> + * => received ACK must be greater than oldest SEQ or smaller than latest
> + * sent Update ID to be valid
> + */

This comment is hard to understand without knowledge about your internal seq/ack data structure. Please describe how the two fields update_id_out_lower_bound and update_id_out are used in a normal update situation.

> + if ((state->update_id_out_lower_bound <= hip_update_get_out_id(state) &&
> + ack_peer_update_id >= state->update_id_out_lower_bound &&
> + ack_peer_update_id <= hip_update_get_out_id(state)) ||
> + (state->update_id_out_lower_bound > hip_update_get_out_id(state) &&
> + (ack_peer_update_id >= state->update_id_out_lower_bound ||
> + ack_peer_update_id <= hip_update_get_out_id(state)))) {
> + state->update_id_out_lower_bound = ack_peer_update_id;
> + return true;
> + } else {
> + return false;
> + }

Please break this down into a if () ... else if () ... else for higher readability.

> * Check if UPDATE sequence and acknowledgment numbers are as expected.
> *
> * @param packet_type the packet type
> @@ -216,12 +252,12 @@
> HIP_DEBUG("ACK parameter found with peer Update ID %u.\n",
> ack_peer_update_id);
>
> - // we only want acks for our most current update
> - if (ack_peer_update_id != hip_update_get_out_id(localstate)) {
> - HIP_DEBUG("Update ID (%u) in the ACK parameter is not "
> - "equal to the last outgoing Update ID (%u). "
> + if (!check_and_update_ack_id_bounds(localstate, ack_peer_update_id)) {
> + HIP_DEBUG("Update ID (%u) in the ACK parameter is not one of the "
> + "yet to be acknowledged Update IDs (%u-%u). ...

[René Hummen (rene-hummen)]

review: Needs Fixing

[Diego Biurrun (diego-biurrun)]

On Wed, Aug 10, 2011 at 11:52:31AM +0000, René Hummen wrote:
> review needs-fixing

If your intention is to set this by email, then you need to

a) Indent that statement with a space.
b) GPG-sign the email.

Diego

[David Martin (martin-lp)]

Hi,
thanks for you input René!

On Wed, Aug 10, 2011 at 1:52 PM, René Hummen <email address hidden> wrote:
> On 10.08.2011, at 13:16, David Martin wrote:
>> David Martin has proposed merging lp:~martin-lp/hipl/update_ack_handling into lp:hipl.
>> === modified file 'modules/update/hipd/update.c'
>> --- modules/update/hipd/update.c 2011-07-18 16:41:13 +0000
>> +++ modules/update/hipd/update.c 2011-08-10 11:16:05 +0000
>
>> @@ -157,6 +158,41 @@
>> }
>>
>> /**
>> + * Verify the validity of the ACK ID of a received Update packet and update the
>> + * range in which Update packets will be accepted accordingly.
>> + *
>> + * @param state The update state of the respective HA.
>> + * @param ack_peer_update_id The received ACK ID.
>> + *
>> + * @return true if received ACK is valid
>> + * false else
>> + */
>> +static bool check_and_update_ack_id_bounds(struct update_state *const state,
>> + uint32_t ack_peer_update_id)
>
> const uint32_t ack_peer_update_id would be more const correct.

Fixed.

>> +{
>> + HIP_ASSERT(state);
>> +
>> + /* two cases:
>> + * oldest not yet acknowledged SEQ <= latest sent Update SEQ
>> + * => received ACK must be in those bounds to be valid
>> + * oldest not yet acknowledged SEQ > latest sent Update SEQ (counter overflow)
>> + * => received ACK must be greater than oldest SEQ or smaller than latest
>> + * sent Update ID to be valid
>> + */
>
> This comment is hard to understand without knowledge about your internal seq/ack data structure. Please describe how the two fields update_id_out_lower_bound and update_id_out are used in a normal update situation.

See my comment below on this.

>> + if ((state->update_id_out_lower_bound <= hip_update_get_out_id(state) &&
>> + ack_peer_update_id >= state->update_id_out_lower_bound &&
>> + ack_peer_update_id <= hip_update_get_out_id(state)) ||
>> + (state->update_id_out_lower_bound > hip_update_get_out_id(state) &&
>> + (ack_peer_update_id >= state->update_id_out_lower_bound ||
>> + ack_peer_update_id <= hip_update_get_out_id(state)))) {
>> + state->update_id_out_lower_bound = ack_peer_update_id;
>> + return true;
>> + } else {
>> + return false;
>> + }
>
> Please break this down into a if () ... else if () ... else for higher readability.

Is it even necessary to differentiate between the two cases? Come to think of it an overflow seems very unlikely. I would not count too much on my math skills but by my calculation it would take more than 130 years with an update every second to reach the end of that range as it is unsigned 32bit. My suggestion would be to leave it out. The part of the code dealing with the incoming UPDATE IDs does not seem to care about this either.

>> * Check if UPDATE sequence and acknowledgment numbers are as expected.
>> *
>> * @param packet_type the packet type
>> @@ -216,12 +252,12 @@
>> HIP_DEBUG("ACK parameter found with peer Update ID %u.\n",
>> ack_peer_update_id);
>>
>> - // we only want acks for our most current update
>> - ...

[Diego Biurrun (diego-biurrun)]

On Wed, Aug 10, 2011 at 01:58:54PM +0000, David Martin wrote:
>
> On Wed, Aug 10, 2011 at 1:52 PM, René Hummen <email address hidden> wrote:
> > On 10.08.2011, at 13:16, David Martin wrote:
> >> David Martin has proposed merging lp:~martin-lp/hipl/update_ack_handling into lp:hipl.
> >> --- modules/update/hipd/update.c 2011-07-18 16:41:13 +0000
> >> +++ modules/update/hipd/update.c 2011-08-10 11:16:05 +0000
> >> @@ -157,6 +158,41 @@
> >> + if ((state->update_id_out_lower_bound <= hip_update_get_out_id(state) &&
> >> + ack_peer_update_id >= state->update_id_out_lower_bound &&
> >> + ack_peer_update_id <= hip_update_get_out_id(state)) ||
> >> + (state->update_id_out_lower_bound > hip_update_get_out_id(state) &&
> >> + (ack_peer_update_id >= state->update_id_out_lower_bound ||
> >> + ack_peer_update_id <= hip_update_get_out_id(state)))) {
> >> + state->update_id_out_lower_bound = ack_peer_update_id;
> >> + return true;
> >> + } else {
> >> + return false;
> >> + }
> >
> > Please break this down into a if () ... else if () ... else for higher readability.
>
> Is it even necessary to differentiate between the two cases? Come to
> think of it an overflow seems very unlikely. I would not count too
> much on my math skills but by my calculation it would take more than
> 130 years with an update every second to reach the end of that range
> as it is unsigned 32bit.

general comment that may or may not be relevant here: Do not calculate
based on some likely scenario. Calculate what may happen if one were
to (maliciously) hammer the daemon with update packets.

Diego

[Christof Mroz (christof-mroz)]

You introduce the lower bound in this branch, but other than initializing it to zero it's not updated. Don't you need to adjust it upon succesfully processing the update tagged with the smallest ID?

review: Needs Information

[David Martin (martin-lp)]

> On Wed, Aug 10, 2011 at 01:58:54PM +0000, David Martin wrote:
> >
> > On Wed, Aug 10, 2011 at 1:52 PM, René Hummen <email address hidden>
> wrote:
> > > On 10.08.2011, at 13:16, David Martin wrote:
> > >> David Martin has proposed merging lp:~martin-lp/hipl/update_ack_handling
> into lp:hipl.
> > >> --- modules/update/hipd/update.c 2011-07-18 16:41:13 +0000
> > >> +++ modules/update/hipd/update.c 2011-08-10 11:16:05 +0000
> > >> @@ -157,6 +158,41 @@
> > >> + if ((state->update_id_out_lower_bound <=
> hip_update_get_out_id(state) &&
> > >> + ack_peer_update_id >= state->update_id_out_lower_bound &&
> > >> + ack_peer_update_id <= hip_update_get_out_id(state)) ||
> > >> + (state->update_id_out_lower_bound > hip_update_get_out_id(state)
> &&
> > >> + (ack_peer_update_id >= state->update_id_out_lower_bound ||
> > >> + ack_peer_update_id <= hip_update_get_out_id(state)))) {
> > >> + state->update_id_out_lower_bound = ack_peer_update_id;
> > >> + return true;
> > >> + } else {
> > >> + return false;
> > >> + }
> > >
> > > Please break this down into a if () ... else if () ... else for higher
> readability.
> >
> > Is it even necessary to differentiate between the two cases? Come to
> > think of it an overflow seems very unlikely. I would not count too
> > much on my math skills but by my calculation it would take more than
> > 130 years with an update every second to reach the end of that range
> > as it is unsigned 32bit.
>
> general comment that may or may not be relevant here: Do not calculate
> based on some likely scenario. Calculate what may happen if one were
> to (maliciously) hammer the daemon with update packets.

Good point. I've kept it in an separated the cases for readability.

[David Martin (martin-lp)]

On Wed, Aug 10, 2011 at 7:40 PM, Christof Mroz <email address hidden> wrote:
> You introduce the lower bound in this branch, but other than initializing it to zero it's not updated. Don't you need to adjust it upon succesfully processing the update tagged with the smallest ID?

It gets updated in check_and_update_ack_id_bounds() with every incoming valid ACK. The problems that may be as it is:
- it gets updated before the HMAC check is performed, a retransmission would still be accepted though so this should not make trouble.
- out-of-order ACKs get dropped. Say you send two update packets with SEQ x and x+1 and you receive the ACK for x+1 first, it would update the lower bound and ACK for x would be dropped. This is non-trivial to fix though. Seems to be ok with the RFC in any case: "The semantics are such that the receiver MUST acknowledge the UPDATE, but the sender MAY choose to not care about receiving the ACK."

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Thu, Aug 11, 2011 at 01:46:52PM +0000, David Martin wrote:
>
> --- modules/update/hipd/update.c 2011-07-18 16:41:13 +0000
> +++ modules/update/hipd/update.c 2011-08-11 13:46:45 +0000
> @@ -157,6 +158,46 @@
>
> + if (state->update_id_out_lower_bound <= hip_update_get_out_id(state) &&
> + ack_peer_update_id >= state->update_id_out_lower_bound &&
> + ack_peer_update_id <= hip_update_get_out_id(state)) {
> + state->update_id_out_lower_bound = ack_peer_update_id;
> + return true;
> + } else if (state->update_id_out_lower_bound > hip_update_get_out_id(state) &&
> + (ack_peer_update_id >= state->update_id_out_lower_bound ||
> + ack_peer_update_id <= hip_update_get_out_id(state))) {
> + state->update_id_out_lower_bound = ack_peer_update_id;
> + return true;
> + } else {
> + return false;
> + }

Geez, this is complicated. Luckily some of it is redundant, witness:

  if (state->update_id_out_lower_bound <= hip_update_get_out_id(state)) {
      if (ack_peer_update_id >= state->update_id_out_lower_bound &&
          ack_peer_update_id <= hip_update_get_out_id(state)) {
          state->update_id_out_lower_bound = ack_peer_update_id;
          return true;
  } else {
      if (ack_peer_update_id >= state->update_id_out_lower_bound ||
          ack_peer_update_id <= hip_update_get_out_id(state)) {
          state->update_id_out_lower_bound = ack_peer_update_id;
          return true;
  }
  return false;

Diego

review: Needs Fixing

[David Martin (martin-lp)]

On Thu, Aug 11, 2011 at 4:22 PM, Diego Biurrun <email address hidden> wrote:
> review needs-fixing
>
> On Thu, Aug 11, 2011 at 01:46:52PM +0000, David Martin wrote:
>>
>> --- modules/update/hipd/update.c 2011-07-18 16:41:13 +0000
>> +++ modules/update/hipd/update.c 2011-08-11 13:46:45 +0000
>> @@ -157,6 +158,46 @@

> Geez, this is complicated.

I know I know, but there does not seem to be much you can do about it.

> Luckily some of it is redundant, witness:
>
> if (state->update_id_out_lower_bound <= hip_update_get_out_id(state)) {
> if (ack_peer_update_id >= state->update_id_out_lower_bound &&
> ack_peer_update_id <= hip_update_get_out_id(state)) {
> state->update_id_out_lower_bound = ack_peer_update_id;
> return true;
> } else {
> if (ack_peer_update_id >= state->update_id_out_lower_bound ||
> ack_peer_update_id <= hip_update_get_out_id(state)) {
> state->update_id_out_lower_bound = ack_peer_update_id;
> return true;
> }
> return false;

Hah, you are awesome! Fixed it.

[Diego Biurrun (diego-biurrun)]

 review approve

On Thu, Aug 11, 2011 at 02:43:17PM +0000, David Martin wrote:
>
> --- modules/update/hipd/update.c 2011-08-11 13:51:14 +0000
> +++ modules/update/hipd/update.c 2011-08-11 14:43:04 +0000
> @@ -157,6 +158,47 @@
>
> + if (state->update_id_out_lower_bound <= hip_update_get_out_id(state)) {
> + if (ack_peer_update_id >= state->update_id_out_lower_bound &&
> + ack_peer_update_id <= hip_update_get_out_id(state)) {
> + state->update_id_out_lower_bound = ack_peer_update_id;
> + return true;
> + }
> + } else {
> + if (ack_peer_update_id >= state->update_id_out_lower_bound ||
> + ack_peer_update_id <= hip_update_get_out_id(state)) {
> + state->update_id_out_lower_bound = ack_peer_update_id;
> + return true;
> + }
> + }

Somebody please surprise us with a way to avoid this code duplication.
My only idea is goto.

Anyway, LGTM now.

Diego

review: Approve

[René Hummen (rene-hummen)]

review: Approve

[Christof Mroz (christof-mroz)]

On Thu, 11 Aug 2011 17:01:35 +0200, Diego Biurrun <email address hidden> wrote:

>> + if (state->update_id_out_lower_bound <=
>> hip_update_get_out_id(state)) {
>> + if (ack_peer_update_id >= state->update_id_out_lower_bound &&
>> + ack_peer_update_id <= hip_update_get_out_id(state)) {
>> + state->update_id_out_lower_bound = ack_peer_update_id;
>> + return true;
>> + }
>> + } else {
>> + if (ack_peer_update_id >= state->update_id_out_lower_bound ||
>> + ack_peer_update_id <= hip_update_get_out_id(state)) {
>> + state->update_id_out_lower_bound = ack_peer_update_id;
>> + return true;
>> + }
>> + }
>
> Somebody please surprise us with a way to avoid this code duplication.
> My only idea is goto.

Reversing the logic and returning false early (but it may be harder to
comprehend this way).

[Christof Mroz (christof-mroz)]

On Thu, 11 Aug 2011 15:43:33 +0200, David Martin
<email address hidden> wrote:

> On Wed, Aug 10, 2011 at 7:40 PM, Christof Mroz
> <email address hidden> wrote:
>> You introduce the lower bound in this branch, but other than
>> initializing it to zero it's not updated. Don't you need to adjust it
>> upon succesfully processing the update tagged with the smallest ID?
>
> It gets updated in check_and_update_ack_id_bounds() with every incoming
> valid ACK.

You are right of course. It's hard to spot where the if(...) condition
ends and the body starts IMHO. You may consider adding an empty line after
the opening brace for readability.

> The problems that may be as it is:
> - it gets updated before the HMAC check is performed, a retransmission
> would still be accepted though so this should not make trouble.
> - out-of-order ACKs get dropped. Say you send two update packets with
> SEQ x and x+1 and you receive the ACK for x+1 first, it would update the
> lower bound and ACK for x would be dropped. This is non-trivial to fix
> though. Seems to be ok with the RFC in any case: "The semantics are such
> that the receiver MUST acknowledge the UPDATE, but the sender MAY choose
> to not care about receiving the ACK."

Yes, don't bother as long as the branch solves the original problem (which
it does as far as I can tell).

[Christof Mroz (christof-mroz)]

LGTM

review: Approve

[Diego Biurrun (diego-biurrun)]

On Thu, Aug 11, 2011 at 03:18:39PM +0000, Christof Mroz wrote:
> On Thu, 11 Aug 2011 17:01:35 +0200, Diego Biurrun <email address hidden> wrote:
>
> >> + if (state->update_id_out_lower_bound <=
> >> hip_update_get_out_id(state)) {
> >> + if (ack_peer_update_id >= state->update_id_out_lower_bound &&
> >> + ack_peer_update_id <= hip_update_get_out_id(state)) {
> >> + state->update_id_out_lower_bound = ack_peer_update_id;
> >> + return true;
> >> + }
> >> + } else {
> >> + if (ack_peer_update_id >= state->update_id_out_lower_bound ||
> >> + ack_peer_update_id <= hip_update_get_out_id(state)) {
> >> + state->update_id_out_lower_bound = ack_peer_update_id;
> >> + return true;
> >> + }
> >> + }
> >
> > Somebody please surprise us with a way to avoid this code duplication.
> > My only idea is goto.
>
> Reversing the logic and returning false early (but it may be harder to
> comprehend this way).

Good idea, I fully support that :)

Diego

[lp:~martin-lp/hipl/update_ack_handling]

6053. By David Martin on 2011-08-12

Invert logic of ACK checks for improved readability, avoided code duplication.

[David Martin (martin-lp)]

On Thu, Aug 11, 2011 at 5:18 PM, Christof Mroz <email address hidden> wrote:
> On Thu, 11 Aug 2011 17:01:35 +0200, Diego Biurrun <email address hidden> wrote:
>
>>> + if (state->update_id_out_lower_bound <=
>>> hip_update_get_out_id(state)) {
>>> + if (ack_peer_update_id >= state->update_id_out_lower_bound &&
>>> + ack_peer_update_id <= hip_update_get_out_id(state)) {
>>> + state->update_id_out_lower_bound = ack_peer_update_id;
>>> + return true;
>>> + }
>>> + } else {
>>> + if (ack_peer_update_id >= state->update_id_out_lower_bound ||
>>> + ack_peer_update_id <= hip_update_get_out_id(state)) {
>>> + state->update_id_out_lower_bound = ack_peer_update_id;
>>> + return true;
>>> + }
>>> + }
>>
>> Somebody please surprise us with a way to avoid this code duplication.
>> My only idea is goto.
>
> Reversing the logic and returning false early (but it may be harder to
> comprehend this way).

Nice one! Actually I think it makes it easier to comprehend as > and < are at least for me easier to imagine than >= and <=. The update of the lower bound is more visible as well.

I'll have to ask another review of the change of you though. It still seems to do the same but better safe than sorry and it's always these small things that cause the hardest to find bugs.

Unmerged revisions

=== modified file 'modules/update/hipd/update.c'
--- modules/update/hipd/update.c	2011-08-11 13:51:14 +0000
+++ modules/update/hipd/update.c	2011-08-12 09:58:25 +0000
@@ -36,6 +36,7 @@
 #define _BSD_SOURCE
 
 #include <errno.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <string.h>
 
@@ -157,6 +158,46 @@
 }
 
 /**
+ * Verify the validity of the ACK ID of a received Update packet and update the
+ * range in which Update packets will be accepted accordingly.
+ *
+ * @param  state              The update state of the respective HA.
+ * @param  ack_peer_update_id The received ACK ID.
+ *
+ * @return true  if received ACK is valid
+ *         false else
+ */
+static bool check_and_update_ack_id_bounds(struct update_state *const state,
+                                           const uint32_t ack_peer_update_id)
+{
+    HIP_ASSERT(state);
+
+    /* Of the three UPDATE packets the first and second must be acknowledged.
+     * hip_update_get_out_id() gets the latest outgoing UPDATE ID whereas
+     * update_id_out_lower_bound stores the oldest sent UPDATE ID with an
+     * outstanding acknowledgement. Together they form the window in which
+     * incoming ACKs are valid. Multiple ACKs may be outstanding for example
+     * when both host and peer initiate an update. In this case both
+     * U1 (update initiated by the host) and U2 (response to update initiated
+     * by the peer) are to be acknowledged and their IDs stored in
+     * update_id_out_lower_bound and hip_update_get_out_id() respectively.
+     */
+    if (state->update_id_out_lower_bound <= hip_update_get_out_id(state)) {
+        if (ack_peer_update_id < state->update_id_out_lower_bound ||
+            ack_peer_update_id > hip_update_get_out_id(state)) {
+            return false;
+        }
+    } else {
+        if (ack_peer_update_id < state->update_id_out_lower_bound &&
+            ack_peer_update_id > hip_update_get_out_id(state)) {
+            return false;
+        }
+    }
+    state->update_id_out_lower_bound = ack_peer_update_id;
+    return true;
+}
+
+/**
  * Check if UPDATE sequence and acknowledgment numbers are as expected.
  *
  * @param packet_type the packet type
@@ -216,12 +257,12 @@
         HIP_DEBUG("ACK parameter found with peer Update ID %u.\n",
                   ack_peer_update_id);
 
-        // we only want acks for our most current update
-        if (ack_peer_update_id != hip_update_get_out_id(localstate)) {
-            HIP_DEBUG("Update ID (%u) in the ACK parameter is not "
-                      "equal to the last outgoing Update ID (%u). "
+        if (!check_and_update_ack_id_bounds(localstate, ack_peer_update_id)) {
+            HIP_DEBUG("Update ID (%u) in the ACK parameter is not in the "
+                      "current Update ID window (%u-%u). "
                       "Dropping the packet.\n",
                       ack_peer_update_id,
+                      localstate->update_id_out_lower_bound,
                       hip_update_get_out_id(localstate));
             err = -1;
             goto out_err;
@@ -488,10 +529,11 @@
         return -1;
     }
 
-    update_state->update_state   = 0;
-    update_state->valid_locators = 0;
-    update_state->update_id_out  = 0;
-    update_state->update_id_in   = 0;
+    update_state->update_state              = 0;
+    update_state->valid_locators            = 0;
+    update_state->update_id_out             = 0;
+    update_state->update_id_out_lower_bound = 0;
+    update_state->update_id_in              = 0;
 
     res = lmod_add_state_item(state, update_state, "update");
     if (res == -1) {

=== modified file 'modules/update/hipd/update.h'
--- modules/update/hipd/update.h	2011-07-28 18:11:17 +0000
+++ modules/update/hipd/update.h	2011-08-12 09:58:25 +0000
@@ -81,9 +81,15 @@
      */
     unsigned valid_locators;
 
-    /** Stored outgoing UPDATE ID counter. */
+    /** UPDATE ID of the latest outgoing UPDATE packet. */
     uint32_t update_id_out;
-    /** Stored incoming UPDATE ID counter. */
+
+    /** UPDATE ID of the oldest not yet acknowledged outgoing UPDATE packet.
+     *  Usually this value is equal to @c update_id_out. The only exception is
+     *  when more than one UPDATE packet is yet to be acknowledged by the peer. */
+    uint32_t update_id_out_lower_bound;
+
+    /** UPDATE ID of the latest incoming UPDATE packet. */
     uint32_t update_id_in;
 };