Hi,
On Tue, Feb 13, 2024 at 04:54:24PM +0400, Sergey Kandaurov wrote:
>
> > On 9 Feb 2024, at 13:56, Roman Arutyunyan <arut@nginx.com> wrote:
> >
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1707472496 -14400
> > # Fri Feb 09 13:54:56 2024 +0400
> > # Node ID 9b89f44ddd3637afc939e31de348c7986ae9e76d
> > # Parent 73eb75bee30f4aee66edfb500270dbb14710aafd
> > QUIC: fixed unsent MTU probe acknowledgement.
> >
> > Previously if an MTU probe send failed early in ngx_quic_frame_sendto()
> > due to allocation error or congestion control, the application level packet
> > number was not increased, but was still saved as MTU probe packet number.
> > Later when a packet with this number was acknowledged, the unsent MTU probe
> > was acknowledged as well. This could result in discovering a bigger MTU than
> > supported by the path, which could lead to EMSGSIZE (Message too long) errors
> > while sending further packets.
> >
> > The problem existed since PMTUD was introduced in 58afcd72446f (1.25.2).
> > Back then only the unlikely memory allocation error could trigger it. However
> > in efcdaa66df2e congestion control was added to ngx_quic_frame_sendto() which
> > can now trigger the issue with a higher probability.
> >
> > diff --git a/src/event/quic/ngx_event_quic_migration.c b/src/event/quic/ngx_event_quic_migration.c
> > --- a/src/event/quic/ngx_event_quic_migration.c
> > +++ b/src/event/quic/ngx_event_quic_migration.c
> > @@ -925,12 +925,6 @@ ngx_quic_send_path_mtu_probe(ngx_connect
> >
> > qc = ngx_quic_get_connection(c);
> > ctx = ngx_quic_get_send_ctx(qc, ssl_encryption_application);
> > - path->mtu_pnum[path->tries] = ctx->pnum;
> > -
> > - ngx_log_debug4(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > - "quic path seq:%uL send probe "
> > - "mtu:%uz pnum:%uL tries:%ui",
> > - path->seqnum, path->mtud, ctx->pnum, path->tries);
> >
> > log_error = c->log_error;
> > c->log_error = NGX_ERROR_IGNORE_EMSGSIZE;
> > @@ -943,14 +937,26 @@ ngx_quic_send_path_mtu_probe(ngx_connect
> > path->mtu = mtu;
> > c->log_error = log_error;
> >
> > + if (rc == NGX_OK) {
> > + path->mtu_pnum[path->tries] = ctx->pnum;
>
> It's too late to save the packet number here after we've sent it.
> A successful call to ngx_quic_output_packet() or ngx_quic_frame_sendto()
> updates ctx->pnum to contain the next packet number, so it's off-by-one.
> It may have sense to preserve mtu_pnum, and restore it on non-success,
> but see below.
Indeed, thanks.
> > +
> > + ngx_log_debug4(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > + "quic path seq:%uL send probe "
> > + "mtu:%uz pnum:%uL tries:%ui",
> > + path->seqnum, path->mtud, ctx->pnum, path->tries);
>
> IMHO, such late logging makes hard to follow through debug log messages.
> I'd prefer to keep it first, before all the underlined logging.
Logging early may report the pnum that will not be sent.
However we can leave it as is for simplicity.
> > +
> > + return NGX_OK;
> > + }
> > +
> > + path->mtu_pnum[path->tries] = NGX_QUIC_UNSET_PN;
>
> This will break matching ACK'ed probes on subsequent retries in
> ngx_quic_handle_path_mtu(), it stops looking after NGX_QUIC_UNSET_PN.
> Possible solutions are to rollback path->tries after NGX_AGAIN from
> ngx_quic_frame_sendto(), or to ignore NGX_QUIC_UNSET_PN in
> ngx_quic_handle_path_mtu().
Rolling back path->tries is hardly possible. We need to skip NGX_QUIC_UNSET_PN
ngx_quic_handle_path_mtu().
> > +
> > + ngx_log_debug2(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > + "quic path seq:%uL rejected mtu:%uz",
> > + path->seqnum, path->mtud);
> > +
> > if (rc == NGX_ERROR) {
> > if (c->write->error) {
> > c->write->error = 0;
> > -
> > - ngx_log_debug2(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > - "quic path seq:%uL rejected mtu:%uz",
> > - path->seqnum, path->mtud);
> > -
> > return NGX_DECLINED;
> > }
> >
> > _______________________________________________
> > nginx-devel mailing list
> > nginx-devel@nginx.org
> > https://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> --
> Sergey Kandaurov
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1707915388 -14400
# Wed Feb 14 16:56:28 2024 +0400
# Node ID 2ed3f57dca0a664340bca2236c7d614902db4180
# Parent 73eb75bee30f4aee66edfb500270dbb14710aafd
QUIC: fixed unsent MTU probe acknowledgement.
Previously if an MTU probe send failed early in ngx_quic_frame_sendto()
due to allocation error or congestion control, the application level packet
number was not increased, but was still saved as MTU probe packet number.
Later when a packet with this number was acknowledged, the unsent MTU probe
was acknowledged as well. This could result in discovering a bigger MTU than
supported by the path, which could lead to EMSGSIZE (Message too long) errors
while sending further packets.
The problem existed since PMTUD was introduced in 58afcd72446f (1.25.2).
Back then only the unlikely memory allocation error could trigger it. However
in efcdaa66df2e congestion control was added to ngx_quic_frame_sendto() which
can now trigger the issue with a higher probability.
diff --git a/src/event/quic/ngx_event_quic_migration.c b/src/event/quic/ngx_event_quic_migration.c
--- a/src/event/quic/ngx_event_quic_migration.c
+++ b/src/event/quic/ngx_event_quic_migration.c
@@ -909,6 +909,7 @@ static ngx_int_t
ngx_quic_send_path_mtu_probe(ngx_connection_t *c, ngx_quic_path_t *path)
{
size_t mtu;
+ uint64_t pnum;
ngx_int_t rc;
ngx_uint_t log_error;
ngx_quic_frame_t *frame;
@@ -925,7 +926,7 @@ ngx_quic_send_path_mtu_probe(ngx_connect
qc = ngx_quic_get_connection(c);
ctx = ngx_quic_get_send_ctx(qc, ssl_encryption_application);
- path->mtu_pnum[path->tries] = ctx->pnum;
+ pnum = ctx->pnum;
ngx_log_debug4(NGX_LOG_DEBUG_EVENT, c->log, 0,
"quic path seq:%uL send probe "
@@ -943,14 +944,18 @@ ngx_quic_send_path_mtu_probe(ngx_connect
path->mtu = mtu;
c->log_error = log_error;
+ if (rc == NGX_OK) {
+ path->mtu_pnum[path->tries] = pnum;
+ return NGX_OK;
+ }
+
+ ngx_log_debug2(NGX_LOG_DEBUG_EVENT, c->log, 0,
+ "quic path seq:%uL rejected mtu:%uz",
+ path->seqnum, path->mtud);
+
if (rc == NGX_ERROR) {
if (c->write->error) {
c->write->error = 0;
-
- ngx_log_debug2(NGX_LOG_DEBUG_EVENT, c->log, 0,
- "quic path seq:%uL rejected mtu:%uz",
- path->seqnum, path->mtud);
-
return NGX_DECLINED;
}
@@ -976,7 +981,7 @@ ngx_quic_handle_path_mtu(ngx_connection_
pnum = path->mtu_pnum[i];
if (pnum == NGX_QUIC_UNSET_PN) {
- break;
+ continue;
}
if (pnum < min || pnum > max) {
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
