Hi all, sorry for the late reply,
I'm just going to plop some comments to John's review inline and then go
through Vicky's diff :)
Cheers,
Lotte
Am 04.11.2015 um 03:39 schrieb John Dowdell <john.dowdell486@xxxxxxxxx>:
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 is
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 to 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
