Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DPLPMTU merge tweaks #3702

Merged
merged 35 commits into from
Jun 9, 2020
Merged

DPLPMTU merge tweaks #3702

merged 35 commits into from
Jun 9, 2020

Conversation

martinthomson
Copy link
Member

This is #3693 with some minor additional changes:

  1. Reflow.
  2. Move the odd text about using Handshake back where it belongs.
  3. Make the language about how to construct a probe less prescriptive.
  4. Mention 1200 when referring to the "smallest allowed maximum packet size" so it is clear what that means.

Closes #3693.
Closes #3695.

gorryfair and others added 19 commits May 25, 2020 14:29
This set of changes seeks to:
* Complete the transfer of DPLPMTUD text from the DPLPMTUD-RFC-to-be to this ID.
* Separate PMTUD from DPLPMTUD where they differ.
* Seek consistent language for packet sizes within these sections.
That would be good, the change was unintended.

Co-authored-by: ianswett <ianswett@users.noreply.github.com>
I like that:-)

Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
This looks good

Co-authored-by: ianswett <ianswett@users.noreply.github.com>
This looks good

Co-authored-by: Martin Thomson <mt@lowentropy.net>
This looks good

Co-authored-by: Martin Thomson <mt@lowentropy.net>
This looks good

Co-authored-by: Martin Thomson <mt@lowentropy.net>
That would be OK. DPLPMTUD only handles PTB messages.

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Typo fixed.

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Fix for /application data/
Seems correct

Co-authored-by: Martin Thomson <mt@lowentropy.net>
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels May 27, 2020
gorryfair and others added 2 commits May 27, 2020 08:43
Understood, and corrected.

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Changes to resolve description of probe packets and need to explain the maximum size.
Copy link
Contributor

@gorryfair gorryfair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reflow of the text seems clear to me, and seems nearly ready. I have carefully checked the text and I think this covers all issues and responds to all discussion, thanks. There are a small number of NiTs and one or two small questions remain.

From the perspective of DPLPMTUD, QUIC transport is an acknowledged
packetization layer (PL). A sender can therefore enter the DPLPMTUD BASE state
when the QUIC connection handshake has been completed and the endpoint has
established a 1-RTT key.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we agreed to remove the 1-RTT clause (as not needed)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Missed that in my pass. Corrected.

packets ({{long-header}}) contain source connection IDs, and long header packets
are not decrypted or acknowledged by the peer once the handshake is complete.

One way to construct a PMTU probe is to coalesce (see {{packet-coalesce}}) a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reading this: Is this only for PMTUD?

The text seems to describe a probe packet that is ack-eliciting, which may mean it can be used with PLPMTUD?
Should we say PMTUD/DPLPMTUD

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this should be either.

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}}. Endpoints could limit the content of probe
packets to PING and PADDING frames as packets that are larger than the current
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check: This may leave data in probe packets slightly under-described, and I think more may need to be said:

The transmission of data in probe packets in discussed in
section 4.1 of {{!DPLPMTUD}}. Probe packets that use a PADDING frame or that coalesce packets with no data implement DPLPMTUD with "Probing using padding data".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coalescing part sort of reveals a flaw in the construction though. It shows that the probe really uses datagrams. At this level, I think that assuming the hack (coalescing with junk Handshake) is not involved, is OK. Happy to take suggestions though, because this is definitely awkward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was starting to leak badly. I think there's some cleaning up that needs to be done in terms of bringing consistency overall, see meta comment above.

@@ -3797,19 +3797,24 @@ later time in the connection.
The QUIC packet size includes the QUIC header and protected payload, but not the
UDP or IP header.

QUIC depends upon a minimum packet size of at least 1280 bytes. This is the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protocol layer relating to "packet size" needs clarification - is it an IP packet or an IP payload, or QUIC packet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. That's been sitting there for ages and you are the first to notice.

