Re: [quicwg/base-drafts] Update DPLPMTUD and PMTUD (#3693)

[Martin Thomson <[e-mail]>]

@martinthomson commented on this pull request.

Thanks for making a concrete proposal.  It really helps to have it written out.

I have a bunch of suggestions, but I don't see any real barrier to including this text on the basis of this PR.

Don't worry about the CI build failures.  This will need some cleanup, but I can do that fairly easily.  Let's get the text right first.

This is a major change, so I'll be opening an issue to track this.  In a sense, it's editorial because we are only taking text that we previously normatively referenced, but it's big enough to track properly.  Besides, I think that I suggested some substantive changes and I would be sad to see those not included.

>  In the absence of these mechanisms, QUIC endpoints SHOULD NOT send IP packets
-larger than 1280 bytes. Assuming the minimum IP header size, this results in a
-QUIC maximum packet size of 1232 bytes for IPv6 and 1252 bytes for IPv4. A QUIC
-implementation MAY be more conservative in computing the QUIC maximum packet
-size to allow for unknown tunnel overheads or IP header options/extensions.
+larger than the minimum QUIC packet size. 
+
+All QUIC
+packets (except for PMTUD/DPLPMTUD probe packets) SHOULD be sized to fit within the

```suggestion
packets other than PMTUD/DPLPMTUD probe packets SHOULD be sized to fit within the
```

>  
 
-## ICMP Packet Too Big Messages {#icmp-pmtud}
+### Handling of ICMP PTB Messages by PMTUD {#icmp-pmtud}

I would drop PTB from this.  It's already alphabet soup, but this is also accurate:

```suggestion
### Handling of ICMP Messages by PMTUD {#icmp-pmtud}
```

If you are looking for a proper parallel construction, then maybe consider just making this the PMTUD section:

```suggestion
## PMTU Discovery {#icmp-pmtud}
```

>  (e.g., IPv6 Packet Too Big messages) that indicate when a packet is dropped
 because it is larger than the local router MTU. DPLPMTUD can also optionally use
 these messages.  This use of ICMP messages is potentially vulnerable to off-path
 attacks that successfully guess the addresses used on the path and reduce the
 PMTU to a bandwidth-inefficient value.
 
 An endpoint MUST ignore an ICMP message that claims the PMTU has decreased below
-1280 bytes.
+the minimum QUIC packet size bytes.

```suggestion
the minimum QUIC packet size.
```

>  as specified in {{!RFC8201}} and Section 5.2 of {{!RFC8085}}. This validation
 SHOULD use the quoted packet supplied in the payload of an ICMP message to
-associate the message with a corresponding transport connection {{!DPLPMTUD}}.
-
+associate the message with a corresponding transport connection (e.g., {{!DPLPMTUD}}).

I appreciate the correction of the citation here, but I am not getting the connection still.  Do you have a specific citation in DPLPMTUD that would support this?  Maybe:

```suggestion
associate the message with a corresponding transport connection; see Section X.X
of {{!DPLPMTUD}}.
```

or 
```suggestion
associate the message with a corresponding transport connection; for instance,
Section X.X of {{!DPLPMTUD}} describes Y.
```

> +packet size. The MIN_PLPMTU is the same as the BASE_PMTU.
+
+QUIC endpoints implementing DPLPMTUD maintain a maximum packet size 
+(DPLPMTUD MPS) for each combination of local and remote IP
+addresses. 
+If a QUIC endpoint determines that the PLPMTU between any pair of local
+and remote IP addresses has fallen below the size needed to support
+the minimum QUIC packet size (BASE_PLPMTU), it MUST immediately cease
+sending QUIC packets, except for DPLPMTUD probe packets, on the affected
+path. An endpoint MAY terminate the connection if an alternative
+path cannot be found.
+
+###  DPLPMTUD and Initial Connectivity
+
+From the perspective of DPLPMTUD, QUIC transport is an 
+acknowledged PL. A sender can therefore enter the DPLPMTUD BASE

Expand on first use please.  More so because this acronym is used only once here.

```suggestion
acknowledged packetization layer (PL). A sender can therefore enter the DPLPMTUD BASE
```

>  
-The considerations for processing ICMP messages in the previous section also
+When implementing the algorithm in Section 5 of {{!DPLPMTUD}}, the
+initial value of BASE_PMTU SHOULD be consistent with the minimum QUIC
+packet size. The MIN_PLPMTU is the same as the BASE_PMTU.
+
+QUIC endpoints implementing DPLPMTUD maintain a maximum packet size 
+(DPLPMTUD MPS) for each combination of local and remote IP
+addresses. 

Missing paragraph break.
```suggestion
addresses.

```

> +QUIC endpoints implementing DPLPMTUD maintain a maximum packet size 
+(DPLPMTUD MPS) for each combination of local and remote IP
+addresses. 

Is this a path property?  Because this could be "pair of local and remote addresses" (strike the IP) and then it would include port numbers too.  That is likely more general.

> +state when the QUIC connection handshake has been completed and
+the endpoint has established a 1-RTT key.

Is it necessary for this to happen so late?  I see some implementations probing much earlier than that.  Also, this definition is a new state.  We have "handshake completed", but 1-RTT keys are established earlier than that.  I would just say "handshake completed" and leave it at that.

```suggestion
state when the QUIC connection handshake is complete.
```

> +DPLPMTU Probe packets consists of a QUIC Header and a payload containing a
+PING Frame and multiple PADDING Frames, this can implement
+"Probing using padding data" (see section 4.1 of {{!DPLPMTUD}}. 

This is too narrowly restrictive. I would instead say:

```suggestion
DPLPMTU probe packets are ack-eliciting packets.  Probe packets that use the
PADDING frame therefore implement "Probing using padding data", as defined in
Section 4.1 of {{!DPLPMTUD}}.
```

> +These can be generated without affecting the transfer of other QUIC frames.
+The PING Frame is used to trigger generation of an acknowledgement.
+Multiple PADDING Frames are used together to control the length of the probe packet.  

Again, too prescriptive.

```suggestion
```

> +These frames might not be retransmitted if a probe packet containing
+them is lost.  The frames consume congestion window,
+which could delay the transmission of subsequent application data.

This is incorrect.  PADDING and PING are not retransmitted on loss.  The fact that they are ack-eliciting causes them to consume congestion window, but this is less of a problem if they include other data.  Which they may well do (albeit with a higher risk of that data needing retransmission).

```suggestion

DPLPMTUD probe packets consume congestion window, so they could delay
the transmission of subsequent application.
```

> +DPLPMTU Probe packets consists of a QUIC Header and a payload containing a
+PING Frame and multiple PADDING Frames, this can implement
+"Probing using padding data" (see section 4.1 of {{!DPLPMTUD}}. 
+These can be generated without affecting the transfer of other QUIC frames.
+The PING Frame is used to trigger generation of an acknowledgement.
+Multiple PADDING Frames are used together to control the length of the probe packet.  
+These frames might not be retransmitted if a probe packet containing
+them is lost.  The frames consume congestion window,
+which could delay the transmission of subsequent application data.
+
+###  Validating the QUIC Path with DPLPMTUD
+
+QUIC provides an acknowledged PL, therefore a sender does not
+implement the DPLPMTUD CONFIRMATION_TIMER while in the SEARCH_COMPLETE state.
+
+###  Handling of ICMP PTB Messages by DPLPMTUD

```suggestion
###  Handling of ICMP Messages by DPLPMTUD
```

> +which could delay the transmission of subsequent application data.
+
+###  Validating the QUIC Path with DPLPMTUD
+
+QUIC provides an acknowledged PL, therefore a sender does not
+implement the DPLPMTUD CONFIRMATION_TIMER while in the SEARCH_COMPLETE state.
+
+###  Handling of ICMP PTB Messages by DPLPMTUD
+
+An endpoint using DPLPMTUD requires the validation of any received PTB message 
+before using the PTB information, as defined in section 4.6 of {{!DPLPMTUD}}.
+In addition to UDP Port
+validation, QUIC validates an ICMP message by using other PL
+information (e.g., validation of connection identifiers (CIDs) in the
+quoted packet of any received ICMP message). 
+The further considerations for processing ICMP messages described in the previous section also

```suggestion

The considerations for processing ICMP messages described in {{icmp-pmtud}} also
```

>  The endpoint SHOULD ignore all ICMP messages that fail validation.
 
 An endpoint MUST NOT increase PMTU based on ICMP messages; see Section 3, clause
 6 of {{!DPLPMTUD}}.  Any reduction in the QUIC maximum packet size in response
 to ICMP messages MAY be provisional until QUIC's loss detection algorithm
 determines that the quoted packet has actually been lost.
 
+### PMTUD Probes with Handshake packets

Please move this back.  It belongs where it was taken from.

I realize that you now have a section for PMTUD parallel to the section for PLPMTUD, and this allows you to make the structure cleaner (with two subsections and not one, which I agree is nice), but this text is relevant to both.

(I think that this move points out that the specific use of Handshake isn't the ideal. Though Initial packets might be a bad idea, using 0-RTT is also fine.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3693#pullrequestreview-417911791