Re: [manet] draft-ietf-manet-aodvv2-15 review

[Lotte Steenbrink <[e-mail]>]

Hi Jiazi,
thank you for your detailed review. A few comments and questions inline. (I wanted to get this reply out the door as soon as I could, so I/we will follow up regarding the yet unresponded to items later)

> Am 28.04.2016 um 16:53 schrieb Jiazi YI <[e-mail]>:
> 
> Hi, 
> 
> Here is my preliminary review on aodvv2-15. There are still some points that I'm not sure that I have fully understood -- I will get back to them if I had time. 
> 
> A summary of my review:
> 
> 1. The specification lacks of a "big picture" of the protocol functioning. People prefer to have a general idea of the whole protocol at the very beginning. This is particularly important for readers who are not familiar with reactive protocol in general. 
> Some of the sections can be reorganized a bit to improve the readability. For example, with the current structure, the readers need to reach section 6.6 (Route discovery) before they can really touch the core function of the protocol. 
> 
> 2. As an experimental protocol, the part related to experiments needs to be improved. IMO, at least the following information must be provided:
>        - The motivation of this experimental document 
>        - The experiments to be conducted 
>        - What kind of feedback is expected to reported.  
> 
> 3. There are some redundant processing procedures in the draft. On the other hand, some connections between the sections seem to be missing. Sometimes it's hard to build connection between the message processing and related information bases. 
> I think this is partially due to the lack of implementation, or the implementation experience is not reported to the author team?
> 
> 4. The specification uses lots of multicast messages. While multicast RREQ is inevitable, the necessity of multicast RREP/RERR is not clear. It bring issues related to control message overhead and security. 
> 
> 5. The interaction with RIB: The route state maintenance requires "LocalRoute.State MUST be checked before IP packet forwarding and before any operation based on LocalRoute.State.". Having such check for each data packet might be heavy load for the system if the routing daemon runs in userspace (because of context switch). While interaction with RIB is inevitable, we want to reduce it as much as possible -- per data packet operation is too much. 

Hmmm. Yeah. To me it would make more sense to say "if you have a RIB, you MUST update it any time LocalRoute.State changes [so instead of the polling described above, you’d let the changes „trickle up“ and avid constant context switches]. If you don’t have a RIB and LocalRoute is consulted directly when forwarding a packet [this would’ve been the case in RIOT a few releases ago.. but there’s no kernel vs user space there either, so you wouldn’t have the expensive context switches], you MUST check LocalRoute.State before you forward to make sure the route is still valid.“ ...

> 
> 6. There are too many "MAY" and "SHOULD" in the document without elaboration. Some of which might cause interoperability issues or even loops. 
> 
> 8. security issues. There are discussions going on in different threads, so I won't repeat them here. I think the current regeneration-based approach is complex to secure and hard for future extensions.  

We’ve gotten rid of that approach as of now. :)

> I haven't checked the security considerations section in detail yet. It's important to get the big direction right first. 
> 
> Please check the details inline:
> 
> section 1, overview
> 
>    The Ad hoc On-Demand Distance Vector Version 2 (AODVv2) protocol
>    enables dynamic, self-starting, multihop routing
> 
> The term "self-starting" is very vague. It makes people think of auto-configuration. And I didn't see such kind of feature is supported. I would avoid using it. 

Makes sense. I’ve removed the term.

> 
>   AODVv2 is an Experimental protocol.
>  ...
>  
> I think much more information is required here:
> 
> 1) Because RFC 3561 has already been experimented for 13 years, as its version 2, AODVv2 should summarize what kind of experiences have been obtained in the past 13 years, and justify the design choices made in the current draft (like use of RFC5444, removing some of the features, etc)
> 
> 2) The motivation and experiments to be conducted need to be elaborated. It's even necessary to have a separate section/subsection for it. Some experiment points that come to my mind without much thinking: support of multiple interfaces, support of gateways, use of different metrics, the security model, etc..

