Hi Lotte, everyone,
just one comment below regarding the MTU discussion.
On Sun, Nov 8, 2015 at 8:56 PM, Lotte Steenbrink <
lotte.steenbrink@xxxxxxxxxxxx> wrote:
Hi all, sorry for the late reply,Data Elements, if RFC5444 is going to move bits around and organise the
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 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.”?
Just for the record, I +1 Charlie's comment to this paragraph
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?
Isn't that up to the implementation to ensure? Should we add a note that
reminds people to do this?
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?
Not anymore, but it did back when we were installing routes back to
RREQ-Gen upon receipt of a RREQ. So I guess this is deprecated text?
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.
No, but it is in charge of forming the message. I hope I'm not
misinterpreting you but... At packet-level, the 5444 builder/parser can't
say "oh, this message is way too big to be sent out in one chunk, I'll
split that up" because it doesn't know how to split the message up in a way
that it can still be understood by the receiving end (i.e. the AODVv2
instance on the neighbouring node).
So (I guess) it has to just send out its packets and pray that the
underlying layers will work their fragmentation magic and that none of the
fragments get lost, because the retransmissions would cause overhead...
Since AODVv2 was also designed for environments in which it was rather
likely that fragments could get lost, I'd just always assumed that the goal
behind that step was to convey as much information as we can with as little
packets as possible. So if an AODVv2 implementation happens to know that
their RREP is going to be too big, it makes sense to split it up into
multiple RREPs, and if only half of them reach their destination that's
better than nothing. I understood Step 5 as a hint that they should to this
if their system permits them to and they're fine with the possible layer
violation.
Whew, that was a lot more text on my part than I'd anticipated and I'm not
quite sure what to make of it...
Wellllll also, how can we know exactly where the limit is, based on our
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
