Early Review of draft-ietf-manet-dlep-credit-flow-control-09
review-ietf-manet-dlep-credit-flow-control-09-tsvart-early-black-2021-11-19-00

TSVART Review of:

        DLEP Credit-Based Flow Control Messages and Data Items
                draft-ietf-manet-dlep-credit-flow-control-09
        DLEP DiffServ Aware Credit Window Extension
                draft-ietf-manet-dlep-da-credit-extension-12
        DLEP Traffic Classification Data Item
                draft-ietf-manet-dlep-traffic-classification-06

Reviewer: David L. Black <david.black@dell.com>
Date: November 19, 2021

This is a combined review of all three drafts.

These three drafts specify a flow control mechanism between a "router" and a
"modem" in the sending direction (router to modem) to prevent overrun of
buffers in the "modem".  The mechanism is designed to provide flow control for
multiple queues in the "modem" that are independently protected, with the
"modem" in control of how much data the router is allowed to send - to a first
approximation, there is a separate instance of flow control, called a Credit
Window, for each queue.  Both Diffserv (DSCP) and Ethernet Priority (PCP)
values can be used to classify traffic to determine which flow control instance
(Credit Window) applies to each packet being sent to the "modem" by the
"router".

Each of the drafts is reasonably well written, but there are some difficulties
in understanding the combination of the three drafts, which have to be used
together in order to implement this flow control mechanism. That's one of a
number of issues that deserve attention.

****** Major Issues:

-- [1] -- Number of Documents

I understand and see the merit in specifying the flow control mechanism
(-credit-flow-control draft) independently of traffic classification
(-traffic-classification draft).  In contrast, I do not see a strong rationale
for the separate DA credit extension draft (-da-credit-extension), which has
very little content (about a page of actual protocol specification plus one
addition to an IANA registry).

I strongly suggest folding the DA credit extension draft into the traffic
classification draft, as the resulting two-draft structure will be easier to
explain, and more importantly, should be easier for implementers to understand.

-- [2] -- Flow Match Criteria

The Traffic Classification draft does not explain how traffic classification is
actually performed, i.e., how the applicable FID is determined for a packet,
particularly when more than one FID is possible. For example, suppose a packet
carries a DSCP that matches FID 5 and a PCP that matches FID 7 - does that
packet consume FID 5 credits or FID 7 credits?  If the router and modem make
different decisions, the result may be undesirable.  This example is within the
same TID - the situation appears to be worse across TIDs for the same
destination, because the same match criteria could appear in multiple TIDs,
e.g., same DSCP could match different FIDs under different TIDs.  Clarity is
needed here so that the router and modem agree on which FID's credits are
consumed by each packet, and on a related note, the DA credit extension draft
does not prohibit use of Ethernet traffic classification - perhaps it should.

-- [3] -- Router MAY ignore flow control

The Management Considerations section in both the traffic classification and DA
credit extension drafts contains this text:

   When modem-provided credit window information exceeds the capabilities of a
   router, the router MAY use a subset of the provided credit windows.

In other words, the router MAY ignore any credit windows that exceed the
routers capabilities ... which in practice could amount to "MAY ignore any
credit windows that the router implementer views as inconvenient" ... with the
router proceeding to overrun the corresponding modem queues.  That doesn't seem
right, so if this is intended, some serious rationale/explanation ought to be
provided.

-- [4] -- Association Not Well-Specified

The overall association structure is that flow control instances are identified
by FIDs which are associated with TIDs which are associated with destinations. 
That structure is described towards the end of Section 2 of the flow control
draft, but the details of how it is done are mostly missing - the flow control
draft ought to state that:

        i) FIDs are associated with TIDs via traffic classification information
        specified in the traffic classification draft. ii) TIDs (and their
        associated FIDs) are associated with destinations via use of the Credit
        Window Association data item in DLEP Destination Up and Destination
        Update messages.

The first item (i) can be mostly inferred from the last few paragraphs in
section 2 of the flow control draft (and needs to be more explicit), but the
second item (ii) is missing because the flow control draft does not specify how
to determine the "destination" with which a TID is associated by a Credit
Window Association data item (in a DLEP Destination Up or Destination Update)
message.  I would expect that the destination is a relatively obvious field in
those messages, but the draft needs to specify that field.  This should be easy
to fix, although it made the drafts much more difficult to understand.

-- [5] -- Initialization vs. in-flight traffic

The Credit Window Initialization data item appears to be intended to establish
common state for a Credit Window (e.g., size, number of credits) across the
router and modem ... but ... that data item appears to be allowed to be sent
while there's traffic in flight.  The result would be that the modem would
count in-flight traffic against the initialized Credit Window, but the router
would not.   The resulting inconsistency deserves discussion - it may be
acceptable if the amount of traffic in flight is miniscule by comparison to
both the Credit Window size and initial credit balance.

-- [6] -- Security Considerations

Although this is a Transport review, I found a security issue that would be
better dealt with now before the security directorate points it out ;-) - the
security considerations sections in each of the 3 drafts claims that adding
credit window control and flow functionality to DLEP does not introduce any new
security considerations (vulnerabilities). That's a nice try, but it's
incorrect.

