Subject: Rtgdir telechat review of draft-ietf-babel-rfc6126bis-11

From: Yingzhen Qu via Datatracker <[email-masked]>
Date: Thu, 01 Aug 2019 16:32:43 -0700

Reviewer: Yingzhen Qu
Review result: Has Issues

I have been selected as the Routing Directorate reviewer for this draft. The
Routing Directorate seeks to review all routing or routing-related drafts as
they pass through IETF last call and IESG review, and sometimes on special
request. The purpose of the review is to provide assistance to the Routing ADs.
For more information about the Routing Directorate, please see
http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Although these comments are primarily for the use of the Routing ADs, it would
be helpful if you could consider them along with any other IETF Last Call
comments that you receive, and strive to resolve them through discussion or by
updating the draft.

Document: draft-ietf-babel-rfc6126bis
Reviewer: Yingzhen Qu
Review Date: 1 August 2019
Intended Status: Standards Track

Summary:
This document describes the Babel routing protocol, and obsoletes RFC 6126 and
7557. Note: both RFC 6126 and 7557 are experimental, the current bis version is
standards track. I don’t know the history behind, but just want to point it out.

Overall comments:
The document is mostly well written.
I’m not a native speaker, so I’d leave all language nits to RFC editors.

Issues: (the line number is used is from idnits)

There are security concerns raised by Secdir review:
https://datatracker.ietf.org/doc/review-ietf-babel-rfc6126bis-10-secdir-lc-kaufman-2019-06-28/

The description about packet trailer is in RFC 7557 section 2.5, but not
included in the bis version. Considering this document will obsolete RFC 7557,
I suppose it should be self-complete, so readers don’t need to go back to RFC
7557 for more information.

There are multiple places in the document about TLV/Sub-TLV being
“self-terminating”, but I didn’t find what it means. Maybe I’m missing
something here?

Section 1.1
“unmanaged and wireless environment”, what does “unmanaged” mean here?

Section 3.2.3
The interface Hello seqno is changed to outing multicast hello seqno, however
there is no description about unicast hello at all. While I was reading it, I
kept thinking what if it’s unicast hello? Is there a unicast hello timer
needed? From later sections, I realized there are unicast hellos. So I’d
suggest add some descriptions to avoid the confusion.

Section 3.2.4
There is “the expected incoming Unicast Hello sequence number” and “the
outgoing Unicast Hello sequence number”, but it was not mentioned in section
3.2.3 (related with previous comment).

Section 3.8.1.1
After a node receives a route request, if the given prefix doesn’t exist in its
route table, it MUST send a retraction for that prefix. So my question is
whether a node is allowed to send multiple explicit requests for a given
prefix? If so, what happens if both retractions and updates are received?

Section 4.4
1616    4.4.  Sub-TLV Format

1618       Every TLV carries an explicit length in its header; however, most
1619       TLVs are self-terminating, in the sense that it is possible to
1620       determine the length of the body without reference to the explicit
1621       Length field.  If a TLV has a self-terminating format, then it MAY
1622       allow a sequence of sub-TLVs to follow the body.
This section is about Sub-TLV, however the description language in this
paragraph is mainly about TLV.

Section 4.5
it says that “an implementation may choose to use a dedicated stateless parser
to parse the packet trailer”. Will the packet trailer be able to use the state
parser if there are state there useful or just for implementation purpose?
Although there is no packet trailer defined at this moment.

Section 4.5
1674       parsing a TLV MUST update the parser state even if the TLV is
1675       otherwise ignored due to an unknown mandatory sub-TLV.

Section 4.6.5
1805                 send a new scheduled Hello TLV with the same setting of the
1806                 Unicast flag.  If this is 0, then this Hello represents an
I’d suggest changing the text to “the same setting of flags” instead of “the
same setting of the Unicast flag”, considering the flags will be extended later.

Nits:

1049       metric) from a neighbour neigh with a link cost value equal to cost,
1050       it checks whether it already has a route table entry indexed by
[neighbour neigh] => [neighbour]

Thanks,
Yingzhen

---

