Ad Hoc On-demand Distance Vector Version 2 (AODVv2) Routing
draft-perkins-manet-aodvv2-04

My AD term is ending, and this document hasn't been revised in several years.  As it is AD-Sponsored, I am clearing the state and allowing the new responsible AD to decide how to proceed.

Request for posting confirmation emailed to previous authors: Lotte Steenbrink , db3546@att.com, Charles Perkins , Stan Ratliff , aretana.ietf@gmail.com, Victoria Mercieca , martin.vigoureux@nokia.com, John Dowdell

=== AD Review of draft-perkins-manet-aodvv2-02 (Part 3) ===

Dear authors:

Part 3 of my review includes Section 6 (AODVv2 Protocol Operations).

I have to say that I got lost in some places, specially around §6.7.  IOW, I have a lot of clarifying questions.

Thanks!

Alvaro.


...
  951 6.1.  Initialization

  953  When an AODVv2 router does not have information about its previous
  954  sequence number, or if its sequence number is lost at any point, the
  955  router reinitializes its sequence number to one (1).  However, other
  956  AODVv2 routers may still hold sequence number information that this
  957  router previously issued.  Since sequence number information is
  958  removed if there has been no update to the sequence number in
  959  MAX_SEQNUM_LIFETIME, the initializing router MUST wait for
  960  MAX_SEQNUM_LIFETIME before it creates any messages containing its new
  961  sequence number.

[nit] This section starts when the router has already been initialized...in fact, it already somehow doesn't have information about itself (?).  It just seems a weird place to start talking about Initialization.

[major] §4.4 says that "each AODVv2 router in the network MUST maintain its own sequence number."  Yes, a bug can cause a router to not have information or have lost its sequence number...but the case (guessing from the section name) would more like be initial setting (not reinitialization).  Is that what you're trying to convey?

[minor] §4.4 also says that "zero (0) is reserved to indicate that the router's sequence number is unknown".  Is unknown the same as lost?

[major] "initializing router MUST wait for MAX_SEQNUM_LIFETIME before it creates any messages containing its new sequence number"  That's a long time!  Does that mean that a new router in the network can't request routes (peeking below, what else?) for 5 minutes?  I may be interpreting "initialization" not as intended: I'm thinking of what happens when the AODVv2 process is first started...  Are we talking about the same thing?

  963  During this wait period, the router is permitted to do the following:

  965  o  Process information in a received RREQ or RREP message to obtain a
  966      route to the originator or target of that route discovery

  968  o  Forward a received RREQ or RREP

  970  o  Send an RREP_Ack

  972  o  Maintain valid routes in the Local Route Set

  974  o  Create, process and forward RERR messages

  976 6.2.  Next Hop Monitoring
…
  983  AODVv2 provides a mechanism for testing bidirectional connectivity
  984  during route discovery, and blacklisting routers where bidirectional
  985  connectivity is not available.  If a route discovery is retried by
  986  RREQ_Gen, the blacklisted routers are excluded from the process, and
  987  a different route can be discovered.  Further, a route is not to be
  988  used for forwarding until the bidirectionality of the link to the
  989  next hop is confirmed.  AODVv2 routers do not need to monitor
  990  bidirectionality for links to neighboring routers which are not used
  991  as next hops on routes in the Local Route Set.

[minor] "If a route discovery is retried by RREQ_Gen, the blacklisted routers are excluded from the process..."  I'm assuming that the blacklisted routers are also excluded from new RREQs (not just retries).  Is that true?  §3 does mention it ("Uni-directional links are not suitable; AODVv2 will detect and exclude those links from route discovery."), but it would be better if the statement was clearly made...

Note that the next sentence ("Further, a route is not to be used for forwarding until the bidirectionality of the link to the next hop is confirmed.") does confirm...  Maybe consolidate the two into a general statement.

  993  o  Bidirectional connectivity to upstream routers is tested as
  994      necessary by requesting acknowledgement of RREP messages,
  995      including an AckReq element to indicate that an acknowledgement is
  996      requested.  This MUST be answered by sending an RREP_Ack in
  997      response.  Receipt of an RREP_Ack within RREP_Ack_SENT_TIMEOUT
  998      demonstrates that bidirectional connectivity exists.  Otherwise,
  999      the link is considered to be unidirectional.  All AODVv2 routers
  1000      MUST support this process, which is explained in Section 7.2 and
  1001      Section 7.3.

[nit] I think that should only be 7.3.

[minor] What does "as necessary" mean?  §7.3 says that the mechanism is used only "over a link which is not known to be bidirectional".  Please say the same thing here…if that is “as necessary".

...
  1019  If no such entry exists, a new entry is created as described in
  1020  Section 6.3.  While the value of Neighbor.State is HEARD,
  1021  acknowledgement of RREP messages sent to that neighbor MUST be
  1022  requested.  If an acknowledgement is not received within the timeout
  1023  period, the neighbor MUST have Neighbor.State set to BLACKLISTED.  If
  1024  an acknowledgement is received within the timeout period,
  1025  Neighbor.State is set to CONFIRMED.  When the value of Neighbor.State
  1026  is CONFIRMED, the request for an acknowledgement of any other RREP
  1027  message is unnecessary.

[major] If continuous (or at least periodic) bidirectional checking is not done, is the assumption then that once the link is determined to be bidirectional that the characteristics won't change?  Or is it assumed that as long as the rest of the protocol machinery is working, then everything is ok?

  1029  Additional indications of connectivity may be available from other
  1030  operations, for example:

  1032  o  MAC layer protocol assuring bidirectional links

  1034  o  Route timeout

  1036  o  Lower layer triggers, e.g. message reception or link status
  1037      notifications

  1039  o  TCP timeouts

  1041  o  Promiscuous listening

  1043  o  receipt of a Neighborhood Discovery Protocol HELLO message with
  1044      the receiving router listed as a neighbor [RFC6130]

[major] AODVv2 seems to not have any type of neighbor discovery (except waiting for protocol packets to show up).  Is that true?  Does that mean that the routers promiscuously accept request from anyone?  Is that what "promiscuous listening" means?  Since this is the only place where the word "promiscuous" appears in the document, it may be worth explaining further.

[major] What is the relationship with NHDP?  This is the only place where it is mentioned -- is the assumption that it is being used?  Is it optional?  Mandatory?  What are the advantages/changes if it is used?  NHDP has the concept of a "symmetric link", which is equivalent to bidirectional...why isn't that used (instead of defining a native mechanism)? What am I missing?

  1046  o  Other monitoring mechanisms or heuristics
  1047  If such an external process signals that the link to a neighbor is
  1048  bidirectional, the AODVv2 router MAY update the matching Neighbor Set
  1049  entry by changing the value of Neighbor.State to CONFIRMED.  If an
  1050  external process signals that a link is not bidirectional, then the
  1051  value of Neighbor.State MAY be changed to BLACKLISTED.

[major] The text above and §7.3 use MUST to require the ack'ing mechanism to verify bidirectionality.  IOW, the MAYs here contradict that part of the specification.  If external mechanisms can be used (as mentioned here), then the ack'ing above can't be mandatory (MUST).

  1053 6.3.  Neighbor Set Update

...
[minor] What about Neighbor.AckSeqNum and Neighbor.HeardRERRSeqNum?  For completeness, please mention them here too.

...
  1082  o  an RREP_Ack response received from a Neighbor with Neighbor.State
  1083      set to HEARD, where Neighbor.Timeout > CurrentTime

[minor] I think that instead of "Neighbor.Timeout > CurrentTime" you mean something like "the response was received within RREP_Ack_SENT_TIMEOUT".

  1085  then the link to the neighbor is bidirectional and the Neighbor Set
  1086  entry is updated as follows:

[major] This text also proposes an alrernative to the ack mechanism (§7.3), which puts it in conflict with the mandatory (MUST) nature of the ack.


...
  1092  When the Neighbor.Timeout is reached and Neighbor.State is HEARD,
  1093  then an RREP_Ack response has not been received from the neighbor
  1094  within RREP_Ack_SENT_TIMEOUT of sending the RREP_Ack request.  The
  1095  link is considered to be uni-directional and the Neighbor Set entry
  1096  is updated as follows:

[nit] s/When.../If...


…
  1105  o  Neighbor.State := HEARD

[minor] o  Neighbor.Timeout := INFINITY_TIME

  1107  If an external mechanism reports a link as broken, the Neighbor Set
  1108  entry SHOULD be removed.

[major] This dependency on the external mechanisms makes them Normative, but (except for NHDP) there are no references for them.  I'm assuming you're referring to the list earlier in this section...

[major] If the link is reported as broken, why wouldn't the Neighbor Set always be removed?  IOW, why SHOULD and not MUST?


...
  1116 6.4.  Interaction with the Forwarding Plane

  1118  The signals described in the following are conceptual in nature, and
  1119  can be implemented in various ways.  Conformant implementations of
  1120  AODVv2 are not mandated to implement the forwarding plane separately
  1121  from the control plane or data plane; these signals and interactions
  1122  are identified simply as assistance for implementers who may find
  1123  them useful.

[nit] s/conformant/conforming  According to [1] "conformant" is obsolete.

[1] https://www.merriam-webster.com/dictionary/conformant

[major]  I'm having some trouble with this first paragraph... It talks about conforming implementations, but it also says that the "signals and interactions are identified simply as assistance for implementers who may find them useful", which sounds to me as informative text.  The question is: is the forwarding plane behavior Normative in this document?

To expand a little more...  A "route is unavailable" failure seems to be very important to AODVv2 because of its reactive nature.  OTOH, forwarding success seems less critical.

  1125  AODVv2 requires signals from the forwarding plane:

  1127  o  A packet cannot be forwarded because a route is unavailable:
  1128      AODVv2 needs to know the source and destination IP addresses of
  1129      the packet.  If the source of the packet is configured as a Router
  1130      Client, the router SHOULD initiate route discovery to the
  1131      destination.  If it is not a Router Client, the router SHOULD
  1132      create a Route Error message.

[major] When would the router not perform the actions?  IOW, why use SHOULD and not MUST?

  1134  o  A packet is to be forwarded: AODVv2 needs to check the state of
  1135      the route to ensure it is still valid.

[minor] I'm assuming that the RIB has currently valid information...  But the text above sounds as if the validity of the routes has to be verified each time a packet is to be forwarded.

  1137  o  Packet forwarding succeeds: AODVv2 needs to update the record of
  1138      when a route was last used to forward a packet.