supports a reasonable Maximum Transmission Unit (MTU). Padding datagrams also
helps reduce the amplitude of amplification attacks caused by server responses
toward an unverified client address; see {{address-validation}}.
supports a reasonable Path Maximum Transmission Unit (PMTU). Padding datagrams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/reasonable/minimum size required by QUIC/

supports a reasonable Maximum Transmission Unit (MTU). Padding datagrams also
helps reduce the amplitude of amplification attacks caused by server responses
toward an unverified client address; see {{address-validation}}.
supports a reasonable Path Maximum Transmission Unit (PMTU). Padding datagrams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... If coalescing also has this property, should we say:
/Passing datagrams/Using the minimum packet size/

causes an ICMP message to be sent, the first part of the datagram will be quoted
in that message. If the source connection ID is within the quoted portion of
the UDP datagram, that could be used for routing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used for routing ... add: /and for validation of an ICMP message./

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. And it is either. (Though using other junk in the packet might be better for validation, if the quoted part is indeed long enough. One real concern with this hack is that the router might not quote enough of the packet include the the whole Source Connection ID field, especially if the Destination Connection ID - which you can't truncate either - has to be longer.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-). (Knowing that needs a measurement survey) However, the implications are you wouldn't be able to rely on ICMP to help you. Well that's no new news there are many reasons why you may not see a PTB message, that's why DPLPMTUD was made.


Note:
: The purpose of using a packet with a long header is only to ensure that the
quoted packet contained in the ICMP message contains a Source Connection ID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/SCID/ may already be defined if you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've taken to writing these out in full.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-)

Note:
: The purpose of using a packet with a long header is only to ensure that the
quoted packet contained in the ICMP message contains a Source Connection ID
field that can be use for routing. This packet does not need to be a valid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...only for routing? Does this help with validation (the editor will know best).

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think /CID/ is already defined at this point in the ID.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we use is, so thanks for drawing attention to it. We use "connection ID" and so I have fixed this.

Copy link
Member Author

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review Gorry. You caught a few mistakes - one of them being quite old.

@@ -3797,19 +3797,24 @@ later time in the connection.
The QUIC packet size includes the QUIC header and protected payload, but not the
UDP or IP header.

QUIC depends upon a minimum packet size of at least 1280 bytes. This is the
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. That's been sitting there for ages and you are the first to notice.

From the perspective of DPLPMTUD, QUIC transport is an acknowledged
packetization layer (PL). A sender can therefore enter the DPLPMTUD BASE state
when the QUIC connection handshake has been completed and the endpoint has
established a 1-RTT key.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Missed that in my pass. Corrected.

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}}. Endpoints could limit the content of probe
packets to PING and PADDING frames as packets that are larger than the current
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coalescing part sort of reveals a flaw in the construction though. It shows that the probe really uses datagrams. At this level, I think that assuming the hack (coalescing with junk Handshake) is not involved, is OK. Happy to take suggestions though, because this is definitely awkward.

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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we use is, so thanks for drawing attention to it. We use "connection ID" and so I have fixed this.

packets ({{long-header}}) contain source connection IDs, and long header packets
are not decrypted or acknowledged by the peer once the handshake is complete.

One way to construct a PMTU probe is to coalesce (see {{packet-coalesce}}) a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this should be either.