From: Juliusz Chroboczek <[email-masked]>
Date: Fri, 02 Aug 2019 10:20:46 +0200

Dear Mr Qu,

Thank you for your review.

> 7557 for more information.

The packet trailer is described in Section 4.2.

> “self-terminating”, but I didn’t find what it means. Maybe I’m missing

This is defined in Section 4.4.  I have checked that all uses of
self-terminating occur after its definition.

> “unmanaged and wireless environment”, what does “unmanaged” mean here?

Not being actively managed by a human administrator.  This is a non-normative
section.  Please let me know if I'm using this term badly.

> there is no description about unicast hello at all.

Unicast Hellos are per-neighbour, so they appear in Section 3.2.4 (Section
3.2.3 describes per-interface data structure while 3.2.4 describes
per-neighbour structures).  Both kinds of Hellos are described in more
detail in Section 3.4.1.

> While I was reading it, I kept thinking what if it’s unicast hello? Is
> there a unicast hello timer needed?

There is one, see Section 3.2.4, penultimate paragraph.

> From later sections, I realized there are unicast hellos. So I’d suggest
> add some descriptions to avoid the confusion.

Sections 3.2.4 and 3.4.1.

> 3.2.3 (related with previous comment).

That's right.  Section 3.2.3 describes per-interface data.  Section 3.2.4
describes per-neighbour data.  Multicast Hellos are per-interface.
Unicast Hellos are per-neighbour.

> prefix?

There's nothing forbidding it.  It may send multiple requests to different
neighbours (over unicast), or multiple requests on different interfaces
(over multicast).  There's also nothing forbidding a node from sending
multiple requests to the same destination, e.g. to compensate for packet
loss.

(Our implementation experience shows that, at least over 802.11, such
aggressive behaviour is not useful, it increases noise without having
a measurable effect on convergence time, hence the SHOULD in Section
3.8.2.3 which only requires a single request.  However, further research
might yield new insights, which is why this document does not explicitly
disallow such behaviour.)

> If so, what happens if both retractions and updates are received?

The procedure described in Section 3.5.4 is run for each received update
or retraction, which results in the construction of the data structures
used as input for the precedure described in Section 3.6.

> paragraph is mainly about TLV.

Good catch, thanks.  I'll fix that.

> parser if there are state there useful or just for implementation purpose?

I do not understand this comment.  Please clarify.

> Although there is no packet trailer defined at this moment.

Section 4.2.

> 1675       otherwise ignored due to an unknown mandatory sub-TLV.

I believe you may have forgotten to write your comment.

> same setting of the Unicast flag”, considering the flags will be extended later.

I disagree.  This paragraph is about the multicast/unicast dichotomy, not
about future flags of an informative nature.

> [neighbour neigh] => [neighbour]

I disagree.  We're defining the variable neigh, which we use in the next
sequence.

Regards,

-- Juliusz Chroboczek

---

From: Juliusz Chroboczek <[email-masked]>
Date: Fri, 02 Aug 2019 10:30:09 +0200

> Good catch, thanks.

Actually, this section starts by describing how sub-TLVs fit within TLVs,
so it's correct as it stands.

-- Juliusz Chroboczek

---

From: Yingzhen Qu <[email-masked]>
Date: Tue, 6 Aug 2019 23:44:53 -0700

Hi Juliusz,

Thanks for your reply, please see my response below starting with [YQ].

Thanks,
Yingzhen

On Fri, Aug 2, 2019 at 1:20 AM Juliusz Chroboczek <[email-masked]> wrote:

> [YQ]: In RFC 7557 section 2.5, there is a clear description about how the
packet trailer length is calculated etc. Personally I feel it's more
straightforward, but if you think section 4.2 is enough, I'm not going to
insist.

> [YQ]: in section 4.4, it says self-terminating can be used to determine
the length of the TLV body without using the length field. This is clear,
however what does it look like? how to detect it? maybe I'm missing
something here.

> [YQ]: my understanding is unmanaged can mean anything, like no management
at all. I don't think it's a good term, especially put together with
wireless.  maybe try to add a bit more explanation?

