[PATCH] QUIC: fixed unsent MTU probe acknowledgement

[Sergey Kandaurov]

> On 9 Feb 2024, at 13:56, Roman Arutyunyan <arut at nginx.com> wrote:
> 
> # HG changeset patch
> # User Roman Arutyunyan <arut at 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 at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Sergey Kandaurov