[minor] It would be really nice if there was a clear relationship between this signal and LocalRoute.LastUsed.  In the description of LocalRoute.LastUsed in §4.5 it is not clear how the time is updated.

  1140  o  Packet forwarding failure occurs: AODVv2 needs to create a Route
  1141      Error message.

[minor] What are other examples of forwarding failures (besides the route not being available)?

  1143  AODVv2 needs to send signals to the forwarding plane when:

  1145  o  A route discovery is in progress: buffering might be configured
  1146      for packets requiring a route, while route discovery is attempted.

[nit] A reference to §6.6 would be nice.

  1148  o  A route discovery failed: any buffered packets requiring that
  1149      route should be discarded, and the source of the packet should be
  1150      notified that the destination is unreachable (using an ICMP
  1151      Destination Unreachable message).  Route discovery fails if an
  1152      RREQ cannot be generated because the control message generation
  1153      limit has been reached (see Section 6.5), or if an RREP is not
  1154      received within RREQ_WAIT_TIME (see Section 6.6).

[major] "...the source of the packet should be notified that the destination is unreachable".  Should the "should" be Normative?

[nit] A reference to ICMP Destination Unreachable message would be nice.


...
  1171 6.5.  Message Transmission

  1173  AODVv2 sends [RFC5444] formatted messages using the parameters for
  1174  port number and IP protocol specified in [RFC5498].  Mapping of
  1175  AODVv2 data to [RFC5444] messages is detailed in Section 8.  AODVv2
  1176  multicast messages are sent to the link-local multicast address LL-
  1177  MANET-Routers [RFC5498].  All AODVv2 routers MUST subscribe to LL-
  1178  MANET-Routers on all AODVv2 interfaces [RFC5498] to receive AODVv2
  1179  messages.  Note that multicast messages MAY be sent via unicast.  For
  1180  example, this may occur for certain link-types (non-broadcast media),
  1181  for manually configured router adjacencies, or in order to improve
  1182  robustness.

[nit] s/Note that multicast messages MAY be sent via unicast./Note that messages MAY be sent via unicast.


...
  1222 6.6.  Route Discovery, Retries and Buffering
...
  1239  Buffering might be configured for IP packets awaiting a route for
  1240  forwarding by RREQ_Gen, if sufficient memory is available.  Buffering
  1241  of IP packets might have both positive and negative effects.  TCP
  1242  connection establishment will benefit if packets are queued while
  1243  route discovery is performed [Koodli01], but real-time traffic,
  1244  voice, and scheduled delivery may suffer if packets are buffered and
  1245  subjected to delays.  Recommendations for appropriate buffer methods
  1246  are out of scope for this specification.  Determining which packets
  1247  to discard first when the buffer is full is a matter of policy at
  1248  each AODVv2 router.  Using different (or no) buffer methods might
  1249  affect performance but does not affect interoperability.

[minor] "Determining which packets to discard first when the buffer is full is a matter of policy at each AODVv2 router."  Buffer mechanisms is out of scope, but what to do when the buffer is full isn't??  Or should the policy also be out of scope (i.e. don't mention the "AODVv2 router")?

  1251  RREQ_Gen awaits reception of a Route Reply message (RREP) containing
  1252  a route toward TargAddr.  This can be achieved by monitoring the
  1253  entry in the Multicast Route Message Table that corresponds to the
  1254  generated RREQ.  When CurrentTime exceeds McMsg.Timestamp +
  1255  RREQ_WAIT_TIME and no RREP has been received, RREQ_Gen will retry the
  1256  route discovery.

[minor] "Multicast Route Message Table"  Do you mean Multicast Route Message Set?

...
  1270  After DISCOVERY_ATTEMPTS_MAX and the corresponding wait time for an
  1271  RREP response to the final RREQ, route discovery is considered to
  1272  have failed.  If an attempted route discovery has failed, RREQ_Gen
  1273  SHOULD wait at least RREQ_HOLDDOWN_TIME before attempting another
  1274  route discovery to the same destination, in order to avoid repeatedly
  1275  generating control traffic that is unlikely to discover a route.  Any
  1276  IP packets buffered for TargAddr are also dropped and a Destination
  1277  Unreachable ICMP message (Type 3) with a code of 1 (Host Unreachable
  1278  Error) is delivered to the source of the packet, so that the
  1279  application knows about the failure.

[major] Do we need Normative language for the action taken?


...
  1292 6.7.  Processing Received Route Information

  1294  A Route Request (RREQ) contains a route to OrigPrefix, and a Route
  1295  Reply (RREP) contains a route to TargPrefix.  All AODVv2 routers that
  1296  receive a route message first check that they have sufficient memory
  1297  to store the route contained within it in their Local Route Set.
  1298  Incoming information is first checked to verify that it is both safe
  1299  to use and offers an improvement to existing information, as
  1300  explained in Section 6.7.1.  If these checks pass, the Local Route
  1301  Set MUST be updated according to Section 6.7.2.