causes an ICMP message to be sent, the first part of the datagram will be quoted
in that message. If the source connection ID is within the quoted portion of
the UDP datagram, that could be used for routing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. And it is either. (Though using other junk in the packet might be better for validation, if the quoted part is indeed long enough. One real concern with this hack is that the router might not quote enough of the packet include the the whole Source Connection ID field, especially if the Destination Connection ID - which you can't truncate either - has to be longer.)


Note:
: The purpose of using a packet with a long header is only to ensure that the
quoted packet contained in the ICMP message contains a Source Connection ID
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've taken to writing these out in full.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A high order point. This could use a clean up in terms of noun consistency:

  • PMTU, DPLPMPTU
  • PMTUD/DPLPMTUD/PMTU/DPLPMTU probes / probe packets. I would simply use "probes" and define this away.

My suggestion would be to add a Terminology subsection at the top of this section, so that we can simply define away the ambiguities.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clause does not make sense to me. What does a DPLPMTUD probe packet look like if it's not a QUIC packet? I think this clause should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine to remove: DPLPMTUD will anyway send some "probes" after it thinks a blackhole is detected, to confirm if the black hole exists. Once confirmed I understand QUIC will cease sending.


From the perspective of DPLPMTUD, QUIC transport is an acknowledged
packetization layer (PL). A sender can therefore enter the DPLPMTUD BASE state
when the QUIC connection handshake has been completed and the endpoint has
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use "handshake confirmed". That said, is there a reason to disallow this during the handshake?


### Sending QUIC DPLPMTUD Probe Packets

DPLPMTU probe packets are ack-eliciting packets. Probe packets that use the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like we are using a term without defining it. Perhaps "DPLPMTU probe packets require acknowledgements (Section 3 of {{!DPLPMTUD}}), and are therefore ack-eliciting packets"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
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}}. Endpoints could limit the content of probe
packets to PING and PADDING frames as packets that are larger than the current
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was starting to leak badly. I think there's some cleaning up that needs to be done in terms of bringing consistency overall, see meta comment above.

martinthomson and others added 2 commits May 28, 2020 11:31
Co-authored-by: Jana Iyengar <jri.ietf@gmail.com>
Copy link
Contributor

@gorryfair gorryfair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look goof to me.

maximum packet size are more likely to be dropped by the network.

DPLPMTU probe packets consume congestion window, which could delay subsequent
transmission by an application.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here is more to consider. QUIC's loss detection will trigger on a lost probe packet. Even tough this does not lead to a retransmission, since PING and PAD frames won't be retransmitted, one could misinterpret the loss as a congestion signal. It might worth to note that a lost probe packet should not be treated as a congestion signal by the congestion control.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section 3, bullet 7 of DPLPMTUD clarifies this as: "Loss of a probe packet SHOULD NOT be treated as an indication of congestion and SHOULD NOT trigger a congestion control reaction". Maybe this could be added as clarification?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe one of you can make a suggestion for what this can say (or file an issue for follow-up).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this makes sense? (I merged both paragraphs in the section)

... Endpoints could limit the content of probe packets to PING and PADDING frames as packets that are larger than the current maximum packet size are more likely to be dropped by the network. These frames might not be retransmitted if a probe packet containing them is lost. Additionally, a lost probe packet should not be treated as a signal of congestion. However, these probe packets do consume congestion window, which could delay the transmission of subsequent application data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly, could we say:

