> On 9 Feb 2024, at 13:56, Roman Arutyunyan <a...@nginx.com> wrote:
>
> # HG changeset patch
> # User Roman Arutyunyan <a...@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.
> +
> + 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.
> +
> + 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().
> +
> + 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