These drafts specify a new resource (credits in a credit window) that is
subject to resource exhaustion attacks that could cause denial-of-service.  For
example, suppose an attacker injects a Credit Window Initialization data item
that contains almost no credits and/or specifies a ridiculously tiny Window
(Max) Size.  I expect that the protocol contains mechanisms to counter this and
related attacks on credit resources (e.g., if something looks wrong, the modem
reinitializes the Credit Window), but the current text incorrectly asserts the
non-existence of such attacks.  These sorts of attacks definitely exist - I am
aware of a (subsequently fixed) resource exhaustion problem in another
credit-based flow control mechanism caused by an unanticipated environmental
"attack" on signal integrity of credit exchange messages, resulting in message
discard and credit loss.

***** Minor Issues:

[A] Packets consume credits: Section 2 of the flow control draft needs to say
somewhere early in the section that each packet that a router sends consumes a
credit for each octet in the packet.  This is to be found at the end of the
second paragraph in Section 2.1, but ought to be stated much earlier, e.g., at
the end of second paragraph in Section 2.

[B] Bidirectional messages: Both the Credit Control Message and its response
can be sent in both directions (i.e., by both modems and routers).  Has the
possibility of using different message types in each direction been considered?
 That might help out people who have to read packet traces/dumps.  NOTE: "Yes,
that was carefully considered and the WG decided not to do that." is a fine
response.

[C] Data item usage:  It's not clear which data items are required, allowed or
prohibited in which DLEP messages.  It appears that any data item MAY be used
in any DLEP message, which is surely not what was intended.  Both the flow
control and traffic classification drafts ought to spell out these details,
including what happens when  the rules are violated, e.g., data item shows up
in a message where it's not supposed to be used - is the data item ignored or
does it cause an error?

[D] Uninitialized window: The second sentence of Section 2.3.1 in the flow
control draft begins with:

   This Data Item SHOULD be included in any Session Initialization Response
   Message that also ...

What happens if that SHOULD is not adhered to?  Please explain.

[E] Credit Window Size: In Section 2.3.1 of the flow control draft (Credit
Window Initialization), what happens if the Credit Value exceeds the Credit
Window Size?

I also strongly suggest renaming Credit Window Size to Credit Window Max Size
or something similar to avoid possible confusion of this concept with current
credit balance (Credit Value)

[F] TID association with destination:

Section 2.3.2 of the flow control draft (Credit Window Association) says that a
the traffic classification information associated with a TID "MUST be
associated with the destination" - what does that mean?

Specifically:
- Does that traffic classification information replace any existing     traffic
classification information, e.g., if the info associated with that TID has been
updated since that TID was most recently associated with that destination? - Is
there any limit on the number of TIDs or amount of traffic classification
information that can be associated with a destination?  If so, what happens if
that limit is exceeded? Also note that "the destination" is not currently
defined, see [4] above.

[G] Bad Credit Window math: The next to last paragraph in Section 2.3.3 (Credit
Window Grant) of the flow control draft contains this text about adding credit
to a window:

   If the increase results in a window overflow, i.e., the size of the credit
   window after the increase is smaller than the original credit window size,
   the Credit Window must be set to its maximum (0xFFFFFFFFFFFFFFFF).

That's doubly wrong:
- consider a window max size of 10, a current window size of 3 and an addition
of 15 credits.  Overflow occurs, but the window size increases, as the new
window size is 8. - The maximum size of the credit window may be considerably
smaller than 0xFFFFFFFFFFFFFFFF.

For the first item, just use the concept of overflow in modular arithmetic. 
For the second item, remove that all-F's constant and refer back to the Credit
Window Initialization material for max size.

This text is also an example of why [E] strongly suggests renaming Credit
Window Size to Credit Window Max Size or somthing similar.

[H] Amount of imprecision: Section 2.3.4 of the flow control draft discusses
modem comparison of internal values with values received from the router, and
contains this text:

   If the values significantly differ, i.e., greater than can be  accounted for
   based on observed data frames,

That needs some guidance on what a significant difference is and how much of a
"greater than" difference ought to trigger the consequences, accompanied by a
warning against precise (== vs. !=) comparisons (e.g., courtesy of [5] above).

[I] Update means what?: The last paragraph of Section 2.1 of the traffic
classification draft (Traffic Classification Data Item) contains:

   the router MUST update the information using the values carried in the Data
   Item.

What does "update" mean, e.g., "replace", "merge"?

[J] Error behavior: Section 2.1.1 of the traffic classification draft leaves
error behavior as a "go fish" exercise for the reader:

   Any errors or inconsistencies encountered in parsing
   Sub-Data Items are handled in the same fashion as any other Data
   Item parsing error encountered in DLEP.

That doesn't tell an implementer what to do - this needs to cite a reference
that specifies the applicable error behavior.

[K] RFC 2474 CU field: Section 2.2 of the traffic classification draft defines
the DS Field as an octet and reproduces the definition from RFC 2474 that
includes the DSCP and CU fields.  Unfortunately, this does not reflect updates
to RFC 2474 - those two bits are now the ECN Field, which may be non-zero.

The fix for this is simple - use the 6 bit DSCP field from RFC 2474 and replace
the CU field with two zero bits to produce an octet.

[L] 802.1Q DEI field: This has a similar problem to the CU field ... and a
similar solution - replace the DEI field with a zero bit.

****** Nits:

FID paragraph in section 2.3.5 of the flow control draft:
        The FID also uniquely identifies a credit window
s/identifies/indicates/ for consistency.

End of Section 2 of the traffic classification draft:
        TID and FID values is a modem-local scope.
s/is a/have/