Probe packets are larger than the current maximum packet size and therefore more likely to be dropped by the network. Endpoints could therefore limit the content of probe packets to ACK-eliciting frames that are not retransmitted (e.g., using PING and PADDING frames). Loss of a probe packet SHOULD NOT be treated as an indication of congestion and SHOULD NOT trigger a congestion control reaction (see section 3, bullet 7 of {{!DPLPMTUD}}. However, probe packets do consume congestion window, which could delay subsequent transmission by an application.

@martinthomson
Copy link
Member Author

@janaiyengar, I went with "PMTU probe packet", which is an IP packet. Let me know if that makes sense.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your changes! I think it might be useful to distinguish the "IP packet" from how we commonly use "packet", so I propose (i) simply use "PMTU probe", and (ii) saying explicitly what that refers to. I've made some suggestions through the text to this end; see what you think.


The QUIC maximum packet size is the largest size of QUIC packet that can be sent
across a network path using a single packet. Any maximum packet size larger than
1200 bytes can be discovered using PMTUD/DPLPMTUD.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expand/cite for PMTUD/DPLPMTUD. I don't think it's been introduced before.

Comment on lines 3849 to 3851
An endpoint SHOULD use Datagram Packetization Layer PMTU Discovery
({{!DPLPMTUD=I-D.ietf-tsvwg-datagram-plpmtud}}) or implement Path MTU Discovery
(PMTUD) {{!RFC1191}} {{!RFC8201}} to determine whether the path to a destination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this above to where PMTUD and DPLPMTUD are used for the first time.

Comment on lines 3863 to 3864
bytes, it MUST immediately cease sending QUIC packets, except for PMTU probe
packets, on the affected path. An endpoint MAY terminate the connection if an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bytes, it MUST immediately cease sending QUIC packets, except for PMTU probe
packets, on the affected path. An endpoint MAY terminate the connection if an
bytes, it MUST immediately cease sending QUIC packets, except for those in PMTU
probes, on the affected path. An endpoint MAY terminate the connection if an

Comment on lines 3915 to 3916
PMTU probe packets for DPLPMTUD that use the PADDING frame implement "Probing
using padding data", as defined in Section 4.1 of {{!DPLPMTUD}}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PMTU probe packets for DPLPMTUD that use the PADDING frame implement "Probing
using padding data", as defined in Section 4.1 of {{!DPLPMTUD}}.
PMTU probes for DPLPMTUD that use the PADDING frame implement "Probing
using padding data", as defined in Section 4.1 of {{!DPLPMTUD}}.

Comment on lines 3856 to 3859
Both PMTUD and DPLPMTUD send IP packets that are larger than the current maximum
packet size. Aside from these PMTU probe packets, all QUIC packets SHOULD be
sized to fit within the maximum packet size to avoid the packet being fragmented
or dropped {{?RFC8085}}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Both PMTUD and DPLPMTUD send IP packets that are larger than the current maximum
packet size. Aside from these PMTU probe packets, all QUIC packets SHOULD be
sized to fit within the maximum packet size to avoid the packet being fragmented
or dropped {{?RFC8085}}.
Both PMTUD and DPLPMTUD send IP packets that are larger than the current maximum
packet size. We refer to these packets as PMTU probes. All QUIC packets that are not
in a PMTU probe SHOULD be sized to fit within the maximum packet size to avoid the
packet being fragmented or dropped {{?RFC8085}}.

connection ID is within the quoted portion of the UDP datagram, that could be
used for routing.
packets are likely to require that the connection ID be included in
PMTU probe packets to route any resulting ICMP messages ({{icmp-pmtud}}) back to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PMTU probe packets to route any resulting ICMP messages ({{icmp-pmtud}}) back to
PMTU probes to route any resulting ICMP messages ({{icmp-pmtud}}) back to

contain the Source Connection ID field, and long header packets are not
decrypted or acknowledged by the peer once the handshake is complete.

One way to construct a PMTU probe packet is to coalesce (see
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
One way to construct a PMTU probe packet is to coalesce (see
One way to construct a PMTU probe is to coalesce (see

One way to construct a PMTU probe packet is to coalesce (see
{{packet-coalesce}}) a packet with a long header, such as a Handshake or 0-RTT
packet ({{long-header}}), with a short header packet in a single UDP datagram.
If the resulting PMTU probe packet reaches the endpoint, the packet with the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If the resulting PMTU probe packet reaches the endpoint, the packet with the
If the resulting PMTU probe reaches the endpoint, the packet with the

packet ({{long-header}}), with a short header packet in a single UDP datagram.
If the resulting PMTU probe packet reaches the endpoint, the packet with the
long header will be ignored, but the short header packet will be acknowledged.
If the UDP datagram causes an ICMP message to be sent, the first part of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If the UDP datagram causes an ICMP message to be sent, the first part of the
If the PMTU probe causes an ICMP message to be sent, the first part of the

If the resulting PMTU probe packet reaches the endpoint, the packet with the
long header will be ignored, but the short header packet will be acknowledged.
If the UDP datagram causes an ICMP message to be sent, the first part of the
datagram will be quoted in that message. If the Source Connection ID field is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
datagram will be quoted in that message. If the Source Connection ID field is
probe will be quoted in that message. If the Source Connection ID field is

@gorryfair
Copy link
Contributor

Thank you for your changes! I think it might be useful to distinguish the "IP packet" from how we commonly use "packet", so I propose (i) simply use "PMTU probe", and (ii) saying explicitly what that refers to. I've made some suggestions through the text to this end; see what you think.

OK - There are already "IP packets" and "QUIC packets" - so some other term works fro me.

network path from the client to the server supports a reasonable Path Maximum
Transmission Unit (PMTU). This also helps reduce the amplitude of amplification
attacks caused by server responses toward an unverified client address; see
{{address-validation}}.

Enforcement of the max_udp_payload_size transport parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this paragraph above the one above, since they both talk about maximum packet size and in particular this paragraph uses "additional limit" which reads better one paragraph up.

connection by sending a CONNECTION_CLOSE frame with an error code of
PROTOCOL_VIOLATION; see {{immediate-close-hs}}.
A server MUST discard an Initial packet that is carried in a UDP datagram with a
payload that is less than the smallest allowed maximum packet size of 1200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
payload that is less than the smallest allowed maximum packet size of 1200
payload that is less than the smallest allowed maximum packet size of 1200

An endpoint SHOULD use DPLPMTUD ({{dplpmtud}}) or PMTUD ({{pmtud}}) to determine
whether the path to a destination will support a desired message size without
fragmentation. In the absence of these mechanisms, QUIC endpoints SHOULD NOT
send IP packets larger than the minimum QUIC packet size.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
send IP packets larger than the minimum QUIC packet size.
send IP packets larger than the smallest allowed maximum packet size.

If a QUIC endpoint determines that the PMTU between any pair of local and remote
IP addresses has fallen below the smallest allowed maximum packet size of 1200
bytes, it MUST immediately cease sending QUIC packets, except for those in PMTU
probes, on the affected path. An endpoint MAY terminate the connection if an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't new text, but tt seems like these two statements may be contradictory? Should this be "except for those in PMTU probes or containing connection close frames, on the affected path."?

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 on the
affected path. An endpoint MAY terminate the connection if an alternative path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit repetitive of 3862-3866. It's also still confusing or potentially contradictory to me.

martinthomson added a commit that referenced this pull request Jun 4, 2020
The PMTUD one I will add to #3702.  The others I don't plan to fix.

Closes #3724.
@martinthomson martinthomson mentioned this pull request Jun 4, 2020
@gorryfair
Copy link
Contributor

A top-level comment on the term "probing":
In Section: Probing a New Path: A packet containing only probing frames is a "probing packet", and a packet containing any other frame is a "non-probing packet". Please be sure that "probe" can not be confused with "probing" in PLPMTUD.

@gorryfair
Copy link
Contributor

In the subsection on “Packet Size {#packet-size}”, I think the blocks of text describe two things. There is some general guidance on size, and there are specific instructions for the size of the initial packet. I’d suggest we do try to separate these two, it would make it much easier for someone to understand the size constraints for forming the initial packet.

network path from the client to the server supports a reasonable Path Maximum
Transmission Unit (PMTU). This also helps reduce the amplitude of amplification
attacks caused by server responses toward an unverified client address; see
{{address-validation}}.

Datagrams containing Initial packets MAY exceed 1200 bytes if the client
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be 1200B of payload? (We need to be careful about which layer these numbers are describing, since the datagram itself will be larger.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we caught this in #3742.

@martinthomson
Copy link
Member Author

Regarding the sizing of Initial packets, see #3746.

I checked the text for probing and we have "PMTUD probe" in one case and "probing packet" elsewhere. It's not ideal, but I didn't see any cases that were clearly busted. I don't have a good idea for alternative labels, but would welcome suggestions.

@martinthomson martinthomson merged commit 18becf2 into master Jun 9, 2020
@martinthomson martinthomson deleted the dplpmtud-merge branch June 9, 2020 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate QUIC DPLPMTUD text
5 participants