> per-neighbour structures).  Both kinds of Hellos are described in more
[YQ]: there is nothing wrong with current text. I was thinking of adding
unicast just for easy readability. same for the following three related
questions.

> [YQ] thanks for the explanation. I'm just wondering whether we should
document it somewhere?

> used as input for the precedure described in Section 3.6.

[YQ]: got it.

> Good catch, thanks.  I'll fix that.

[YQ]: same as comments regarding unicast hello, current text is not wrong.

> [YQ]: My understanding is Babel uses a stateful parser, and TLVs in the
packet trailer are not allowed to modify the state. So the text here is
suggesting to implement a stateless parser for packet trailer. so the
question is: will packet trailer to able to use/read the state? is the
stateless parser suggested just for implementation simplicity?

> > Although there is no packet trailer defined at this moment.
[YQ]: I meant besides the pad1 and padN, there is no real functional packet
trailer defined at this moment, but it's open for extensions.

> [YQ]: is it correct that the parser state is updated by a TLV even if the
TLV is ignored due to unknown sub-tlv? what about a TLV ignored due to
other reasons?

> [YQ]: then I misunderstood it. I thought it was meant to copy the entire
flag field.

> [YQ]: it seems that this is the only place using "neigh", so I thought it
was a typo. I would suggest some clarification.

---

From: Juliusz Chroboczek <[email-masked]>
Date: Mon, 12 Aug 2019 07:20:01 +0200

Dear Ms. Qu,

Sorry for the delay, and thank you for your reminder.

>> The description about packet trailer is in RFC 7557 section 2.5, but
>> not included in the bis version.

> straightforward, but if you think section 4.2 is enough, I'm not going to

I see.  Yes, I'll clarify that.

> [YQ]: in section 4.4, it says self-terminating can be used to determine the
> length of the TLV body without using the length field. This is clear, however
> what does it look like? how to detect it? maybe I'm missing something here.

For example, the router-id TLV is always 8 octets long.  It is
self-terminating.

The Pad1 TLV can have arbitrary length, and it is impossible to determine
its length without the explicit Length field.  It is not self-terminating.

> [YQ]: my understanding is unmanaged can mean anything, like no management at
> all. I don't think it's a good term, especially put together with wireless. 
> maybe try to add a bit more explanation?

This is a conceptual overview, unmanaged is used in a non-technical sense.

>> Section 3.2.3 The interface Hello seqno is changed to outing multicast
>> hello seqno, however there is no description about unicast hello at
>> all.

> unicast just for easy readability. same for the following three related

The approach taken here is to define the data structures before explaining
the protocol.  The advantage is to have a clear reference for the
implementer; but of course it leads to a number of things that are not
entirely clear at this point.  It's a difficult tradeoff, I'm going to
leave it as is.

>> After a node receives a route request, if the given prefix doesn’t
>> exist in its route table, it MUST send a retraction for that prefix. So
>> my question is whether a node is allowed to send multiple explicit
>> requests for a given prefix?

[...]

> [YQ] thanks for the explanation. I'm just wondering whether we should

No, I don't think so.  This case is not specific -- for example, sending
multiple copies of updates is also not a good idea.
 
> [YQ]: My understanding is Babel uses a stateful parser, and TLVs in the packet
> trailer are not allowed to modify the state. So the text here is suggesting to
> implement a stateless parser for packet trailer. so the question is: will
> packet trailer to able to use/read the state? is the stateless parser suggested
> just for implementation simplicity?

None of the TLVs allowed in the packet trailer use or modify the state.
See also the last paragraph of the appendix "Considerations for protocol
extensions".

> [YQ]: is it correct that the parser state is updated by a TLV even if the TLV
> is ignored due to unknown sub-tlv? what about a TLV ignored due to other
> reasons?

You're right, I'll add a comment.

> [YQ]: then I misunderstood it. I thought it was meant to copy the entire flag
> field.

> [YQ]: it seems that this is the only place using "neigh", so I thought it was a
> typo. I would suggest some clarification.

This has been removed.

-- Juliusz