On Wed, Feb 14, 2024 at 05:09:35PM +0400, Roman Arutyunyan wrote:
> Hi,
>
> On Tue, Feb 13, 2024 at 04:54:24PM +0400, Sergey Kandaurov wrote:
> >
> > > 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.
>
> 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;
> > > }
> > >
> # HG changeset patch
> # User Roman Arutyunyan <a...@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) {
Looks good.
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