Fair enough.  Thank you for the detailed pointers, I’ll write something up (unless my co-authors beat me to it ;) ) and get back to you, if that’s okay?

> 
>    The basic operations of the AODVv2 protocol are route discovery and
>    route maintenance...
> 
> It's necessary to give an overview of the basic functioning of the protocol, rather than simply saying "use RREQ/RREP to carry information and build routes". Because after this, the draft jumps directly into the protocol details. 
> It's extremely important for the readers who are not familiar with reactive protocol.

Good point. we’ve removed „The basic operations of the AODVv2 protocol are route discovery and route maintenance.“, since that sentence occurs in the first paragraph already, and we’ve substituted the „An AODVv2 router is configured to perform route discovery…“ paragraph with:

   The basic protocol mechanisms are as follows.  Since AODVv2 is a
   reactive protocol, route discovery is initiated only when a route to
   the target is needed (i.e. when a router' client wants to send data).
   AODVv2 does this with the help of a Route Request (RREQ) and Route
   Reply (RREP) cycle: an RREQ is distributed across the network until
   it arrives at the target.  When forwarding an RREQ, all routers
   across the network store the neighbor they've received the RREQ from,
   memorizing a possible route back to the originator of the RREQ.  When
   the target receives the RREQ, it answers with an RREP, which then
   travels back to the originator along the path memorized by the
   intermediate routers.  A metric value is included within the messages
   to record the cost of the route.  AODVv2 uses sequence numbers to
   identify stale routing information, and compares route metric values
   to determine if advertised routes could form loops.

we’ve also slightly updated the language of the next pararaph:

   Route maintenance includes confirming bidirectionality of links to
   next hop AODVv2 routers and issuing Route Error (RERR) messages
   informing other routers of broken links.  It also includes reacting
   to received Route Error messages, and extending and enforcing route
   timeouts.

What do you think?

>  
> 
> section 2, Terminology
> 
> Please pay attention to the style of the terms. Most of the time, style like AckReq is used, but sometimes RERR_Gen is used. 
> Good to be consistent. 

I think I’ve pointed out a similar issue too (in spring 2014, I think?), but I think I recall the conclusion being that while the draft should stick to CamelCase wherever it can, renaming RERR_Gen to RERRGen would have a negative impact on readability, so there are some exceptions to the rule…

> 
>    Unreachable Address
>       An address reported in a Route Error message, as described in
>       Section 7.4.1.
> 
> need to explain what's the address for exactly, rather than giving a pointer to a big section. 

We had a longer explanation there but Justin said we should shorten it, so we did that and added the pointer… :D

> 
> section 3, applicability statement
> 
>    AODVv2 handles a wide variety of mobility and traffic patterns by
>    determining routes on-demand...
> 
> It necessary to explicitly call out what kind of traffic patterns that reactive protocols are not suitable for, such as lots of routers send data packets to instantaneous and multiple destinations. If that happens, what's the consequence (broadcasting storm, etc). 

Good point. We’ve reworded the paragraph to..

   AODVv2 handles a wide variety of mobility and traffic patterns by
   determining routes on-demand.  In networks with a large number of
   routers, AODVv2 is best suited for relatively sparse traffic
   scenarios where each router forwards IP packets to a small percentage
   of other AODVv2 routers in the network.  In this case fewer routes
   are needed, and therefore less control traffic is produced.  In large
   networks with very frequent or bursty traffic, AODVv2 control
   messages may cause a broadcast storm, overwhelming the network with
   control messages and preventing routes from being established.  This
   especially applies to networks with point-to-point or point-to-
   multipoint traffic.  In this case, the transmission priorities
   described in Section 6.5 prioritize route maintenance traffic over
   route discovery traffic.

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

Does that help?

>  
>    AODVv2 provides for message integrity and security against replay
>    attacks by using integrity check values, timestamps and sequence
>    numbers, as described in Section 12.  If security associations can be
>    established, encryption can be used for AODVv2 messages to ensure
>    that only trusted routers participate in routing operations. 
> 
> We need to call out the hop-by-hop security model used, and the network MUST apply necessary mechanisms to make sure that only trusted routers can participate in routing operations. 

We’re currently working on getting rid of the hop by hop trust model in favor of end to end security. (I believe the thread is "Re: On Forwarding-and-regeneration (was: Re: draft-ietf-manet-aodvv2-13 review - a couple of big ticket Items)“?)

>  
>    The routing algorithm in AODVv2 MAY be operated at layers other than
>    the network layer, using layer-appropriate addresses.
> 
> Considering the deep coupling between AODVv2 and IP, I don't see how it can work. This should be removed. 
> 
> section 4 data structures
> 
> 4.1 Interface set
> 
> The InterfaceSet is a conceptual data structure which contains
>    information about all interfaces available to AODVv2.
> ...
> Otherwise the InterfaceSet MAY be empty.
> 
> I'm not sure the sec MAY be empty -- a router contains at least one interface, no? 

Yes, but if it knows it only has one and it has access to this information elsewhere and this won’t change, it might as well save the bytes of stating this again… Iirc the idea was to give implementations for constrained devices a bit of leeway there.

> 
> 4.2 router client set
>    RouterClient.Cost
>       The cost associated with reaching this address or address range.
> 
> Both "metric" and "cost" are used in the draft. If they are the same, please pick a term and use it consistently. 

Cost refers to the result of the Cost(L) function for the link to the RouterClient. Would it make more sense it this entry was called LinkCost?

>  
> The AODVv2 router adding the Router Client MUST wait for
>    any existing routing information about this Router Client to be
>    purged from the network, i.e., at least MAX_SEQNUM_LIFETIME since the
>    last SeqNum update on the router which is removing this Router
>    Client. 
> 
> I'm not sure that I parsed this sentence correctly, but the adding/removing of router client seems to be more an external operation and out of the control of routing protocol itself. 

…But even when it has no influence on when/whether a router is added/removed, shouldn’t it still be able to deal with the consequences? So if an AODVv2 outer is assigned a client that has previously been attached to a different router, it should make sure it doesn’t confuse the network (which is achieved by waiting >= MAX_SEQNUM_LIFETIME before fully treating it as its own client)?

> 
> 4.3 neighbor set
> 
>  Neighbor.State ... There are three possible states: Confirmed, Unknown, and
>       Blacklisted.  Unknown is the initial state.
> 
> I think the "Unknown" is a bit confusing here. It's actually something we already know -- a state that "we heard from that node, but bi-directionality is not confirmed". 
> I think "Heard" is a more appropriate term to be used here. 
> 
> 4.4 sequence numbers
> 
> I think the comparison in wrap-around cases should be specified here. 
> 
> Tod determine staleness,
> 
> typo

Whoopsie, thanks

> 
> 4.5 Local route set
> 
> Alternatively, implementations
>    MAY choose to modify the Routing Information Base directly.
> 
> This should be either more precise (in which condition, how we can modify the RIB?), or be removed.  

Isn’t the how really implementation specific?

> 
>  
>    Multiple valid routes for the same address and prefix length but for
>    different metric types may exist in the Local Route Set, but the
>    decision of which of these routes to install in the Routing
>    Information Base to use for forwarding is outside the scope of
>    AODVv2.
> 
> It's OK, but maybe some rules must be specified and followed. For example, the metric used must be the same: if a packet is forwarded using metric A at router X, and then metric B at router Y, it's a obvious source of loops. 

Of course, but that’s an issue of properly establishing routes, which should be specified farther along in the draft, shouldn’t it? The purpose of this paragraph is to say „you can have multiple valid routes (each following one metric type) to the same destination, but we won’t specify how to pick which metric type is most important to you (i.e. which route you trust the most).“

> 
> 4.6.  Multicast Route Message Set
> 
> Since this set is mainly for duplicate packet detection, I think some of the fields are kind of redundant. For example, an RREQ/RREP message can be identified by its originator's address and sequence number, why 6 fields (OrigAddr, OrigFprefixLen, OrigSeqNum, TargAddr, TargPrefixLen, TargSeqNum) are necessary here. 
> 
> section 5 Metrics
> 
>    In Route Request messages, the metric describes the cost of the route
>    from OrigAddr (and any other addresses included in the prefix length
>    of RREQ_Gen's Router Client entry for OrigAddr) to the router sending
>    the Route Request. 
> 
> So the metric carried in the RREQ is the metric from Router Client to the RREQ_Gen plus the metric from RREQ_Gen to the intermediate router. 
> In the Router Client Set, we have only one field for RouterClient.Cost. On the other hand, "AODVv2 supports different metrics". 
> 
>           routerClient.cost             AODVv2 metric 
> client-----------------------RREQ_gen---------------
> 
> What if those two metrics are of different types? This is quite possible because the two links might use different medium. 
> 
> 
> section 6 AODVv2 protocol Operations
> 
> General comments: 
> 
> I think this section is hard to read and needs some reconstruction. Generally speaking, people prefer begin from the important operations to have a big picture/general idea of the protocol functioning, and then the details. 
> With the current organization, one needs to read till section 6.6 (Route discovery, retries and buffering) before he/she could reach the core functioning of the protocol. 
> 
> I don't quite get the current organization of sections either. For example:
> 
> section 6.2 (Next hop monitoring) and section 6.3 (Neighbor set update) are very related. One is about the function, and one is about the corresponding operation. The following sections don't follow this pattern -- why not making them to one?
> 
> section 6.7.1 (Evaluating route information) and section 6.8 (Suppressing redundant messages using Multicast Route message set) seem to have some overlapped operations. They all need to check the uniqueness of the messages, compare the metrics and stop processing when duplication is detected. Although they are applied to different information bases, they are doing the same thing. 
> 
> section 6.9 (suppressing redundant RERR messages using the Route Error set) should obviously a part of section 6.10.2 (reporting invalid routes), but it's a separate section with higher hierarchy. 
> 
> more details inline:
> 
>    If such an entry does not exist, a new entry is created as described
>    in Section 6.3.  While the value of Neighbor.State is Unknown,
>    acknowledgement of RREP messages sent to that neighbor MUST be
>    requested. ...
> 
> I think we need to precise this is one for the case of RREP transmission. 
> 
> section 6.3
> 
>    If an external mechanism reports a link as broken, the Neighbor Set
>    entry SHOULD be removed.
> 
> I don't know why SHOULD, not MUST is used here. In fact, this is the only place that I found that a Neighbor Set entry to be removed -- considering the network is with constrained devices in a dynamic environment, isn't a timer-based removing mechanisms is more adapted?
>  
>   an RREP which answers a RREQ sent within RREQ_WAIT_TIME over the
>       same interface as Neighbor.Interface
> 
> I'm not a native English speaker, so I'm not sure about it should be "a RREP" or "an RREP". But I think there is no reason to use them differently for RREQ/RREP? Need to check the consistency in the whole draft. 

The „a RREQ“ is probably my fault, since I find that notation more intuitive as well, but we’ve decided that it should be „an RREQ“, because you’d pronounce it „arr-reck“, which warrants an „an“ in front of it. Thanks for the hint, I’ve checked the draft and found more of these errors, all fixed now.

> 
> section 6.4.  Interaction with the Forwarding Plane
> 
>    o  A route has been updated: update the corresponding route in the
>       Routing Information Base.
> 
> Do we need to pay attention to the "multi-metrics" issue here? To support multi-metrics, do we need special fields in the RIB? If so, we need to call it out in the applicability statement. 

I’d guess it depends where you’re choosing one route over the other? If the RIB supports multiple metrics and can sort it out that’s great, if not you might want to decide which route to propagate on to the RIB based on its metric, or you might want to „translate“ metrics into differently prioritized routes in the RIB? (if that makes sense, it should probably be mentioned in 4.5.  Local Route Set though)

> 
> section 6.5.  Message Transmission
> 
>    To implement the congestion control, a queue length is set.  If the
>    queue is full, in order to queue a new message, a message of lower
>    priority must be removed from the queue.  If this is not possible,
>    the new message MUST be discarded.  The queue should be sorted in
>    order of message priority. 
> 
> I think it's more implementation details rather than specification.  
> 
> section 6.6 
> 
> The constants used inSection 6.7.1 this section are
>    defined in Section 10.
> 
> misplaced reference

Oh, thanks. Fixed.

> 
> section 6.7.1
> 
>    4.  Compare route costs
>        *  If AdvRte is better than all matching LocalRoutes, it SHOULD
>           be used to update the Local Route Set because it offers
>           improvement.  If it is not used to update the Local Route Set,
>           the existing non-optimal LocalRoute will continue to be used,
>           causing data traffic to use a non-optimal route.
> 
> If some routers made the update, and some routers choose not updating, I'm afraid there will be loops in the network. 
> 
>        *  If AdvRte is worse and LocalRoute is valid, ignore AdvRte for
>           further processing.  AdvRte MUST NOT be used to update the
>           Local Route Set because it does not offer any improvement.
>        *  If AdvRte is not better (i.e., it is worse or equal) but
>           LocalRoute is Invalid, AdvRte SHOULD be used to update the
>           Local Route Set because it can safely repair the existing
>           Invalid LocalRoute.
> 
> shouldn’t worse AdvRte is already filtered by the LoopFree function in the previous step 3?
> 
> section 6.7.2.  Applying Route Updates
> If there is no existing matching route,
>    AdvRte allows a corresponding RREP to be sent. 
> 
> I don't quite get why this relevant to RREP sending here. 

Yeah… Honestly I don’t get this one either. I’d just delete it? Anybody against that?

>  
>    7.  If an existing LocalRoute.State changed from Invalid or
>        Unconfirmed to become Idle, any matching Unconfirmed LocalRoute
>        with worse metric value SHOULD be expunged.
>    8.  If an existing LocalRoute was updated with a better metric value,
>        any matching Unconfirmed LocalRoute with worse metric value
>        SHOULD be expunged.
>    9.  If this update results in LocalRoute.State of Active or Idle,
>        which matches a route request which is still in progress, the
>        associated route request retry timers SHOULD be cancelled.
> 
> why "SHOULD", not "MUST" here? 
> 
> 6.8.  Suppressing Redundant Messages Using the Multicast Route Message
>       Set
> 
> the processes defined in this section overlap with section 6.7.1 (bullet 1-2-3-4)
> 
> section 6.10 Local route set maintenance
>  
>    During normal operation, AODVv2 does not require any explicit
>    timeouts to manage the lifetime of a route.  At any time, any
>    LocalRoute MAY be examined and updated according to the rules below.
>    If timers are not used to prompt updates of LocalRoute.State, the
>    LocalRoute.State MUST be checked before IP packet forwarding and
>    before any operation based on LocalRoute.State.
> 
> I don't think this is an efficient approach. If the routing daemon runs in user space (which is true in most cases), this normally means context switch. This is inevitable for reactive protocols, but doing so per packet seems to be expensive. 

see above :)

> 
> 
> section 7 AODVv2 Protocol Messages
> 
> general comments:
>   - the message regeneration, rather than the message forwarding, plus different optional fields make the protocol hard to secure 

fixed now

>   - the tradeoff of using multicast RREP and RERR need to be carefully considered. 
>   - the tradeoff of using ValidityTime needs to be carefully considered

We’ve removed ValidityTime just now

> 
> section 7.1
>  
>    PrefixLengthList
>       Contains OrigPrefixLen, i.e., the length, in bits, of the prefix
>       associated with the Router Client entry which includes OrigAddr.
>       If omitted, the prefix length is equal to OrigAddr's address
>       length in bits.
> 
> it seems that only one prefix length is needed, why "list"?
> 
> 
>    ValidityTime
>       The length of time that the message sender is willing to offer a
>       route toward OrigAddr (and any other addresses included in the
>       given prefix length).  Omitted if no time limit is imposed.
> 
> the necessity of using ValidityTime has been raised here https://www.ietf.org/mail-archive/web/manet/current/msg18852.html <https://www.ietf.org/mail-archive/web/manet/current/msg18852.html>
>  
>    5.  Include MetricType and set the type accordingly
>    6.  Set OrigMetric := RouterClient.Cost for the Router Client entry
>        which includes OrigAddr
>  
> What if the metric type of the RouterClient.Cost is different from the metric type to be used in the route discovery? In fact, this is quite possible because they probably use different types of medium. 
> 
>  However, the regenerated RREQ can
>    be unicast to the next hop address of the LocalRoute toward TargAddr,
>    if known.
> 
> This can be a problem if the TargAddr is known to the routing set, but the link is actually broken. Because there is no related RERR messages to notify that, the following route discovery retries might fail also. A more careful design is required if unicast RREQ is to be used. 
> 
> 7.2.  Route Reply (RREP) Message
> 
> I don't quite understand the purpose of multicast RREP. There are, however, lots of disadvantages of using an optional AckReq field. 
> This has been discussed in another thread https://www.ietf.org/mail-archive/web/manet/current/msg18864.html <https://www.ietf.org/mail-archive/web/manet/current/msg18864.html>
>  
>    Before creating an RREP, the router SHOULD check if the corresponding
>    RREQ is redundant, i.e., a Route Reply has already been generated in
>    response to the RREQ, or if the limit for the rate of AODVv2 control
>    message generation has been reached.  If so, the RREP SHOULD NOT be
>    created.
> 
> What about if the RREQ carries a better route?

Then it’s not redundant? Or do you mean the SHOULD NOT be created part? The idea of these priorities is that already established routes should be protected before new routes are created, so even if the new RREQ is better, updating this route would strain the network even more, preventing routes from working, which in turn triggers more RREQs.. etc etc. So I’d say it’s a pity but we have no other option but to drop the better RREQ? 

(I’ve also substituted „the limit for the rate of AODVv2 control message generation“ for „ CONTROL_TRAFFIC_LIMIT“ in the Draft now since that seemed more concise ;) )

> 
> 7.3.2 RREP_Ack Reception
> 
> it seems that some action is missing after the step 2 Neighbour set update. At least, we need to update the Local Route Set?
> 
> 7.4.1 RERR generation
> 
> Is it necessary to generate RERR for RREP failure?
> 
>  
> If there is no route to PktSource, the RERR SHOULD be
>    multicast to LL-MANET-Routers.  If the RERR is sent in response to a
>    broken link, i.e., PktSource is not included, the RERR is, by
>    default, multicast to LL-MANET-Routers.
> 
> Although I know the purpose of using RERR message is to inform as many routers as possible about the route breakage information, it seems to be huge overhead, and more importantly, an appealing attack vector for hackers. 
> 
> 7.4.3 RERR regeneration
> 
>    2.  For each LocalRoute that needs to be reported as identified in
>        Section 7.4.1:
>        *  Insert LocalRoute.Address into the AddressList.
> 
> The AddressList can changed on every RERR regeneration. On the other hand, in section 6.9 (suppressing redundant RERR), the uniqueness of RERR is determined by AddressList and PktSource (optional). It seems to me that the suppressing won't be able to work correctly?
> 
> One more note:
> 
> Jitter is not mentioned in the draft, which is important because the multicast messages used.  I can see RFC5148 in the reference section, but didn't find where it is used. 
> 
> best
> 
> Jiazi
> 
> _______________________________________________
> manet mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/manet

Best regards,
Lotte