[minor] "first check that they have sufficient memory"  What is the significance of this check?  If not enough memory is available, then what?  §6.10.1 says that "Unconfirmed route MUST NOT be expunged" -- at the point of just receiving (and not processing) a RREQ then entry is still not Unconfirmed...so it seems like it may be ok (at least not contradicting) to not store the route (if that is the action to be taken if there's insuffiecient memory).

[minor] §6.7.1 talks about a route being "safe against routing loops".  Is that what you mean above by "safe to use"?  Please be clear/consistent.

[major] "MUST be updated according to Section 6.7.2."  §6.7.2 contains some SHOULDs -- I haven't read that section in detail (yet), but to avoid a conflict between the MUST in this section and the SHOULDs later on, s/MUST/must  I think that the first paragraph in §6.7.2 is clear in indicating the next step in the process.

  1303  In the processes below, RteMsg is used to denote the received route
  1304  message, AdvRte is used to denote the route contained within it, and
  1305  LocalRoute denotes an existing entry in the Local Route Set which
  1306  matches AdvRte on address, prefix length, metric type, and SeqNoRtr.

[nit] "In the processes below..."  The definitions don't just apply here, right?

[minor] So...if AdvRte is contained in the RteMsg, then they're the same, right?  Why do we need two different representations of the same?  ...and then it's all mapped to LocalRoute.  It seems like they are the same but the terminology is used to apply for different stages in the process (from receiving the route to using it)...  If that is true, it may be nice to explain that upfront (here, or maybe even sooner).

[minor] Should the AdvRte structure be defined somewhere?  Maybe generalize the description of McMsg since that seems to overlap with RteMsg...


…
  1313  o  AdvRte.PrefixLength := RteMsg.OrigPrefixLen (in RREQ) or
  1314      RteMsg.TargPrefixLen (in RREP).  If no prefix length was included
  1315      in RteMsg, prefix length is the address length, in bits, of
  1316      RteMsg.OrigPrefix (in RREQ) or RteMsg.TargPrefix (in RREP)

[major] §4.2 describes RouterClient.PrefixLength and §4.6 McMsg.OrigPrefixLen, but neither point out what to do if the information is not available.  I think that this point should be mentioned there as well.

  1318  o  AdvRte.SeqNum := RteMsg.OrigSeqNum (in RREQ) or RteMsg.TargSeqNum
  1319      (in RREP)

  1321  o  AdvRte.NextHop := RteMsg.IPSourceAddress (an address of the
  1322      sending interface of the router from which the RteMsg was
  1323      received)

[major] IPSourceAddress is not previously defined -- in fact, this is the only place where it comes up.  I think I made the comment before about not understanding the difference between RteMsg and McMsg -- I looked there (§4.6), but couldn't find IPSourceAddress either.  ...  But I did find IP.SourceAddress (§7.3.2), which is compared to Neighbor.IPAddress; IP.SourceAddress doesn't seem to be defined anywhere either...but the use seems to match the description.


...
  1329  o  AdvRte.Cost := Cost(R) using the cost function associated with the
  1330      route's metric type.  For cost metrics, Cost(R) = AdvRte.Metric +
  1331      Cost(L), as described in Section 5, where L is the link from the
  1332      advertising router

[minor] I think that the content in §5 is enough to not have to repeat any of it here.  IOW, s/AdvRte.Cost := Cost(R) using.../AdvRte.Cost := Cost(R) (Section 5)

  1334  o  AdvRte.SeqNoRtr := the IP address in the Address List of type
  1335      SeqNoRtr if one exists, otherwise 0

[minor] I think that after 2 or 3 similar comments I finally came to reqlize that SeqNoRtr is not a sequence number, but the "sequence number router", right?  Or am I still lost?  The previous definitions were slightly different -- please make them the same (I guess the difference will be the router being refered to).

[minor] "Address List..."  Hmmm...  "Address List" shows up only once more (§7.2.1), but AddressList shows up more.  I'm guessing you mean the same thing, right?  Please be consistent!!

[major] "Address List of type SeqNoRtr"  §7.1.1/§7.2.1 mention "AddressList, with AddressType SeqNoRtr", but the types (?) are not discussed anywhere.  In fact, when AddressList is defined (§7.2, for example) no types are mentioned.

  1337 6.7.1.  Evaluating Route Information
...
  1344  1.  Search for LocalRoutes in the Local Route Set matching AdvRte's
  1345      address, prefix length, metric type, and SeqNoRtr (the AODVv2
  1346      router address corresponding to the sequence number).

[major] §6.7 uses notation like AdvRte.Address to refer to these items.  Please use them here.  Consistency!

[minor] There's not need to keep defining SeqNoRtr at every mention (regardless of how confusing it may be).  A clear single definition in a prominent place should be enough -- which also avoids variations.

  1348      *  If no matching LocalRoute exists, AdvRte MUST be used to
  1349          update the Local Route Set and no further checks are required.

[minor] It seems like §6.7.2 is where the instructions on how to create new LocalRoute entries are given.  Please add a forward reference.

  1351      *  If matching LocalRoutes are found, continue to the next step.

[minor] Just to be clear, a match means that all the characteristics above are the same, right?  Even the SeqNoRtr?  If that is different then it implies the same destination but different origin of the information...  Is this considered somewhere else?

  1353  2.  Compare sequence numbers using the technique described in
  1354      Section 4.4

  1356      *  If AdvRte is more recent than all matching LocalRoutes, AdvRte
  1357          MUST be used to update the Local Route Set and no further
  1358          checks are required.

[minor] §4.4 talks about "newer", not "more recent".  Please be consistent!

  1360      *  If AdvRte is stale, AdvRte MUST NOT be used to update the
  1361          Local Route Set. Ignore AdvRte for further processing.

[minor] §4.4 talks about the route being "older...and therefore stale and redundant"; I guess using "stale" is ok, but "older" would be better.

[major] §4.4 says than an older route "MUST therefore be discarded".  Is that effectively the same as the text above?  IOW, is not using the route and ignoring it the same thing as discarding it?  I guess the result is the same: the route has no effect; but discarding implies (among other things) freeing up memory, while not using/ignoring doesn't (at least not immediately).  In a constrained device it maters to be specific.

Note that some of the steps below also talk about ignoring.  Should the action be discard instead?  Are there potential benefits in ignoring/not using and keeping the route around in case it is needed later?

  1363      *  If the sequence numbers are equal, continue to the next step.

[major] (I made a similar comment in §4.4.)  Doesn't that imply a duplicate message?  §14.1.1 says that "duplicate RREQ messages are dropped", but the text above says to continue.


...
  1376  4.  Compare route costs

[minor] §5 says that "implementers MAY consider metric types that are not strictly increasing".  The description below seems to be based on strictly increasing metrics.  Do the better/worse concepts apply in all cases?

  1378      *  If AdvRte is better than all matching LocalRoutes, it MUST be
  1379          used to update the Local Route Set because it offers
  1380          improvement.

  1382      *  If AdvRte is equal in cost and LocalRoute is valid, AdvRte
  1383          SHOULD NOT be used to update the Local Route Set because it
  1384          will offer no improvement.

[major] If it offers no improvement, which are the cases where it could be used?  IOW, why SHOULD NOT and not MUST NOT?

  1386      *  If AdvRte is worse and LocalRoute is valid, ignore AdvRte for
  1387          further processing.  AdvRte MUST NOT be used to update the
  1388          Local Route Set because it does not offer any improvement.

  1390      *  If AdvRte is not better (i.e., it is worse or equal) but
  1391          LocalRoute is Invalid, AdvRte SHOULD be used to update the
  1392          Local Route Set because it can safely repair the existing
  1393          Invalid LocalRoute.

[major] There's a Normative contradiction: the worse condition says that the "AdvRte MUST NOT be used", but the not better condition says that "AdvRte SHOULD be used".  MUST trumps SHOULD all the time.  Perhaps a better way to word the process is to check whether the LocalRoute is valid before comparing the cost...or even before step 3 (?).

...
  1402 6.7.2.  Applying Route Updates

  1404  After determining that AdvRte is to be used to update the Local Route
  1405  Set (as described in Section 6.7.1), the following procedure applies.

  1407  If AdvRte is obtained from an RREQ message, the link to the next hop
  1408  neighbor may not be confirmed as bidirectional (see Section 4.3).  If
  1409  there is no existing matching route in the Local Route Set, AdvRte
  1410  MUST be installed to allow a corresponding RREP to be sent.  If a
  1411  matching entry already exists, and the link to the neighbor can be
  1412  confirmed as bidirectional, AdvRte offers potential improvement.

[major] If I interpreted §6.7.1 correctly, it seemed that by the time we get to this section all the ties with existing LocalRoutes have been broken...and where better, the AdvRte "MUST be used to update the Local Route Set".  However, the last sentence above, and the steps below, still have us comparing the AdvRte and the LocalRoutes.  Are there steps missing above?  Did I miss anything?


...
  1420  2.  If two matching LocalRoutes exist in the Local Route Set, one is
  1421      a valid route, and one is an Unconfirmed route, AdvRte may offer
  1422      further improvement to the Unconfirmed route, or may offer an
  1423      update to the valid route.

[major nit] "valid" not one of the options for LocalRoute.State (§4.5).  I know that the descriptions of Idle and Active mention valid -- maybe specifically call out the fact that Valid (capitalized) means Idle or Active.


...
  1436  3.  If only one matching LocalRoute exists in the Local Route Set:

  1438      *  If AdvRte.NextHop's Neighbor.State is CONFIRMED, continue
  1439          processing from Step 5 to update the existing LocalRoute.

  1441      *  If AdvRte.NextHop's Neighbor.State is HEARD, AdvRte may offer
  1442          improvement the existing LocalRoute, if the link to
  1443          AdvRte.NextHop can be confirmed as bidirectional.

[minor] It seems like this step might also continue to Step 5...??


...
  1453      *  If LocalRoute.State is Active or Idle, AdvRte SHOULD be stored
  1454          as an additional entry in the Local Route Set, with
  1455          LocalRoute.State set to Unconfirmed.  Continue processing from
  1456          Step 4 to create a new LocalRoute.

[major] When wouldn't the AdvRte not be stored?  IOW, why SHOULD and not MUST?

[major] What exactly is an "additional entry"?  Is it just an extra route, where both/either could be used?  Are you talking about a backup?  §6.7.1 says that if "AdvRte is equal in cost and LocalRoute is valid, AdvRte SHOULD NOT be used", so I'm confused as to how we got here and the use of the "additional entry".

The description of Unconfirmed (§4.5) says that a route "MUST NOT be stored in the RIB to forward general data-plane traffic".  This helps with the question about it being an "additional entry"...but soon enough the route will be confirmed as bidirectional...

  1458  4.  Create an entry in the Local Route Set and initialize as follows:

  1460      *  LocalRoute.Address := AdvRte.Address

  1462      *  LocalRoute.PrefixLength := AdvRte.PrefixLength

  1464      *  LocalRoute.MetricType := AdvRte.MetricType

[major] What about the rest of the Local Route Set (§4.5)?  If the rest is in the next step, please say so.

  1466  5.  Update the LocalRoute as follows:
...
  1472      *  LocalRoute.NextHopInterface := interface on which RteMsg was
  1473          received

[minor] Why isn't there an AdvRte.NextHopInterface entry?

  1475      *  LocalRoute.Metric := AdvRte.Cost

  1477      *  LocalRoute.LastUsed := CurrentTime

[minor] I was going to comment on the fact that the route hasn't been used at this point...but I rechecked the definition of LocalRoute.LastUsed (§4.5).  By looking at the Local Route Set, how can I tell if the route has been installed in the RIB?

…
  1482  6.  If a new LocalRoute was created, or if the existing
  1483      LocalRoute.State is Invalid or Unconfirmed, update LocalRoute as
  1484      follows:

[nit] Because the LocalRoute was updated in the last step, the "existing LocalRoute" is the same as the current one...  I think the use of existing is not clear enough...

...
  1492  7.  If an existing LocalRoute.State changed from Invalid or
  1493      Unconfirmed to become Idle, any matching Unconfirmed LocalRoute
  1494      with worse metric value SHOULD be expunged.

[Hmmm..] I'm lost...I don't know anymore which LocalRoute is the new one, which was the existing one (old?), if the new one is still Unconfirmed, should it be expunged?

An example to illustrate the update process would be very nice.  Maybe in a new sub-section (6.7.3)...

  1496  8.  If an existing LocalRoute was updated with a better metric value,
  1497      any matching Unconfirmed LocalRoute with worse metric value
  1498      SHOULD be expunged.

[major] When would these routes not be expunged?  IOW, why SHOULD and not MUST?

  1500  9.  If this update results in LocalRoute.State of Active or Idle,
  1501      which matches a route request which is still in progress, the
  1502      associated route request retry timers MUST be cancelled.

[?] Now I *really* got lost! :-(  A LocalRoute.State of Active or Idle means that the route is valid, has been confirmed as bidirectional, and (for Active) has been used recently.  How can this match "a route request which is still in progress"?  Wouldn't the route request have already been complete?

  1504  If this update to the Local Route Set results in two LocalRoutes to
  1505  the same address, the best LocalRoute will be Unconfirmed.  In order
  1506  to improve the route used for forwarding, the router SHOULD try to
  1507  determine if the link to the next hop of that LocalRoute is
  1508  bidirectional, by using that LocalRoute to forward future RREPs and
  1509  request acknowledgements (see Section 7.2.1 and Section 7.3.

[nit] There a ")" missing.

[minor] "the best LocalRoute will be Unconfirmed"  The best one, really?  Why not the worst one?

[major] When would the router not try to determine bidirectionality?  IOW, why SHOULD and not MUST?

  1511 6.8.  Suppressing Redundant Messages (Multicast Route Message Set)

  1513  When route messages are flooded in a MANET, an AODVv2 router may
  1514  receive several instances of the same message.  Forwarding every one
  1515  of these would provide little additional benefit, while generating
  1516  unnecessary signaling traffic and consequently additional
  1517  interference.  There have been a number of variations of AODV (e.g.,
  1518  [I-D.ietf-roll-aodv-rpl]), that have specified multicast for flooding
  1519  RREP messages as well as RREQ messages.  Since, in this document,
  1520  suppression techniques are only needed for RREQ messages, multicast
  1521  RREP messages are not considered.  However, the technique involved is
  1522  almost identical, and can be handled by substituting "RteMsg" instead
  1523  of RREQ in the following text.

[minor] The slight detour towards "variations of AODV" seems unnecessary at best, and may, at worst, raise questions about their status with respect to this document...  If anyone looks, they'll discover that this document is not even a reference in I-D.ietf-roll-aodv-rpl.

...
  1528  In this section, an entry in the Multicast Route Message Set will be
  1529  called a "multicast entry" for short.  Each multicast entry SHOULD be
  1530  maintained for at least RteMsg_ENTRY_TIME after the last Timestamp
  1531  update in order to account for long-lived RREQs traversing the
  1532  network.  An entry MUST be deleted when the sequence number is no
  1533  longer valid, i.e., after MAX_SEQNUM_LIFETIME.  Memory-constrained
  1534  devices MAY remove the entry before this time.

[major] "MUST be deleted" and "MAY remove" are Normative contradictions.  s/MUST/SHOULD


…
  1547  If the received message is determined to be redundant, no forwarding
  1548  or response to the message is needed.  A message is considered to be
  1549  redundant if either (a) a comparable newer (as determined by the
  1550  OrigSeqNum) entry has already been received with information about
  1551  the source and destination addresses of the route discovery operation
  1552  or (b) it cannot be determined whether the message is newer compared
  1553  to existing entries, but the received message metric value is not any
  1554  better than metric values in compatible multicast entries.

[nit] simplify: s/a comparable newer (as determined by the OrigSeqNum) entry has already been received with information about the source and destination addresses of the route discovery operation/a comparable newer (as determined by the OrigSeqNum) entry has already been received

[minor] Let me see if I understand.  Point (a) refers to an existing multicast entry (and not the received message)...and point (b) refers to the received message, right?

[minor] Why can't it be "determined whether the message is newer"?  Isn't that what the sequence numbers are for, as explained in §4.4?


…
  1560  1.  First, search for a comparable multicast entry.  If there is no
  1561      such entry, then create a new entry as follows:

[minor] For the values before, aren't these RteMsg.*?  If so, then just use RteMsg.* and save writing up more explanations.


...
  1623  If processing for the RREQ has not been discontinued according to the
  1624  above instructions, then continue processing the message as specified
  1625  in Section 7.1.3.

[minor] It doesn't feel right that §7 (AODVv2 Protocol Messages) contains operation information when §6 (AODVv2 Protocol Operations) exists...

  1627 6.9.  Suppressing Redundant Route Error Messages (Route Error Set)

  1629  In order to avoid flooding the network with RERR messages when a
  1630  stream of IP packets to an unreachable address arrives, an AODVv2
  1631  router SHOULD avoid creating duplicate messages by determining
  1632  whether an equivalent RERR has recently been sent.  This is achieved
  1633  with the help of the Route Error Set (see Section 4.7).

[major] When would a router create duplicate messages?  IOW, why SHOULD and not MUST?

[major] "SHOULD avoid" is not really something that can be enforced, how do you avoid?  Perhaps: "SHOULD NOT create".

  1635  To determine if a RERR should be created:

  1637  1.  Search for an entry in the Route Error Set where:

[major] Note that §4.7 says that "each RERR message...SHOULD be recorded", not MUST.  This means that even if a RERR was sent it may not be in the Route Error Set -- this could be the justification for using SHOULD above.

  1639      *  RerrMsg.UnreachableAddress == UnreachableAddress to be
  1640          reported

[minor] It may be obvious, but UnreachableAddress is not defined anywhere.

§7.4.1 talks about "unreachable address", but that is not defined anywhere either.  The packet format (§7.4) has an entry called AddressList (defined as "The addresses of the routes not available through RERR_Gen.").  That seems to be the closest definition of UnreachableAddress.  Suggestion: rename the AddressList in the RERR to UnreachableAddress; this will then match the terminology in the Route Error Set (§4.7) and here.

  1642      *  RerrMsg.PktSource == PktSource to be included in the RERR

[minor] PktSource is only defined in the terminology section -- I suggested there that this type of field name should be defined elsewhere.  In this case §4.7 seems like the right one.

  1644      If a matching entry is found, no further processing is required
  1645      and the RERR SHOULD NOT be sent.

  1647  2.  If no matching entry is found, a new entry with the following
  1648      properties is created, and the RERR is created and sent as
  1649      described in Section 7.4.1:

[nit] Redundant: "...a new entry with the following properties is created, and the RERR is created..."


...
  1658 6.10.  Local Route Set Maintenance
...
  1666  o  (for possibly unidirectional links) confirming a route to
  1667      OrigAddr,

[minor] "possibly unidirectional links" is what, Unconfirmed?

  1669  o  reporting routes that become Invalid.

  1671 6.10.1.  LocalRoute State Changes

  1673  During normal operation, AODVv2 does not require explicit timeouts to
  1674  manage the lifetime of a valid route.  At any time, any LocalRoute
  1675  MAY be examined and updated according to the rules below.  In case a
  1676  Routing Information Base is used for forwarding, the corresponding
  1677  RIB entry MUST be updated as soon as the state of a LocalRoute.State
  1678  changes.  Otherwise, if timers are not used to prompt updates of
  1679  LocalRoute.State, the LocalRoute.State MUST be checked before IP
  1680  packet forwarding and before any operation based on LocalRoute.State.

[major] "At any time, any LocalRoute MAY be examined and updated..."  The implication (from the rest of the paragraph) is that there's a timer that controls what "at any time" is.  Do you have recommendations for those potetial timers or other triggers (besides the ones mentioned at the end of the paragraph, which are not optional)?

[major] "the LocalRoute.State MUST be checked before IP packet forwarding"  I'm sure this doesn't mean "for every packet", but it might be worth it adding some qualifiers of what "before IP packet forwarding" means.  I'm assuming that at least within ACTIVE_INTERVAL or MAX_IDLETIME the state doesn't need to be checked.

[major] "the LocalRoute.State MUST be checked...before any operation based on LocalRoute.State"  Which are those operations?

  1682  Route timeout behaviour is as follows:

  1684  o  An Unconfirmed route MUST be expunged at MAX_SEQNUM_LIFETIME after
  1685      LocalRoute.LastSeqNumUpdate.

[major] §4.5 defines Invalid as "a route that has expired or has broken."  Should an Unconfirmed route be considered expired after MAX_SEQNUM_LIFETIME?  If so, then it seems to me that it shouldn't be expunged (at least not immediately) becasue of "Invalid route SHOULD remain in the Local Route Set" below.

...
  1693  o  An Invalid route SHOULD remain in the Local Route Set, since
  1694      LocalRoute.SeqNum is used to classify future information about
  1695      LocalRoute.Address as stale or fresh.

[major] I think there may be a Normative conflict between this statement ("SHOULD remain") and (from later in this same section) "Any Invalid route MAY be expunged."  The conflict can be resolved by indicating above what are the conditions (is there more than one?) under which the Invalid route would not be kept in the Local Route Set -- a mention of the expunging considerations below should be enough.

  1697  o  In all cases, if the time since LocalRoute.LastSeqNumUpdate
  1698      exceeds MAX_SEQNUM_LIFETIME, LocalRoute.SeqNum must be set to 0.
  1699      This is required so that any AODVv2 routers following the
  1700      initialization procedure can safely begin routing functions using
  1701      a new sequence number.  A LocalRoute with LocalRoute.State set to
  1702      Active or Idle can remain in the Local Route Set after the
  1703      sequence number has been set to 0, for example if the route is
  1704      reliably carrying traffic.  If LocalRoute.State is Invalid, or
  1705      later becomes Invalid, the LocalRoute MUST be expunged from the
  1706      Local Route Set.

[minor] Should this me a MUST: "LocalRoute.SeqNum must be set to 0"?

  1708  LocalRoutes can become Invalid before a timeout occurs, as follows:

  1710  o  If an external mechanism reports a link as broken, all LocalRoutes
  1711      using that link for LocalRoute.NextHop MUST immediately have
  1712      LocalRoute.State set to Invalid.

[major] Which "external mechanisms"? Becausee of the MUST, I think that there shuold be a Normative reference to them...  See my comment in §6.3 about this.

…
  1720      *  There is an Address in AddressList which matches
  1721        LocalRoute.Address, and:

  1723        +  The prefix length associated with this Address, if any,
  1724            matches LocalRoute.PrefixLength

  1726        +  The sequence number associated with this Address, if any, is
  1727            newer or equal to LocalRoute.SeqNum (see Section 4.4)

[major] These last two checks are described with an "and", but both conditions contain "if any".  Is the condition satisfied if the prefix length or sequence number is not present?  Both values are optional in the RRER (§7.4).

  1729        +  The metric type associated with this Address matches
  1730            LocalRoute.MetricType

  1732  A LocalRoute can be confirmed by inferring connectivity to OrigAddr.

[minor] By "confirmed" you mean "confirmed to be bidirectional", right?  Please be specific as the term "confirmed" (on its own) has no specific meaning.

  1734  o  When an AODVv2 router sends an RREP to OrigAddr for destination
  1735      TargAddr, and subsequently the AODVv2 router receives a packet
  1736      from OrigAddr with destination TargAddr, the AODVv2 router infers
  1737      that the route to OrigAddr has been confirmed.  The corresponding
  1738      state for LocalRoute.OrigAddr is changed to Active.

[minor] I don't see LocalRoute.OrigAddr mentioned anywhere else.  I guess you mean that the LocalRoute.State for the entry where LocalRoute.Address is changed to Active, right?

[major] There's no Normative language above, but it contradicts the mandatory nature (MUST) of the ack'ing mechaninsm to confirm bidirectionality.

  1740  LocalRoutes are updated when Neighbor.State is updated:

  1742  o  While the value of Neighbor.State is set to HEARD, any routes in
  1743      the Local Route Set using that neighbor as a next hop MUST have
  1744      LocalRoute.State set to Unconfirmed.

[minor] Please use the structures you already defined: ...any routes with LocalRoute.NextHop...

  1746  o  When the value of Neighbor.State is set to BLACKLISTED, any valid
  1747      routes in the Local Route Set using that neighbor for their next
  1748      hop MUST have LocalRoute.State set to Invalid.

[minor] It seems to me that the definition of BLACKLISTED may correspond more to Unconfirmed than Invalid.  As opposed to removed (below), BLACKLISTED seems to be a case where the bidirectionality has not been confirmed (yet) (§4.3).


…
  1754  Memory constrained devices MAY choose to expunge routes from the
  1755  AODVv2 Local Route Set at other times, but MUST adhere to the
  1756  following rules:

  1758  o  An Active route MUST NOT be expunged, as it is in use.  If
  1759      deleted, IP traffic forwarded to this router would prompt
  1760      generation of a Route Error message, necessitating a Route Request
  1761      to be generated by the originator's router to re-establish the
  1762      route.

  1764  o  An Idle route SHOULD NOT be expunged, as it is still valid for
  1765      forwarding IP traffic.  If deleted, this could result in dropped
  1766      IP packets and a Route Request could be multicasted to re-
  1767      establish the route.

[minor] You didn't mention the RRER case here..

[minor] Knowing that ACTIVE_INTERVAL is really short, I would expect it to be normal for routes to flip back and forth between Active and Idle.  If so, why would you allow the Idle routes to be expunged?  IOW, why is that a SHOULD NOT and not a MUST NOT?


...
  1779 6.10.2.  Reporting Invalid Routes

  1781  When LocalRoute.State changes from Active to Invalid as a result of a
  1782  broken link or a received Route Error (RERR) message, other AODVv2
  1783  routers MUST be informed by sending an RERR message containing
  1784  details of the invalidated route.

[minor] §7.4.1 talks about the possibility of sending multicast RERRs.  If the received RERR above is multicast, then a new RERR is not needed, right?

=== AD Review of draft-perkins-manet-aodvv2-02 (Part 2) ===

Dear authors:

Part 2 of my review includes Section 4 (Data Structures) and 5 (Metrics).

In general, I have a large (too large, I think, for a document that should be mature) set of comments.

Thanks!

Alvaro.



  503 4.  Data Structures

  505 4.1.  Interface Set

  507  The Interface Set is a conceptual data structure which contains
  508  information about all interfaces configured for use by AODVv2.  Any
  509  interface with an IP address can be used.  Multiple interfaces on a
  510  single router can be used.  Multiple interfaces on the same router
  511  may be configured with the same IP address.

  513  Each member in the Interface Set MUST contain the following:

[major] I have issues with the use of Normative language to describe a "conceptual data structure"...specially one that is not needed if only one interface is used.

[major] It sounds like the intent of the Normative language is not to mandate that the data structure contain an Interface.Id entry for each interface, but that each interface can be identified (using an id).  Is that correct?  If so, then please use the Normative language to mandate an ID per interface, and not an entry in a data structure.  [This may require some structural changes to the document.]

  515  Interface.Id
  516      An identifier that is unique in node-local scope, allowing the
  517      AODVv2 implementation to identify exactly one local network
  518      interface.

[major] How long should the Interface.Id be?  32-bits?  Any locally-significant length?

[major] After asking the question above, I noticed that this document doesn't mention Interface.Id (or identifier) anywhere else.  Given that multiple interfaces can share the same IP address, it sounds important to distinguish between them (maybe when sending or receiving)...  Am I missing something?

  520  If multiple interfaces of the AODVv2 router are configured for use by
  521  AODVv2, they MUST be configured in the Interface Set.

[major] Following the comment above about using Normative language...what does "configured in the Interface Set" mean?  The Interface Set is "a conceptual data structure which contains information about all interfaces configured for use by AODVv2", so it sounds like there's a loop if the interfaces are configured in the Interface Set, which contains information about the interfaces (which presumably were configured directly).  IOW, the interfaces are the ones that need to be configured, right?

[minor] Noticing that "Interface Set" is only used once outside of this Section, it seems to me like the whole construct may be unnecessary beyond a high level description.

[nit] BTW, note that in §6.5 the use of Interface Set seems redundant: "all interfaces that have been configured for AODVv2 operation, as given in the Interface Set" -- no need to mention the Interface Set if already mentioning "all interfaces".

  522  Implementations using only one interface do not need the Interface
  523  Set, since their single interface is already uniquely identifiable.

[major] Pointing out that the Interface Set is not needed is a contradiction of the MUSTs used above.


  525 4.2.  Router Client Set

  527  An AODVv2 router discovers routes for its own local applications and
  528  also for its Router Clients that are reachable without traversing
  529  another AODVv2 router.  The addresses used by these devices, and the
  530  AODVv2 router itself, are configured in the Router Client Set. An
  531  AODVv2 router will only originate Route Request and Route Reply
  532  messages on behalf of its configured Router Client addresses.

  534  Router Client Set entries contain:

[major] How are Router Clients discovered, or are they configured?  In several places (4.4, 6.4, 7.2, 9), there's talk about a configured Router Client -- if configured, in a manet, how would the operator know who are its Router Clients?

The terminology definition talked about a Router Client being an address...does that mean that the range is configured (only configured?) and attached devices can use addresses while connected to the router?

I'm confused...  In the end, this may just be an issue with the terminology...

[minor] The list below seems to be incomplete: there are several mentions of RouterClient.SeqNoRtr.

  536  RouterClient.IPAddress
  537      An IP address or the start of an address range that requires route
  538      discovery services from the AODVv2 router.

  540  RouterClient.PrefixLength
  541      The length, in bits, of the routing prefix associated with the
  542      RouterClient.IPAddress.  If the prefix length is not equal to the
  543      address length of RouterClient.IPAddress, the AODVv2 router MUST
  544      participate in route discovery on behalf of all addresses within
  545      that prefix.

  547  RouterClient.Cost
  548      The cost associated with reaching this address or address range.

  550 4.3.  Neighbor Set
...
  574  Neighbor.Timeout
  575      Indicates the time at which the Neighbor.State should be updated:

[nit] So...the Neighbor.Timeout is not a timer, but it represents the absolute time.  Just curious: why was it decided to do it this way?  Just wondering because there seem to be "unknowns" like INFINITY_TIME which could vary...  OTOH, if the concept of (for example) "CurrentTime + MAX_BLACKLIST_TIME" really means "set a timer of duration MAX_BLACKLIST_TIME", then that makes it easier to implement, but that is not how the text is specifying the operation...

[minor] It looks like §6.3 includes all the details about the values of Neighbor.Timeout depending on Neighbor.State.  I think that a reference to that Section is better than repeating the information here.

  577  o  If the value of Neighbor.State is BLACKLISTED, this indicates the
  578      time at which Neighbor.State will revert to HEARD.  This value is
  579      calculated at the time the router is blacklisted and by default is
  580      equal to CurrentTime + MAX_BLACKLIST_TIME.

[major] Is the CurrentTime maintained in seconds (since all the times are in seconds)?  I didn't see that specified anywhere.  It seems awkward to compare/add the current time (12:15pm) to a constant (200 sec)...

  582  o  If Neighbor.State is HEARD, and an RREP_Ack has been requested
  583      from the neighbor, it indicates the time at which Neighbor.State
  584      will be set to BLACKLISTED, if an RREP_Ack has not been received.

  586  o  If the value of Neighbor.State is HEARD and no RREP_Ack has been
  587      requested, or if Neighbor.State is CONFIRMED, this time is set to
  588      INFINITY_TIME.

[nit] A forward reference to §11.3 would be nice...or maybe just make the reference below general (not just about Neighbor.AckSeqNum and Neighbor.HeardRERRSeqNum).

  590  Neighbor.Interface
  591      The interface on which the link to the neighbor was established.

[major] Aha!  I was going to ask how should the interface be represented here...but then I found that §6.3 refers back to §4.1, where the Interface Set is defined.  Does that mean that *.Interface should basically be the Interface.Id?  If so, please say it!

  593  Neighbor.AckSeqNum
  594      The next sequence number to use for the TIMESTAMP value in an
  595      RREP_Ack request, in order to detect replay of an RREP_Ack
  596      response.  AckSeqNum is initialized to a random value.

  598  Neighbor.HeardRERRSeqNum
  599      The last heard sequence number used as the TIMESTAMP value in a
  600      RERR received from this neighbor, saved in order to detect replay
  601      of a RERR message.  HeardRERRSeqNum is initialized to zero.

  603  See Section 11.3 and Section 11.4 for more information on how
  604  Neighbor.AckSeqNum and Neighbor.HeardRERRSeqNum are used.

[major] The text above uses "TIMESTAMP value", at least one "timestamp value" is used...in other places you talk about the "TIMESTAMP TLV".  I had to do a little digging to figure out that the TIMESTAMP value refers to the "value in the TIMESTAMP TLV" (§11.3), which rfc7182 calls "time-value".  Please be consistent (including the case) inside the document and with the terminology already defined elsewhere.

[minor] I had to go digging again to figure out the size of the sequence numbers...went looking at §11.3 and 11.4 because that's what the reference above says...then dug into rfc7182 before figuring out that the length of time-value should be defined in this document...that took me to §11, which pointed be back to §4.4.  It was under my nose the whole time...  Please add a reference to §4.4 somewhere above to make it clear that AckSeqNum's initial random value is in fact 16-bit long...

  606 4.4.  Sequence Numbers

  608  Sequence Numbers enable AODVv2 routers to determine the temporal
  609  order of route discovery messages that originate from a AODVv2
  610  router, and thus to identify stale routing information so that it can
  611  be discarded.  The sequence number fulfills the same roles as the
  612  "Destination Sequence Number" of DSDV [Perkins94], and the AODV
  613  Sequence Number in [RFC3561].  The sequence numbers from two
  614  different routers are not comparable; route discovery messages with
  615  sequence numbers belonging to two different routers cannot be
  616  compared to determine temporal ordering.

  618  Each AODVv2 router in the network MUST maintain its own sequence
  619  number.  All RREQ and RREP messages created by an AODVv2 router
  620  include the router's sequence number, reported as a 16-bit unsigned
  621  integer.  Each AODVv2 router MUST ensure that its sequence number is
  622  strictly increasing, and that it is incremented by one (1) whenever
  623  an RREQ or RREP is created, except when the sequence number is 65,535
  624  (the maximum value of a 16-bit unsigned integer), in which case it
  625  MUST be reset to one (1) to achieve wrap around.  The value zero (0)
  626  is reserved to indicate that the router's sequence number is unknown.

[major] "MUST ensure" is not really Normatively enforceable.  Suggestion: "The sequence number in each router MUST be strictly increasing; it MUST be incremented..."

  628  An AODVv2 router MUST use its sequence number only on behalf of its
  629  configured Router Clients; route messages forwarded by other routers
  630  retain the originator's sequence number.

  632  To determine if newly received information is stale and therefore
  633  redundant compared to other information originated by the same
  634  router, the sequence number attached to the information is compared
  635  to the sequence number of existing information about the same route.
  636  The comparison is carried out by subtracting the existing sequence
  637  number from the newly received sequence number, using unsigned
  638  arithmetic.  The result of the subtraction is to be interpreted as a
  639  signed 16-bit integer.

[nit] You just defined HeardRERRSeqNum above...use it.

  641  o  If the result is negative, the newly received information is
  642      considered older than the existing information and therefore stale
  643      and redundant and MUST therefore be discarded.

  645  o  If the result is positive, the newly received information is newer
  646      than the existing information and is not considered stale or
  647      redundant and MUST therefore be processed.

[major] §11.4 also has Normative text about the comparison of the sequence numbers.  The text about the conditions to discard a packet is not exact, but effectively the same.  However, the condition for processing is simply implicit.  In general, I don't think that defining Normative behavior for the same thing in two different places in the text is a good idea.  A reference (from here to there, or vice-versa) would work better.

  649  o  If the result is zero, the newly received information is not
  650      considered stale, and therefore MUST be processed further in case
  651      the new information offers a better route (see Section 6.7.1 and
  652      Section 6.8).

[major] If the result is zero, doesn't that imply a duplicate message?  §14.1.1 says that "duplicate RREQ messages are dropped", but the text above says that they "MUST be processed".

[major] What does "processed further" mean?  Given that we're talking about a new message that was just received, I'm assuming you really meant "processed".

  ...
  666 4.5.  Local Route Set

  668  All AODVv2 routers MUST maintain a Local Route Set, containing
  669  information obtained from AODVv2 route messages.  The Local Route Set
  670  is considered to be stored separately from the forwarding plane's
  671  routing table (referred to as Routing Information Base (RIB)), which
  672  may be updated by other routing protocols operating on the AODVv2
  673  router as well.  The Routing Information Base is updated using
  674  information from the Local Route Set. Alternatively, if the
  675  information specified below can be added to RIB entries,
  676  implementations MAY choose to modify the Routing Information Base
  677  directly instead of maintaining a dedicated Local Route Set.

[major] There is a Normative contradiction: "routers MUST maintain a Local Route Set...MAY choose to modify the Routing Information Base directly instead of maintaining a dedicated Local Route Set".  Solution: s/MUST/SHOULD

  679  Routes obtained from AODVv2 route messages are referred to in this
  680  document as LocalRoutes, and MUST contain the following information:

  682  LocalRoute.Address
  683      An address, which, when combined with LocalRoute.PrefixLength,
  684      describes the set of destination addresses for which this route
  685      enables forwarding.

  687  LocalRoute.PrefixLength
  688      The prefix length, in bits, associated with LocalRoute.Address.

  690  LocalRoute.SeqNum
  691      The sequence number associated with LocalRoute.Address, obtained
  692      from the last route message that successfully updated this entry.

  694  LocalRoute.NextHop
  695      The source IP address of the IP packet containing the AODVv2
  696      message advertising the route to LocalRoute.Address, i.e., an IP
  697      address of the AODVv2 router used for the next hop on the path
  698      toward LocalRoute.Address.

[minor] §6.10.1 says that "LocalRoute.SeqNum is used to classify future information about LocalRoute.Address as stale or fresh" -- but it doesn't say that there is a close relationship between it and LocalRoute.NextHop because "sequence numbers belonging to two different routers cannot be compared to determine temporal ordering" (§4.4).  It feels to me like that relationship should be highlighted somewhere to make sure that the appropriate sequence number is used when determining whether an address is stale or fresh...

  700  LocalRoute.NextHopInterface
  701      The interface used to send IP packets toward LocalRoute.Address.

[major] Same comment as above for Neighbor.Interface.

  703  LocalRoute.LastUsed
  704      If this route is installed in the Routing Information Base, the
  705      time it was last used to forward an IP packet.  If not, the time
  706      at which the LocalRoute was created.

[minor] It would be really nice if there was a clear relationship between LocalRoute.LastUsed and the interaction with the forwarding plane (§6.4).  As it is, it is not clear how the time is updated.

  708  LocalRoute.LastSeqNumUpdate
  709      The time LocalRoute.SeqNum was last updated.

  711  LocalRoute.MetricType
  712      The type of metric associated with this route.  See Section 5 for
  713      information about AODVv2's handling of multiple metric types.

  715  LocalRoute.Metric
  716      The cost of the route toward LocalRoute.Address expressed in units
  717      consistent with LocalRoute.MetricType.

[major] What does it mean to be "consistent with"?  I understand that for Hop Count, for example the Metric could never be 2.3.  The question really comes from me trying to figure out "what happens if the Metric is not consistent?".  I traced LocalRoute.Metric all the way to TargMetric, but there's no place where a check is done for consistency...

Then I found this text in §5: "A maximum value, denoted MAX_METRIC[MetricType].  This MUST always be the maximum expressible metric value of type MetricType"

...which means that the Hop Count could be up to 256 hops.  In the Overview it is claimed that AODVv2 avoids the count to infinity problem, but it is not clear to me how that is achieved specially because if the cost "is not better (i.e., it is worse or equal)...AdvRte SHOULD be used to update the Local Route Set" (§6.7.1) -- that sounds to me like a recipe for counting to infinity.  But I may be missing something...what am I missing?


  719  LocalRoute.Precursors (optional feature)
  720      A list of upstream neighbors using the route (see Section 10).

[major] Listing this item here contradicts the "MUST" used before the information in the LocalRoutes.  Suggestion: introduce this item in Section 10.

[minor] Should the list of neighbors include a full Neighbor Set entry or just Neighbor.IPAddress?

[nit] LocalRoute.Precursors is not mentioned anywhere else in the document, not even Section 10.

  722  LocalRoute.SeqNoRtr
  723      If nonzero, the IP address of the router that originated the
  724      Sequence Number for this route.

[minor] This definition is confusing to me...  If what is nonzero, the sequence number?  From §4.4, "value zero (0) is reserved to indicate that the router's sequence number is unknown" -- doesn't that mean that the route shouldn't even be considered?  If so, then at this point (when putting stuff in the Local Route Set) a 0 sequence number shouldn't be an issue...right?

§7.2 talks about "the IP address of TargRtr -- i.e., the router that originated the Sequence Number for this RREP"...which I assume is the same thing we're talking about here.  But I can't find who TargRtr is (that term doesn't show up anywhere else)...

In the end, I'm assuming that the text above could be simplified to something like "the IP address of the router that originated this route".

...
  762 4.6.  Multicast Route Message Set

  764  Multicast Route Request (RREQ) messages can be tested for redundancy
  765  to avoid unnecessary processing and forwarding.

[minor] The first paragraph seems to be redundant with the text below.

  767  The Multicast Route Message Set is a conceptual set which contains
  768  information about previously received multicast route messages, so
  769  that incoming route messages can be compared with previously received
  770  messages to determine if the incoming information is redundant or
  771  stale, so that the router can avoid sending redundant control
  772  traffic.

  774  Multicast Route Message Set entries contain the following
  775  information:

[major] Why is there no Normative language used when describing the contents of this set?  Or maybe I should be asking why is Normative language used for the other sets?  All the Sets (except this one and the Router Client Set) include a "MUST" to describe the contents...  I've made some comments above about optional operation/functionality in conflict with the MUSTs...and even not liking the Normative language in the data structure if not needed (Interface Set)...  Do we need these data structures to be described using Normative language?

[minor] The notation used here (McMsg.*) made me go revisit Table 1, which says this:

  | McMsg.Field          | A field in a Multicast Route Message      |
  |                      | entry                                    |
  | RteMsg.Field          | A field in either RREQ or RREP            |

...but this section talks about "Multicast Route Request (RREQ) messages".  What is the difference then between McMsg.* and RteMsg.* when considering RREQs.

  777  McMsg.OrigPrefix
  778      The prefix associated with OrigAddr, the source address of the IP
  779      packet triggering the route request.

[minor] If I've been keeping track correctly, and you followed my comments related to the terminology, I think this is the first time that OrigAddr is used -- this would be a good time to expand it.

  781  McMsg.OrigPrefixLen
  782      The prefix length associated with McMsg.OrigPrefix, originally
  783      from the Router Client Set entry on RREQ_Gen which includes
  784      OrigAddr.

[major] The Router Client Set described above doesn't contain an entry on RREQ_Gen.

  786  McMsg.TargPrefix
  787      The prefix associated with TargAddr, the destination address of
  788      the IP packet triggering the route request.  In an RREQ this MUST
  789      be set to TargAddr.

[minor] Given that the first sentence of this section says that it talks about RREQ, then the McMsg.TargPrefix is always set to TargAddr, right?

  791  McMsg.OrigSeqNum
  792      The sequence number associated with the route to OrigPrefix, if
  793      RteMsg is an RREQ.

[minor] Please see the question above about the difference between McMsg and RteMsg...to me it is confusing because both seem to be talking about the same thing.

  795  McMsg.TargSeqNum
  796      The sequence number associated with the route to TargPrefix.

  798  McMsg.MetricType
  799      The metric type of the route requested.

  801  McMsg.Metric
  802      The metric value received in the RteMsg.

[minor] Would the McMsg.Metric and the McMsg.MetricType both be contained in the RteMsg?

...
  814  McMsg.SeqNoRtr
  815      If nonzero, the IP address of the router that originated the
  816      Sequence Number for this route.

[minor] See my comment above about LocalRoute.SeqNoRtr.

  818  The Multicast Route Message Set is maintained so that no two entries
  819  have the same OrigPrefix, OrigPrefixLen, TargPrefix, and MetricType.
  820  See Section 6.8 for details about updating this set.

  822 4.7.  Route Error (RERR) Set

  824  Each RERR message sent because no route exists for packet forwarding
  825  SHOULD be recorded in a conceptual set called the Route Error (RERR)
  826  Set. Each entry contains the following information:

  828  RerrMsg.Timeout
  829      The time after which the entry SHOULD be deleted.

[major] Are there case where the entry wouldn't be deleted at RerrMsg.Timeout?  IOW, why is the SHOULD not a MUST?

  831  RerrMsg.UnreachableAddress
  832      The UnreachableAddress reported in the AddressList of the RERR.

[nit] A forward reference to §7.4 would be nice.

  834  RerrMsg.PktSource:
  835      The PktSource of the RERR.

[minor] §7.4 says that PktSource is optional.  What if it is not received?  What should this entry be set to?

  837  See Section 6.9 for instructions on how to update the set.

  839 5.  Metrics

  841  Metrics measure a cost or quality associated with a route or a link,
  842  e.g., latency, delay, financial cost, energy, etc.  Metric values are
  843  reported in Route Request and Route Reply messages.

  845  In Route Request messages, the metric describes the cost of the route
  846  from OrigPrefix to the router transmitting the Route Request.  For
  847  RREQ_Gen, this is the cost associated with the Router Client Set
  848  entry which includes OrigAddr.  For routers which forward the RREQ,
  849  this is the cost from OrigPrefix to the forwarding router, combining
  850  the metric value from the received RREQ message with knowledge of the
  851  link cost from the sender to the receiver, i.e., the incoming link
  852  cost.  This updated route cost is included when forwarding the Route
  853  Request message, and used to install a route to OrigPrefix.

[minor] Is this operation of "combining" (adding?) explained somewhere else, or is this it?  The text above is not clear enough.

  855  Similarly, in Route Reply messages, the metric reflects the cost of
  856  the route from TargPrefix to the router transmitting the Route Reply.
  857  For RREP_Gen, this is the cost associated with the Router Client Set
  858  entry which includes TargAddr.  For routers which forward the RREP,
  859  this is the cost from TargPrefix to the forwarding router, combining
  860  the metric value from the received RREP message with knowledge of the
  861  link cost from the sender to the receiver, i.e., the incoming link
  862  cost.  This updated route cost is included when forwarding the Route
  863  Reply message, and used to install a route to TargPrefix.

  865  When link metrics are symmetric, the cost of the routes installed in
  866  the Local Route Set at each router will be correct.  This assumption
  867  is often inexact, but calculating incoming/outgoing metric data is
  868  outside of scope of this document.  The route discovered is good for
  869  the requesting router, but the return path may not be the optimal
  870  route.

[nit] Describing the Local Route Set as "correct" gives the impression that asymmetric links result in some type of error condition.  Explaining the fact that the resulting paths are symmetric and that because of how the metrics are reported, the result may be suboptimal may be better.

  872  AODVv2 enables the use of multiple metric types.  Each route
  873  discovery attempt indicates the metric type which is requested for
  874  the route.  Multiple valid routes may exist in the Local Route Set
  875  for the same address and prefix length but for different metric
  876  types.  More than one route to a particular address and prefix length
  877  MUST NOT exist in the Routing Information Base unless each packet can
  878  be inspected to determine which route in the RIB has the proper
  879  metric type as required for that packet.  Otherwise, only one route
  880  at a time to a particular address and prefix length may exist in the
  881  RIB.  The algorithm used to inspect the packet and make the
  882  determination about which the routes should be installed in the
  883  Routing Information Base is outside the scope of AODVv2.

[?] How does the router decide which metric type to request?

[?] You lost me when talking about inspecting the packet to determine the metric type -- are you talking about a "normal" IP packet to be forwarded?

  885  For each MetricType, AODVv2 requires:

[major] What is a "MetricType"?  I'm not asking what is the MetricType, which seems obvious, but *a* MetricType as in "for each MetricType"?

  887  o  A MetricType number, to indicate the metric type of a route.
  888      Currently allocated MetricType numbers are listed in Section 13.4.

[major] In many (all?) of the notation where MetricType is used as the type itself, it corresponds to the MetricType number (described above), right?  If so, then there seems to be no difference between a MetricType and the MetricType number...this adds to the confusion of this section when listing what a MetricType requires.

  890  o  A maximum value, denoted MAX_METRIC[MetricType].  This MUST always
  891      be the maximum expressible metric value of type MetricType.  Field
  892      lengths associated with metric values are found in Section 13.4.
  893      If the cost of a route exceeds MAX_METRIC[MetricType], the route
  894      cannot be stored and is ignored.

[nit] The "cost of a route" is the same as the "route cost", right?  Please be consistent in the terminology used.

  896  o  A function for incoming link cost, denoted Cost(L).  Using
  897      incoming link costs means that the route obtained has a metric
  898      accurate for the direction back towards the originating router.

[nit] Cost(L) doesn't seem to be a function, just a value...

[comment] So the route towards the destination (the prefix being requested) is not accurate, right?  Not a big deal, just a little ironic that the requested route cant be determined accurately...

  900  o  A function for route cost, denoted Cost(R).

  902  o  A function to analyze routes for potential loops based on metric
  903      information, denoted LoopFree(R1, R2).  LoopFree verifies that a
  904      route R2 is not a sub-section of another route R1.  An AODVv2
  905      router invokes LoopFree() as part of the process in Section 6.7.1,
  906      when an advertised route (R1) and an existing LocalRoute (R2) have
  907      the same destination address, metric type, and sequence number.
  908      LoopFree returns FALSE to indicate that an advertised route is not
  909      to be used to update a stored LocalRoute, as it may cause a
  910      routing loop.  In the case where the existing LocalRoute is
  911      Invalid, it is possible that the advertised route includes the
  912      existing LocalRoute and came from a router which did not yet
  913      receive notification of the route becoming Invalid, so the
  914      advertised route should not be used to update the Local Route Set,
  915      in case it forms a loop to a broken route.

[major] Should the text above use rfc2119 language to define what to do in the last case?  The text sounds to me as if there could be situations where the advertised route could be used, even if it could form a loop.

  917  AODVv2 currently supports cost metrics where Cost(R) is strictly
  918  increasing, by defining:

  920  o  Cost(R) := Sum of Cost(L) of each link in the route

  922  o  LoopFree(R1, R2) := ( Cost(R1) <= Cost(R2) )

  924  Implementers MAY consider metric types that are not strictly
  925  increasing, but the definitions of Cost and LoopFree functions for
  926  such types are undefined, and interoperability issues need to be
  927  considered.

[major] The "MAY" is out of place because there's no Normative action.  s/MAY/may.  Instead of using the Normative language there, you may want to use it to define what MUST be defined if a different type of metric is considered.

=== AD Review of draft-perkins-manet-aodvv2-02 (Part 1) ===

Dear authors:

I decided to start reviewing this document in parallel with Sri (as Shepherd).  Being that this is a long and in-depth draft, my plan is to post partial reviews (maybe about 10 pages at a time) — that is why this message contains “Part 1” in the subject.  I’ll look to post reviews in between working on other documents in my queue.

Please go ahead and address the comments, discuss, post updates at any time — no need to wait for the final part.

Disclaimer: I only skimmed some of the text not included in this review.  This means that some of my questions/comments may already be addressed somewhere else, but also that I reserve the right to revisit the earlier sections if something in the future triggers that.  Also, I haven’t done a thorough review of the WG mail archive.


This review (Part 1) covers the first 3 Sections.  Even though these Sections include introductory material, I have a significant number of comments.  In particular, some of the Major items that I call out below are:

(1) What is the relationship between this document and rfc3561?

(2) The Terminology section includes a lot of entries that I don’t think belong there: abbreviations, elements or variables also described elsewhere.

Please see details below.

Thanks!

Alvaro.


[The line numbers come from the idnits output.]


...
  168 1.  Overview

  170  The Ad hoc On-Demand Distance Vector Version 2 (AODVv2) protocol
  171  enables dynamic, multihop routing between participating mobile nodes
  172  wishing to establish and maintain an ad hoc network.  The basic
  173  operations of the AODVv2 protocol are route discovery and route
  174  maintenance.  AODVv2 does not require nodes to maintain routes to
  175  destinations that are not in active communication.  AODVv2 allows
  176  mobile nodes to respond to link breakages and changes in network
  177  topology in a timely manner.  The operation of AODVv2 is loop-free,
  178  and by avoiding the Bellman-Ford "counting to infinity" problem
  179  offers quick convergence when the ad hoc network topology changes
  180  (typically, when a node moves in the network).  When links break,
  181  AODVv2 causes the affected set of nodes to be notified so that they
  182  are able to invalidate the routes using the lost link.

[minor] An Informative reference to "the Bellman-Ford "counting to infinity" problem" would be nice.  I know what it is, but the casual reader (as in IESG reviewers) may not.

  184  One distinguishing feature of AODVv2 is its use of a destination
  185  sequence number for each route entry.  The destination sequence
  186  number is created by the destination to be included along with any
  187  route information it sends to requesting nodes.  Using destination
  188  sequence numbers ensures loop freedom and is simple to program.
  189  Given the choice between two routes to a destination, a requesting
  190  node is required to select the one with the greatest sequence number.

[minor] It looks like the "destination sequence number" is discussed in §4.4 -- please put a forward reference here.  However, that section just uses "sequence number" (not "destination sequence number") to describe the functionality for AODVv2.  Please be consistent in the use of names/terminology.

  192  Compared to AODV [RFC3561], AODVv2 has moved some features out of the
  193  scope of the document, notably intermediate route replies, expanding
  194  ring search, and route lifetimes.  However, the document has been
  195  designed to allow their specification in a separate document.  Hello
  196  messages and local repair have been removed.  AODVv2 provides a

[major] What is the formal relationship between this document and rfc3561?  The text above sounds as if AODVv2 is the evolution of AODV (which makes sense); should this document Obsolete rfc3561?

[minor] (Pending the response from the last question.) It would be very nice if there was a section that talks about the differences between AODV and AODVv2, specially about the major things that were removed/added/changed.  For example, intermediate route replies seem useful to me -- so understanding the reason to leave them out would be nice.  Maybe this could go in an Appendix.  Note that this item is just "nice to have", not a blocking comment.

[major] I'm confused by the statement of moving "some features out of the scope of the document...[while]...allow their specification in a separate document."  Do those other documents exist?  Are they planned?  Having an extensible protocol is great...but my confusion comes from the fact that the text sounds as if you're talking about an "extensible document".

[minor] It looks like the Hellos have been replaced with NHDP, is that true?  It might be nice to mention that somewhere.  I only found one reference to NHDP (in §6.2) -- I'm not sure if more is needed to understand the relationship between it and AODVv2.

  197  mechanism for the use of multiple metric types.  Message formats have
  198  been updated and made compliant with [RFC5444].  AODVv2 control
  199  messages are defined as sets of data, which are mapped to message
  200  elements using the Generalized MANET Packet/Message Format defined in
  201  [RFC5444] and sent using the parameters in [RFC5498].  Verification
  202  of link bidirectionality has been substantially improved, and
  203  additional refinements made for route timeouts and state management.
  204  Finally, multihoming is now supported.


...
  237 2.  Terminology

  239  The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
  240  "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
  241  "OPTIONAL" in this document are to be interpreted as described in
  242  [RFC2119].  In addition, this document uses terminology from
  243  [RFC5444], and defines the following terms:

[nit] Please use the rfc8174 template.

[minor] Some of the terminology included is not really about terms, but about expanding (or explaining abbreviations)  I think those are better served when first used.

[major] In general, it seems to me like pretty much every term in this section doesn't belong here because it represents an abbreviation, it is an element or variable described later in the text...  I have comments below for many entries...


  245  AddressList
  246      A list of IP addresses as used in AODVv2 messages.

[major] There are several terms (including AddressList) defined here that represent fields in messages and that should be fully defined there (and not partially here).  IOW, please remove field names from this section.  If the names are used in the text before they are properly defined, please include a forward reference.

  248  AckReq
  249      Used in a Route Reply Acknowledgement message to indicate that a
  250      Route Reply Acknowledgement is expected in return.

[major] AckReq is an element (as defined in §7.3), not a term.

  252  AdvRte
  253      A route advertised in an incoming route message.

[minor] AdvRte is really an abbreviation...and better described in §6.7.  Note also that the definitions throughout the text are similar, but not exactly the same: "the route contained within [the received route message]" (§6.7), "incoming advertised route" (§6.7.1)

  255  AODVv2 Router
  256      An IP addressable device in the ad hoc network that performs the
  257      AODVv2 protocol operations specified in this document.

  259  CurrentTime
  260      The current time as maintained by the AODVv2 router.

[major] Some terms (including CurrentTime) represent variables used later in the explanation of the protocol.  I think these would be better explained in the specific sections.  IOW, the terminology section shouldn't be used to initialize variables.

  262  ENAR (External Network Access Router)
  263      An AODVv2 router with an interface to an external, non-AODVv2
  264      network.

  266  Interface Set
  267      The set of all network interfaces supporting AODVv2.

  269  Invalid route
  270      A route that cannot be used for forwarding but still contains
  271      useful sequence number information.

[minor] This definition is unnecessary.  The Invalid state is better explained later in §4.5 -- and the qualification of a route as invalid becomes obvious then.  Note also that Invalid is also the only state to be defined in this Section...

  273  LocalRoute
  274      An entry in the Local Route Set as defined in Section 4.5.

[nit] If the term is actually defined somewhere else, then we probably don't need to define it here.

In any case, §4.5 says that "Routes obtained from AODVv2 route messages are referred to in this document as LocalRoutes"...which is not exactly the same definition as above.  To be fair, earlier it also points out the "Local Route Set, containing information obtained from AODVv2 route messages"...there is some relation with the definitions, but the reader has to work to find it.  That is also a good reason to keep the definition in one place.

  276  MANET
  277      A Mobile Ad Hoc Network as defined in [RFC2501].

[major] I didn't find an explicit definition of MANET in rfc2501.  It then looks like the term above is just an expansion...which would be better positioned when MANET is used in the text for the first time -- the first sentence of §3 seems ideal.

  279  MetricType
  280      The metric type for a metric value included in a message.

[major] This one seems like an obvious abbreviation.  But it is also better discussed/described in §5.  Note that the text there seems to describe MetricType much more in depth that one-line ever could.

  282  MetricTypeList
  283      A list of metric types associated with the addresses in the
  284      AddressList of a Route Error message.

[major] This is a field in the RERR messge...better described there.

  286  Neighbor
  287      An AODVv2 router from which an RREQ or RREP message has been
  288      received.  Neighbors exchange routing information and verify
  289      bidirectionality of the link to a neighbor before installing a
  290      route via that neighbor into the Local Route Set.

[minor] §4.3 does a far better job at completely defining neighbors.  The second part of the text tries to summarize the operation in one sentence...

  292  OrigAddr
  293      The source IP address of the IP packet triggering route discovery.

[minor] This one, at best, sounds like a contraction.  In cases like this, what you may want to do is to define "Originator Address" (for example) and then say that OrigAddr is how it is referred to in the text.

  295  OrigMetric
  296      The metric value associated with the route to OrigPrefix.

  298  OrigPrefix
  299      The prefix configured in the Router Client Set entry which
  300      includes OrigAddr.

  302  OrigPrefixLen
  303      The prefix length, in bits, configured in the Router Client Set
  304      entry which includes OrigAddr.

  306  OrigSeqNum
  307      The sequence number of the AODVv2 router which originated the
  308      Route Request on behalf of OrigAddr.

  310  PktSource
  311      The source address of the IP packet that triggered a Route Error
  312      message.

  314  PrefixLengthList
  315      A list of routing prefix lengths associated with the addresses in
  316      the AddressList of a message.

[major] As with AddressList, these 6 are fields better described elsewhere...

  318  Reactive
  319      Performed only in reaction to specific events.  In AODVv2, routes
  320      are requested only when data packets need to be forwarded.  In
  321      this document, "reactive" is synonymous with "on-demand".

[nit] Defining "reactive" by using "reaction" is a circular definition.  Maybe use "in response to" (or something like that).

  323  RERR (Route Error)
  324      The AODVv2 message type used to indicate that an AODVv2 router
  325      does not have a valid LocalRoute toward one or more destinations.

  327  RERR_Gen (RERR Generating Router)
  328      The AODVv2 router generating a Route Error message.

  330  RerrMsg (RERR Message)
  331      A Route Error (RERR) message.

[major] So...RerrMsg is a "REER Message"...but RERR is itself a type of "AODVv2 message".  Seems redundant...or maybe incomplete since (§4.7) a RERR set is also considered.  In any case, it is better to describe these where the messages are defined.  I also note that Table 1 does a pretty good job in explaining the meaning of ReffMsg anyway...

  333  Routable Unicast IP Address
  334      A unicast IP address that is scoped sufficiently to be forwarded
  335      by a router.  Globally-scoped unicast IP addresses and Unique
  336      Local Addresses (ULAs) [RFC4193] are examples of routable unicast
  337      IP addresses.

[minor] I find the definition (specifically the used of "scoped") to be unclear.  However, maybe it doesn't matter since this term is not used anywhere in the text.

  339  Router Client
  340      An address within an address range configured on an AODVv2 router,
  341      on behalf of which that router will initiate and respond to route
  342      discoveries.  These addresses may be used by the AODVv2 router
  343      itself or by devices that are reachable without traversing another
  344      AODVv2 router.

[major] The definition here says that a Router Client is an address.  But §4.2 says that a "router discovers routes...for its Router Clients that are reachable without traversing another AODVv2 router.  The addresses used by these devices...", which then implies that a Router Client is not an address, but a device.  And then the Router Client Set is described (to include an IP address, but also a cost and prefix length).  In short, the definition here is not clear/consistent with respect to the rest of the text...and it would be better to simple provide a clear definition in §4.2...or a short summary here.

  346  RREP (Route Reply)
  347      The AODVv2 message type used to reply to a Route Request message.

  349  RREP_Gen (RREP Generating Router)
  350      The AODVv2 router that generates the Route Reply message, i.e.,
  351      the router configured with TargAddr as a Router Client.

  353  RREQ (Route Request)
  354      The AODVv2 message type used to discover a route to TargAddr and
  355      distribute information about a route to OrigPrefix.

  357  RREQ_Gen (RREQ Generating Router)
  358      The AODVv2 router that generates the Route Request message, i.e.,
  359      the router configured with OrigAddr as a Router Client.

  361  RteMsg (Route Message)
  362      A Route Request (RREQ) or Route Reply (RREP) message.

[major] Same comments as for RERR/RerrMsg (above).

  364  SeqNum
  365      The sequence number maintained by an AODVv2 router to indicate
  366      freshness of route information.

  368  SeqNumList
  369      A list of sequence numbers associated with the addresses in the
  370      AddressList of a message.

  372  TargAddr
  373      The target address of a route request, i.e., the destination
  374      address of the IP packet triggering route discovery.

  376  TargMetric
  377      The metric value associated with the route to TargPrefix.

  379  TargPrefix
  380      The prefix configured in the Router Client Set entry which
  381      includes TargAddr.

  383  TargPrefixLen
  384      The prefix length, in bits, configured in the Router Client Set
  385      entry which includes TargAddr.

  387  TargSeqNum
  388      The sequence number of the AODVv2 router which originated the
  389      Route Reply on behalf of TargAddr.

[major] Back to contractions/abbreviations, fields, elements...

  391  Unreachable Address
  392      An address reported in a Route Error message, as described in
  393      Section 7.4.1.

[minor] §7.4.1 talks about the generation of RERRs, but it doesn't explain what an unreachable address is.  IOW, there is no definition here.

[major] Note that the definition of RERR (above) talks about the router not having a "valid LocalRoute", not an address.  Also, §4.5 talks about a LocalRoute being a route...not an address.  At some point a route is expressed as an address...but the point here is that the definitions/terminology is not consistent.  Please be consistent!

[major] If something is already defined elsewhere, please just define it in one place.

[minor] I think that "unreachable address" is relatively clear as used in the text (without needed to define it).

[major] §4.5 (Local Route Set) talks about an "Invalid" route as one that "has expired or has broken".  Is that the same thing as Unreachable?

  395  Upstream
  396      In the direction from destination to source (from TargAddr to
  397      OrigAddr).

[minor] Even though the reader can figure out the direction reference (destination to souce of what?), please be specific here.

[minor] If upstream is defined, why is downstream not included?

  399  Valid route
  400      A route that can be used for forwarding.

[major] Again, I think that this definition is better served in §4.5, where the specific qualities are described.


...
  418 3.  Applicability Statement

...
  436  AODVv2 handles a wide variety of mobility and traffic patterns by
  437  determining routes on-demand.  In networks with a large number of
  438  routers, AODVv2 is best suited for relatively sparse traffic
  439  scenarios where each router forwards IP packets to a small percentage
  440  of destination addresses in the network.  In such cases fewer routes
  441  are needed, and far less control traffic is produced.  In large
  442  networks with dense traffic patterns, AODVv2 control messages may
  443  cause a broadcast storm, overwhelming the network with control
  444  messages.  The transmission priorities described in Section 6.5
  445  prioritize route maintenance traffic over route discovery traffic.

[major] "In large networks...AODVv2 control messages may cause a broadcast storm..."  That sounds pretty serious to me, but I see nothing discussed in the Security Considerations section (or anywhere else).  §14.1 is related, but it doesn't specifically address this point.

To be fair, the text above really means that AODVv2 shouldn't be used in "large networks with dense traffic patterns" (after all, it's part of the applicability statement).  However, qualifying "large" and "dense" is not easy, so I would like to see something in §14 (maybe §14.1) that at least mentions the problem and any potential mitigation.  I'm looking for something like this: "in large networks with dense traffic patterns...a broadcast storm may result...but it can be mitigated by using a, b, or c..."...or maybe it can't, but calling it out is important.

  447  Data packets may be buffered until a route to their destination is
  448  available, as described in Section 6.6.

[minor] When thinking that "buffering might be configured for IP packets awaiting a route...if sufficient memory is available" [§6.6], which sounds optional and or course not guaranteed...and the example below of "emergency and disaster relief, where the ability to communicate [is] important"...makes me think: if buffering is not configured (or misconfigured, even if "appropriate buffer methods are out of scope"), and a route is not available, how can communication be guaranteed?

I don't know that the possibility of a failed "emergency and disaster relief" communication is a security issue with the protocol, but at least something to be taken into consideration when implementing and operating a network.

It would be great if this document included an Operations Considerations section to include topics like that in a single place.  Take a look at rfc5706.

  450  AODVv2 is well suited to reactive scenarios such as emergency and
  451  disaster relief, where the ability to communicate might be more
  452  important than being assured of secure operations.  For many other ad
  453  hoc networking applications, in which insecure operation could negate
  454  the value of establishing communication paths, it is important for
  455  neighboring AODVv2 routers to establish security associations with
  456  one another.

  458  AODVv2 provides for message integrity and security against replay
  459  attacks by using integrity check values, timestamps and sequence
  460  numbers, as described in Section 14.  When security associations have
  461  been established, encryption can be used for AODVv2 messages to
  462  ensure that only trusted routers participate in routing operations.

[nit] Forward references to §11 and §14 would be nice.

...
  475  AODVv2 supports routers with multiple interfaces and multiple IP
  476  addresses per interface.  A router may also use the same IP address
  477  on multiple interfaces.  AODVv2 requires only that each interface
  478  configured for AODVv2 has at least one unicast IP address.  Address
  479  assignment procedures are out of scope for AODVv2.

[minor] The requirement that "each interface...has at least one unicast IP address" sounds contradictory to the ability to share.  Clarifying, or pointing at §4.1 might help.


…
  484  The routing algorithm in AODVv2 has been operated at layers other
  485  than the network layer, using layer-appropriate addresses.

[minor] What does that paragraph means?  At best, it seems to not belong in this document.