Hello John,
A few quick and somewhat incomplete comments before I have to go to
another meeting...
On 11/3/2015 6:39 PM, John Dowdell wrote:
Hi all
So here’s my review of -13a. I’m aware that there is a -13b but since I’m halfway through -13a it makes sense to carry on. Apologies if I’m raking up stuff that’s already been addressed.
Section 1. Overview
I’d like to see an extra paragraph between paras 2 and 3 that explains some more about AODVv2 sending messages that are converted into packets by the RFC5444 parser. It would make the existing paragraph 3 a bit clearer. Maybe move the last but one paragraph starting "AODVv2 control plane messages use the …” to between paras 2 and 3.
The paragraph starting "AODVv2 uses sequence numbers to identify …” is not compatible with the text at section 4.4 which suggests that loop freedom is based purely on sequence numbers “As a consequence, loop freedom is assured”. I think that last sentence should be deleted from section 4.4, or at least modified to state that sequence numbers form only part of the loop freedom calculation. While we’re here, the sentence in section 4.4 is also incompatible with section 5 bullet 5 “A function to analyse routes …”
Last paragraph: I think the word “plane" should be inserted between “control” and “messages” (to read “control plane messages” since that makes it consistent with the last but one paragraph (the one I asked to be moved above).
Section 2. Terminology
The definition of Neighbor says "Neighbors exchange routing information and attempt to verify bidirectionality of the link to a neighbor before installing a route via that neighbor into the Local Route Set.” It doesn’t say that if the bidirectionally test fails, we do not install a route via that neighbor. Possibly just remove the words “attempt to”.
The definition of TargSeqNum says "An AODVv2 Data Element used in a Route Reply message, containing the sequence number of the AODVv2 router which originated the Route Reply.” could be reworded to say "An AODVv2 Data Element used in a Route Reply message, containing a sequence number generated by the AODVv2 router which originated the Route Reply.”
Section 3: Applicability Statement
I believe the opening sentence should be expanded to briefly illustrate the differences between AODVv2 as a reactive protocol and the proactive protocols.
Paragraph 5: “Since the route discovery process typically results …”. I would suggest replacing “typically results in a route being established” with “requires a route to be established”
Para 8: I recall the question being asked before (probably by Thomas Clausen) about whether a single router interface can support multiple IP addresses. If we do, please say, and if not, please also say.
Section 4.3 Neighbor Table
Definition of Neighbor.ResetTime reads "When the State is Blacklisted, this time indicates the point at which the State reverts to Unknown. By default this value is …”. Should this read "When the /*value of Neighbor.State i*/s Blacklisted, this time indicates the point at which the /*value of Neighbor.State */reverts to Unknown. By default this value is …”
Section 4.4 Sequence Numbers
Para 3, last sentence, reads "The value zero (0) is reserved to indicate that the sequence number for an address is unknown.”. Should this read "The value zero (0) is reserved to indicate that the sequence number for /*a message*/ is unknown.”?
Para 4: Should “An AODVV2 router can only attach …” read “An AODVv2 router /*MUST*/ attach …”?
Para 5: "As a consequence, loop freedom is assured.”. This implies loop freedom is a function of sequence number only, but this could well be an old piece of text. My comment to section 1 applies here.
Section 4.5 Multicast Route Message Table (MRMT). This and section 4.6 could really use some introductory words on the uses of the MRMT, Local Route Set and the RIB (or other forwarding table belonging to the operating system), just to set the context for what is to be discussed. This table also needs to be mentioned in the introductory paragraph of section 6.
RREP: It would be worth explaining when RREPs would be multicast, as it is not obvious (or wasn’t to me anyway).
All through this section: would be worth tightening up on the name of the parameter when mentioned (e.g. RteMsg.OrigPrefixLen: The prefix length associated with /*RteMsg*/.OrigAddr)
RteMsg.OrigSeqNum: “The sequence number … if present …”. Why would the sequence number not be present?
RteMsg.RemoveTime: The time .. MUST be removed …”. Removed from where? Also there are two event times mentioned in this definition, which takes priority, or is it whichever is earlier or later?
Section 4.6 Local Route Set
The definition of Idle … this could use a cross-reference to section 6.4 where interaction with the forwarding plane of the host operating system is discussed.
The definition of Invalid … second sentence “… therefore it is not to be installed in the RIB. …”. Surely “therefore it is /*t*/*/o be withdrawn/* from the RIB”?
Para starting “Note that multiple entries …” (bottom of page 15), second sentence “… but may offer improvement …”. So why is that route still Unconfirmed? Is it really up to the implementor to decide which routes to confirm and which to not? This sounds like a potential interoperability issue to me.
Section 5 Metrics
Bullet 2 “ …AODVv2 cannot store routes that cost more than MAX_METRIC(MetricType)”. Anything is possible. Surely “AODVv2 /*MUST NOT*/ store routes …”?
Section 6.1 Initialisation
The sentence at top of page 18 before the bullets reads “During this wait period, the router can do the following”. I would suggest to clarify to “During this wait period, the router /*is permitted to*/ do the following”. I thought about making this a MUST but thought better of it.
Section 6.2 Next Hop Monitoring
Last sentence of bullet 1, “blacklisted" should read “Blacklisted”.
Section 6.3 Neighbour Table Update
Neighbour.IPAddress … where do we find this information, since it isn’t (according to the table in section 7.1) explicitly in the RREQ message? I guess it’s the source address of the RFC5444 packet. In any case, how can we assure that the neighbour IP address that RFC5444 parser uses is the same as the one that AODVv2 has allocated to the interface the message was to be sent over?
Last paragraph in this section on page 20; “However, the reset time allows ..” should perhaps read “However, */Neighbour.ResetTime/* allows …”
Section 6.4 Interaction with the Forwarding Plane
Second bullet in the second batch of bullets reads “A route discovery was not attempted”. Why would the router not attempt the discovery? The sentence continues “any buffered packets requiring that the route be discarded” which should perhaps read “/*If any packets are buffered, they MUST be discarded and the source of the packet should be notified that the destination is unreachable (using an ICMP Destination Unreachable message”.*/
/*
*/
Section 6.6 Route Discovery …
Second para last sentence should read “The AODVv2 router /*concerned*/ is referred to as RREQ_Gen”.
Fourth paragraph reads "Determining which packets to discard first when the buffer is full is a matter of policy at each AODVv2 router. Routers without sufficient memory available for buffering SHOULD have buffering disabled. This will affect the latency for launching TCP applications to new destinations.” So what is the behaviour here when buffer space is constrained? Should the router discard the packets while seeking to discover the route, without sending ICMP Destination Unreachable? Whatever the behaviour is, please advise some text to put here because it’s not clear.
Section 6.7 Processing Received Route Information
“All AODVv2 route messages contain a route”. Surely the RREQ message generated by RREQ_Gen does not include any route information yet?
Section 6.8 Suppressing Redundant …
First para “… and generates unnecessary signalling traffic and interference”. Interference? How can we be sure, this will depend on the exact radio configuration used? Suggest to delete the words “and interference”.
Page 30 “To determine if a received message …”
Point 1 first bullet “If there is none, the message …” should read “If there is /no entry/, the message …”
Section 6.9.2 Reporting Invalid Routes
There are three conditions when an RERR SHOULD be sent. Are there no conditions when an RERR MUST be sent?
Section 7.1.2 RREQ Reception
Point 1, bullets 1 and 2; where is Remove Time defined? I couldn’t find a definition.
Point 7, suggest the words “is redundant” are replaced by “was previously received”. The word ‘redundant’ is so abused in the UK (mostly by HR departments in relation to people they can no longer afford to employ) that the meaning is now unclear.
Section 7.2.1 RREP Generation
Point 2 on page 41: what is the basis on which msg_hop_count is included in the RREP message?
Section 7.2.2 RREP Reception
Step 1; should the blacklisting also be revoked?
Step 7 7; what should be the action if the route to TargAddr does not match an outstanding route request? Should the RREP be discarded?
Step 9; if not, proceed to step 10?
7.3.2 RREP_Ack Reception
It’s obvious to me, but for the sake of the security AD we probably need to point out that if the RREP_Ack was not expected, the router must not respond.
7.4.1 RERR Generation
I was just wondering if we should make an exception for ICMP messages particularly those signalling errors, and not send an RERR in those cases. We could potentially have a compound situation where the routers are trying to deliver ICMP failure messages when the user data IP packet has already triggered an RERR. Or am I chasing my tail here? This occurred to me because a router is required to discard the IP packet that triggered the RERR, and got to wondering whether we should send an ICMP message. I’m open to discussion here.
Third bullet starting “When a link breaks …”. There is a point about MTU being exceeded … we’re sending AODVv2 messages, not RFC5444 packets. How will AODVv2 know the MTU is being exceeded? In fact how do we know the MTU at all, since AODVv2 has not asked to be notified about it by the forwarding plane in section 6.4. Anyhow is that not the RFC5444 parsers problem (or maybe even the SMF parser if that is being used as well)? Perhaps we should convert that text into an advisory to the implementor.
Sending an RERR in response to an undeliverable IP packet … SHOULD be sent unicast to the next hop or /alternatively/ MUST be multicast … which is it please? I can sort of see what is being said but the logic is not clear.
Section 7.4.2 RERR Reception
Typo … Step 1 bullet 1 .. ignore this RREP … should read ignore this /*RERR*/.
I believe we need to use stronger language in the sentence “If any of the above are false …”, I propose "If any of the above are false, the LocalRoute does not need to be made Invalid and the unreachable address does not need to be advertised in a regenerated RERR.” be changed to "If any of the above are false, the LocalRoute /*MUST NOT*/ be made Invalid and the unreachable address /*MUST NOT*/ be advertised in a regenerated RERR.”
There are conflicting directives all over the second half of this section. If we have routes matching those in the RERR, SHOULD or MUST an RERR be regenerated? There is no defining logic here. The bullets in step 2 sometimes say we SHOULD, the directive at step 4 is to do it anyway. There is no clear decision tree. I propose that we MUST regenerate an RERR in all cases if the RERR passes the validity test in the first half of this section.
Section 7.4.3 RERR Regeneration
Step 5 … again we need to advise the implementor that an RERR with lots of addresses could break an MTU and so there needs to be a check somewhere when the packet is to be sent to the interface. AODVv2 is not in charge of forming the packet.
Again … SHOULD be unicast or MUST be sent multicast? Which is it please? Suggest something like "if the message cannot be unicast it MUST be sent multicast”.
8. RFC5444 Representation.
Hold up. Control plane message SHOULD use RFC5444? I thought that was the whole point of AODVv2. Surely MUST?
I haven’t checked the mapping to RFC5444 TLVs because my head hurts, but it looks beautiful.
Section 11.2 Protocol Constants / Section 11.6 MetricType Allocation
MAX_METRIC[MetricType] … just a note for the future that DLEP had a discussion on the field length of a metric. Might as well recommend that AODVv2 metric fields are compatible with DLEP.
Section 13 Security Considerations
There is a theme of reliance on ICV to show the message has integrity. That’s ok by itself, but it should also be pointed out that the messages must be robustly processed according to the steps in section 7 (which must itself be robustly specified). If there are holes in our specifications, or if there are implementation holes, or chances for common attack vectors (e.g. sending a huge RERR causing a buffer overflow), then that’s a problem, particularly for the constrained memory devices we have discussed in the past.
Appendix C.
My page 74 is blank for some reason.
Similarly there are some text flow problems in this section, perhaps where code blocks are included, e.g. C.2.1.3, C.2.2.
I’m, not going to check the code, not really my area!
Overall
Quite a fabulous document. Looks a whole lot better than it did a year or so ago. Well done all.
Cheers
John
