Subject: [Last-Call] Tsvart last call review of draft-ietf-6lo-minimal-fragment-07

From: Joerg Ott via Datatracker <[email-masked]>
Date: Thu, 30 Jan 2020 01:42:16 -0800

Reviewer: Joerg Ott
Review result: Ready with Nits

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
[email-masked] if you reply to or forward this review.

This document defines a mechanism that allows stateful forwarding of 6LoWPAN
fragments at intermediate nodes without prior reassembly. One purpose is to reduce
the need for reassembly buffers in resource-constrained nodes. To this end, a 
forwarding node will create forwarding state upon receipt of the first fragment, 
using the pair (previous hop's MAC address, datagram_tag) as a forwarding label
against which fragments are matched. The routing decision is taken based upon
the first fragment and then applied to all subsequent ones using the above "label".

Generally, the document is in good state and almost ready to publish. A few nits,
one on protocol state management below:

Nits:

The text uses both "6lo fragments" and "6LoWPAN fragments".

Section 5, end of 1st para:
"Since the datagram_tag is uniquely associated to the source Link-Layer address
of the fragment, the forwarding node MUST assign a new datagram_tag from its
own namespace for the next hop and rewrite the fragment header of each
fragment with that datagram_tag."

This sentence is correct but it comes after the description of handling subsequent
fragments, rather than the first one, and subsequent ones should, of course, not
receive a new datagram_tag. Maybe move the sentence further up or make explicit
reference to the first fragment.

Sect. 5, bullet list, "* a datagram_size"
Just as a remark, this does not seem to be used. It *could* be used to check if a
fragment beyond the end of the packet arrives, but has otherwise no (documented)
meaning. The draft should spell out its purpose.

Sect. 5, bullet list, "a timer that allows discarding the stale FF state after some timeout"
Surely needed, but no advice is given. There is generally no explicit statement when to
discard the state. What should an implementation do to interoperate, given that
upstream multiplexing of packet fragments from multiple sources can yield diverse
intervals between consecutive fragments? Would this be the same timer previously
used for reassembly? If so, maybe just state this.

Sect. 7, 2nd bullet: "attck" -> "attack"

---

From: "Pascal Thubert (pthubert)" <[email-masked]>
Date: Thu, 30 Jan 2020 11:01:54 +0000

Many thanks Joerg!

Please see below:

> Generally, the document is in good state and almost ready to publish. A few
> nits, one on protocol state management below:
> 
> Nits:
> 

Changed to 6LoWPAN fragments everywhere

> "Since the datagram_tag is uniquely associated to the source Link-Layer
> address of the fragment, the forwarding node MUST assign a new
> datagram_tag from its own namespace for the next hop and rewrite the
> fragment header of each fragment with that datagram_tag."
> 
> This sentence is correct but it comes after the description of handling
> subsequent fragments, rather than the first one, and subsequent ones should,
> of course, not receive a new datagram_tag. Maybe move the sentence further
> up or make explicit reference to the first fragment.

Great point, moved up.

> Just as a remark, this does not seem to be used. It *could* be used to check if
> a fragment beyond the end of the packet arrives, but has otherwise no
> (documented) meaning. The draft should spell out its purpose.

Since we did not use it the simplest is to remove it 😊

> Surely needed, but no advice is given. There is generally no explicit statement
> when to discard the state. What should an implementation do to interoperate,
> given that upstream multiplexing of packet fragments from multiple sources
> can yield diverse intervals between consecutive fragments? Would this be the
> same timer previously used for reassembly? If so, maybe just state this.

Great point again! Does the below work, added to the bullet?
"
The duration of the timer should be longer than that which covers the reassembly at the receiving end point. 
"

> Sect. 7, 2nd bullet: "attck" -> "attack"

Fixed!

Many thanks again, Joerg. I posted 09 with the above. Please let me know if we are OK?

All the best

Pascal

---

From: Joerg Ott <[email-masked]>
Date: Thu, 30 Jan 2020 12:20:39 +0100

Hi Pascal,

wow, this is a super-fast fix.  Looks all good to me.

Best,
Jörg