Commit Graph

18277 Commits

Author SHA1 Message Date
Frédéric Lécaille
a6920a25d9 MINOR: quic: Remove useless lock for RX packets
This lock was there be able to handle the RX packets for a connetion
from several threads. This is no more needed since a QUIC connection
is always handled by the same thread.

May be backported to 2.6
2022-08-11 14:33:43 +02:00
Willy Tarreau
6a378d1677 BUILD: stconn: fix build warning at -O3 about possible null sc
gcc-6.x and 7.x emit build warnings about sc possibly being null upon
return from sc_detach_endp(). This actually is not the case and the
compiler is a little bit overzealous there, but there exists code
paths that can make this analysis non-trivial so let's at least add
a similar BUG_ON() to let both the compiler and the deverloper know
this doesn't happen.

This should be backported to 2.6.
2022-08-11 13:59:13 +02:00
Frédéric Lécaille
a8b2f843d2 MEDIUM: quic: xprt traces rework
Add a least as much as possible TRACE_ENTER() and TRACE_LEAVE() calls
to any function. Note that some functions do not have any access to the
a quic_conn argument when  receiving or parsing datagram at very low level.
2022-08-11 11:11:20 +02:00
Willy Tarreau
341ac99f4d BUG/MEDIUM: task: relax one thread consistency check in task_unlink_wq()
While testing the fix for the previous issue related to reloads with
hard_stop_after, I've met another one which could spuriously produce:

  FATAL: bug condition "t->tid >= 0 && t->tid != tid" matched at include/haproxy/task.h:266

In 2.3-dev2, we've added more consistency checks for a number of bug-
inducing programming errors related to the tasks, via commit e5d79bccc
("MINOR: tasks/debug: add a few BUG_ON() to detect use of wrong timer
queue"), and this check comes from there.

The problem that happens here is that when hard-stop-after is set, we
can abort the current thread even if there are still ongoing checks
(or connections in fact). In this case some tasks are present in a
thread's wait queue and are thus bound exclusively to this thread.

During deinit(), the collect and cleanup of all memory areas also
stops servers and kills their check tasks. And calling task_destroy()
does in turn call task_unlink_wq()... except that it's called from
thread 0 which doesn't match the initially planned thread number.

Several approaches are possible. One of them would consist in letting
threads perform their own cleanup (tasks, pools, FDs, etc). This would
possibly be even faster since done in parallel, but some corner cases
might be way more complicated (e.g. who will kill a check's task, or
what to do with a task found in a local wait queue or run queue, and
what about other consistency checks this could violate?).

Thus for now this patches takes an easier and more conservative
approach consisting in admitting that when the process is stopping,
this rule is not necessarily valid, and to let thread 0 collect all
other threads' garbage.

As such this patch can be backpoted to 2.4.
2022-08-10 18:03:11 +02:00
Willy Tarreau
e6ca435c04 BUG/MEDIUM: poller: use fd_delete() to release the poller pipes
The poller pipes needed to communicate between multiple threads are
allocated in init_pollers_per_thread() and released in
deinit_pollers_per_thread(). The former adds them via fd_insert()
so that they are known, but the former only closes them using a
regular close().

This asymmetry represents a problem, because we have in the fdtab[]
an entry for something that may disappear when one thread leaves, and
since these FD numbers are very low, there is a very high likelihood
that they are immediately reassigned to another thread trying to
connect() to a server or just sending a health check. In this case,
the other thread is going to fd_insert() the fd and the recently
added consistency checks will notive that ->owner is not NULL and
will crash. We just need to use fd_delete() here to match fd_insert().

Note that this test was added in 2.7-dev2 by commit 36d9097cf
("MINOR: fd: Add BUG_ON checks on fd_insert()") which was backported
to 2.4 as a safety measure (since it allowed to catch particularly
serious issues). The patch in itself isn't wrong, it just revealed
a long-dormant bug (been there since 1.9-dev1, 4 years ago). As such
the current patch needs to be backported wherever the commit above
is backported.

Many thanks to Christian Ruppert for providing detailed traces in
github issue #1807 and Cedric Paillet for bringing his complementary
analysis that helped to understand the required conditions for this
issue to happen (fast health checks @100ms + randomly long connections
~7s + fast reloads every second + hard-stop-after 5s were necessary
on the dev's machine to trigger it from time to time).
2022-08-10 17:25:23 +02:00
Willy Tarreau
54bc78693d BUG/MEDIUM: quic: always remove the connection from the accept list on close
Fred managed to reproduce a crash showing a corrupted accept_list when
firing thousands of concurrent picoquicdemo clients to a same instance.
It may happen if the connection was placed into the accept_list and
immediately closed before being processed (e.g. on error or t/o ?).

In any case the quic_conn_release() function should always detach a
connection to be deleted from any list, like it does for other lists,
so let's add an MT_LIST_DELETE() here.

This should be backported to 2.6.
2022-08-10 07:30:22 +02:00
Amaury Denoyelle
f0f92b2db8 BUG/MINOR: quic: fix crash on handshake io-cb for null next enc level
When arriving at the handshake completion, next encryption level will be
null on quic_conn_io_cb(). Thus this must be check this before
dereferencing it via qc_need_sending() to prevent a crash.

This was reproduced quickly when browsing over a local nextcloud
instance through QUIC with firefox.

This has been introduced in the current dev with quic-conn Tx
refactoring. No need to backport it.
2022-08-09 18:01:10 +02:00
Amaury Denoyelle
96ca1b7c39 BUG/MINOR: mux-quic: open stream on STOP_SENDING
Considered a stream as opened when receiving a STOP_SENDING frame as the
first frame on the stream.

This patch is tagged as BUG because a BUG_ON may occur if only a
STOP_SENDING frame has been received for a frame. This will reset the
stream in respect with RFC9000 but internally it is considered invalid
transition to reset an idle stream.

To fix this, simply use qcs_idle_open() on STOP_SENDING parsing
function. This will mark the stream as OPEN before resetting it.

This was detected on haproxy.org with the following backtrace :

FATAL: bug condition "qcs->st == QC_SS_IDLE" matched at
src/mux_quic.c:383
  call trace(12):
  |       0x490dd3 [b8 01 00 00 00 c6 00 00]: main-0x1d0633
  |       0x4975b8 [48 8b 85 58 ff ff ff 8b]: main-0x1c9e4e
  |       0x497df4 [48 8b 45 c8 48 89 c7 e8]: main-0x1c9612
  |       0x49934c [48 8b 45 c8 48 89 c7 e8]: main-0x1c80ba
  |       0x6b3475 [48 8b 05 54 1b 3a 00 64]: run_tasks_from_lists+0x45d/0x8b2
  |       0x6b4093 [29 c3 89 d8 89 45 d0 83]: process_runnable_tasks+0x7c9/0x824
  |       0x660bde [8b 05 fc b3 4f 00 83 f8]: run_poll_loop+0x74/0x430
  |       0x6611de [48 8b 05 7b a6 40 00 48]: main-0x228
  | 0x7f66e4fb2ea5 [64 48 89 04 25 30 06 00]: libpthread:+0x7ea5
  | 0x7f66e455ab0d [48 89 c7 e8 5b 72 fc ff]: libc:clone+0x6d/0x86

Stream states have been implemented in the current dev tree. Thus, this
patch does not need to be backported.
2022-08-09 17:58:02 +02:00
Amaury Denoyelle
c09ef0c5fc MINOR: quic: skip sending if no frame to send in io-cb
Check on quic_conn_io_cb() if sending is required. This allows to skip
over Tx buffer allocation if not needed.

To implement this, we check if frame lists on current and next
encryption level are empty. We also need to check if there is no need to
send ACK, PROBE or CONNECTION_CLOSE. This has been isolated in a new
function qc_need_sending() which may be reuse in some other functions in
the future.
2022-08-09 16:03:49 +02:00
Amaury Denoyelle
654269c769 MINOR: quic: refactor datagram commit in Tx buffer
This is the final patch on quic-conn Tx refactor. Extend the function
which is used to write a datagram header to save at the same time
written buffer data. This makes sense as the two operations are used at
the same occasion when a pre-written datagram is comitted.
2022-08-09 16:00:30 +02:00
Amaury Denoyelle
5b68986d77 MINOR: quic: release Tx buffer on each send
Complete refactor of quic-conn Tx buffer. The buffer is now released
on every send operation completion. This should help to reduce memory
footprint as now Tx buffers are allocated and released on demand.

To simplify allocation/free of quic-conn Tx buffer, two static functions
are created named qc_txb_alloc() and qc_txb_release().
2022-08-09 16:00:02 +02:00
Amaury Denoyelle
f2476053f9 MINOR: quic: replace custom buf on Tx by default struct buffer
On first prototype version of QUIC, emission was multithreaded. To
support this, a custom thread-safe ring-buffer has been implemented with
qring/cbuf.

Now the thread model has been adjusted : a quic-conn is always used on
the same thread and emission is not multi-threaded. Thus, qring/cbuf
usage can be replace by a standard struct buffer.

The code has been simplified even more as for now buffer is always
drained after a prepare/send invocation. This is the case since a
datagram is always considered as sent even on sendto() error. BUG_ON
statements guard are here to ensure that this model is always valid.
Thus, code to handle data wrapping and consume too small contiguous
space with a 0-length datagram is removed.
2022-08-09 15:45:47 +02:00
Amaury Denoyelle
56c6154dba CLEANUP: mux-quic: remove loop on sending frames
qc_send_app_pkts() has now a while loop implemented which allows to send
all possible frames even if the send buffer is full between packet
prepare and send. This is present since commit :
  dc07751ed7
  MINOR: quic: Send packets as much as possible from qc_send_app_pkts()

This means we can remove code from the MUX which implement this at the
upper layer. This is useful to simplify qc_send_frames() function.

As mentionned commit is subject to backport, this commit should be
backported as well to 2.6.
2022-08-09 15:41:07 +02:00
Willy Tarreau
db3716b8db MINOR: debug/memstats: permit to pass the size to free()
Right now the free() call is not intercepted since all this is done
using macros and that would break a lot of stuff. Instead a __free()
macro was provided but never used. In addition it used to only report
a zero size, which is not very convenient.

With this patch comes a better solution. Instead it provides a new
will_free() macro that can be prepended before a call to free(). It
only keeps the counters up to date, and also supports being passed a
size. The pool_free_area() command now uses it, which finally allows
the stats to look correct:

  pool-os.h:38   MALLOC  size:   5802127832  calls:   3868044  size/call:   1500
  pool-os.h:47     FREE  size:   5800041576  calls:   3867444  size/call:   1499

The few other places directly calling free() could now be instrumented to
use this and to pass the correct sizeof() when known.
2022-08-09 09:11:27 +02:00
Willy Tarreau
4a426e2082 MINOR: debug/memstats: automatically determine first column size
The first column's width may vary a lot depending on outputs, and it's
annoying to have large empty columns on small names and mangled large
columns that are not yet large enough. In order to overcome this, this
patch adds a width field to the memstats applet's context, and this
width is calculated the first time the function is entered, by estimating
the width of all lines that will be dumped. This is simple enough and
does the job well. If in the future some filtering criteria are added,
it will still be possible to perform a single pass on everything
depending on the desired output format.
2022-08-09 08:51:08 +02:00
Willy Tarreau
17200dd1f3 MINOR: debug: also store the function name in struct mem_stats
The calling function name is now stored in the structure, and it's
reported when the "all" argument is passed. The first column is
significantly enlarged because some names are really wide :-(
2022-08-09 08:42:42 +02:00
Willy Tarreau
55c950baa9 MINOR: debug: store and report the pool's name in struct mem_stats
Let's add a generic "extra" pointer to the struct mem_stats to store
context-specific information. When tracing pool_alloc/pool_free, we
can now store a pointer to the pool, which allows to report the pool
name on an extra column. This significantly improves tracing
capabilities.

Example:

  proxy.c:1598      CALLOC   size:  28832  calls:  4     size/call:  7208
  dynbuf.c:55       P_FREE   size:  32768  calls:  2     size/call:  16384  buffer
  quic_tls.h:385    P_FREE   size:  34008  calls:  1417  size/call:  24     quic_tls_iv
  quic_tls.h:389    P_FREE   size:  34008  calls:  1417  size/call:  24     quic_tls_iv
  quic_tls.h:554    P_FREE   size:  34008  calls:  1417  size/call:  24     quic_tls_iv
  quic_tls.h:558    P_FREE   size:  34008  calls:  1417  size/call:  24     quic_tls_iv
  quic_tls.h:562    P_FREE   size:  34008  calls:  1417  size/call:  24     quic_tls_iv
  quic_tls.h:401    P_ALLOC  size:  34080  calls:  1420  size/call:  24     quic_tls_iv
  quic_tls.h:403    P_ALLOC  size:  34080  calls:  1420  size/call:  24     quic_tls_iv
  xprt_quic.c:4060  MALLOC   size:  45376  calls:  5672  size/call:  8
  quic_sock.c:328   P_ALLOC  size:  46440  calls:  215   size/call:  216    quic_dgram
2022-08-09 08:26:59 +02:00
Willy Tarreau
4bc37086b8 MINOR: debug: make the mem_stats section aligned to void*
Not specifying the alignment will let the linker choose it, and it turns
out that it will not necessarily be the same size as the one chosen for
struct mem_stats, as can be seen if any new fields are added there. Let's
enforce an alignment to void* both for the section and for the structure.
2022-08-09 08:09:24 +02:00
Frédéric Lécaille
ba19acd822 MINOR: quic: Replace pool_zalloc() by pool_malloc() for fake datagrams
These fake datagrams are only used by the low level I/O handler. They
are not provided to the "by connection" datagram handlers. This
is why they are not MT_LIST_APPEND()ed to the listner RX buffer list
(see &quic_dghdlrs[cid_tid].dgrams in quic_lstnr_dgram_dispatch().
Replace the call to pool_zalloc() to by the lighter call to pool_malloc()
and initialize only the ->buf and ->length members. This is safe because
only these fields are inspected by the low level I/O handler.
2022-08-08 21:10:58 +02:00
Frédéric Lécaille
ffde3168fc BUG/MEDIUM: quic: Missing AEAD TAG check after removing header protection
After removing the packet header protection, we can check the packet is long
enough to contain a 16 bytes length AEAD TAG (at this end of the packet).
This test was missing.

Must be backported to 2.6.
2022-08-08 18:41:16 +02:00
Frédéric Lécaille
adc7641536 MINOR: quic: Too much useless traces in qc_build_frms()
These traces about the available room into the packet currently built and
its payload length could be displayed for each STREAM frame, even for
those which have no chance to be embedded into a packet leading to
very traces to be displayed from a connection with a lot of stream.
This was revealed by traces provide by Tristan in GH #1808

May be backported to 2.6.
2022-08-08 16:18:55 +02:00
Frédéric Lécaille
99897d11d9 BUG/MEDIUM: quic: Wrong packet length check in qc_do_rm_hp()
When entering this function, we first check the packet length is not too short.
But this was done against the datagram lenght in place of the packet length.
This could lead to the header protection to be removed using data past
the end of the packet (without buffer overflow).

Use the packet length in place of the datagram length which is at <end>
address passed as parameter to this function. As the packet length
is already stored in ->len packet struct member, this <end> parameter is no
more useful.

Must be backported to 2.6.
2022-08-08 11:02:04 +02:00
Willy Tarreau
87e95d38a9 [RELEASE] Released version 2.7-dev3
Released version 2.7-dev3 with the following main changes :
    - BUILD: makefile: Fix install(1) handling for OpenBSD/NetBSD/Solaris/AIX
    - BUG/MEDIUM: tools: avoid calling dlsym() in static builds (try 2)
    - MINOR: resolvers: resolvers_destroy() deinit and free a resolver
    - BUG/MINOR: resolvers: shut off the warning for the default resolvers
    - BUG/MINOR: ssl: allow duplicate certificates in ca-file directories
    - BUG/MINOR: tools: fix statistical_prng_range()'s output range
    - BUG/MINOR: quic: do not send CONNECTION_CLOSE_APP in initial/handshake
    - BUILD: debug: Add braces to if statement calling only CHECK_IF()
    - BUG/MINOR: fd: Properly init the fd state in fd_insert()
    - BUG/MEDIUM: fd/threads: fix incorrect thread selection in wakeup broadcast
    - MINOR: init: load OpenSSL error strings
    - MINOR: ssl: enhance ca-file error emitting
    - BUG/MINOR: mworker/cli: relative pid prefix not validated anymore
    - BUG/MAJOR: mux_quic: fix invalid PROTOCOL_VIOLATION on POST data overlap
    - BUG/MEDIUM: mworker: proc_self incorrectly set crashes upon reload
    - BUILD: add detection for unsupported compiler models
    - BUG/MEDIUM: stconn: Only reset connect expiration when processing backend side
    - BUG/MINOR: backend: Fallback on RR algo if balance on source is impossible
    - BUG/MEDIUM: master: force the thread count earlier
    - BUG/MAJOR: poller: drop FD's tgid when masks don't match
    - DEBUG: fd: detect possibly invalid tgid in fd_insert()
    - BUG/MINOR: sockpair: wrong return value for fd_send_uxst()
    - MINOR: sockpair: move send_fd_uxst() error message in caller
    - Revert "BUG/MINOR: peers: set the proxy's name to the peers section name"
    - DEBUG: fd: split the fd check
    - MEDIUM: resolvers: continue startup if network is unavailable
    - BUG/MINOR: fd: always remove late updates when freeing fd_updt[]
    - MINOR: cli: emit a warning when _getsocks was used more than once
    - BUG/MINOR: mworker: PROC_O_LEAVING used but not updated
    - Revert "MINOR: cli: emit a warning when _getsocks was used more than once"
    - MINOR: cli: warning on _getsocks when socket were closed
    - BUG/MEDIUM: mux-quic: fix missing EOI flag to prevent streams leaks
    - MINOR: quic: Congestion control architecture refactoring
    - MEDIUM: quic: Cubic congestion control algorithm implementation
    - MINOR: quic: New "quic-cc-algo" bind keyword
    - BUG/MINOR: quic: loss time limit variable computed but not used
    - MINOR: quic: Stop looking for packet loss asap
    - BUG/MAJOR: quic: Useless resource intensive loop qc_ackrng_pkts()
    - MINOR: quic: Send packets as much as possible from qc_send_app_pkts()
    - BUG/MEDIUM: queue/threads: limit the number of entries dequeued at once
    - MAJOR: threads/plock: update the embedded library
    - MINOR: thread: provide an alternative to pthread's rwlock
    - DEBUG: tools: provide a tree dump function for ebmbtrees as well
    - MINOR: ebtree: add ebmb_lookup_shorter() to pursue lookups
    - BUG/MEDIUM: pattern: only visit equivalent nodes when skipping versions
    - BUG/MINOR: mux-quic: prevent crash if conn released during IO callback
    - CLEANUP: mux-quic: remove useless app_ops is_active callback
    - BUG/MINOR: mux-quic: do not free conn if attached streams
    - MINOR: mux-quic: save proxy instance into qcc
    - MINOR: mux-quic: use timeout server for backend conns
    - MEDIUM: mux-quic: adjust timeout refresh
    - MINOR: mux-quic: count in-progress requests
    - MEDIUM: mux-quic: implement http-keep-alive timeout
    - MINOR: peers: Add a warning about incompatible SSL config for the local peer
    - MINOR: peers: Use a dedicated reconnect timeout when stopping the local peer
    - BUG/MEDIUM: peers: limit reconnect attempts of the old process on reload
    - BUG/MINOR: peers: Use right channel flag to consider the peer as connected
    - BUG/MEDIUM: dns: Properly initialize new DNS session
    - BUG/MINOR: backend: Don't increment conn_retries counter too early
    - MINOR: server: Constify source server to copy its settings
    - REORG: server: Export srv_settings_cpy() function
    - BUG/MEDIUM: proxy: Perform a custom copy for default server settings
    - BUG/MINOR: quic: Missing in flight ack eliciting packet counter decrement
    - BUG/MEDIUM: quic: Floating point exception in cubic_root()
    - MINOR: h3: support HTTP request framing state
    - MINOR: mux-quic: refresh timeout on frame decoding
    - MINOR: mux-quic: refactor refresh timeout function
    - MEDIUM: mux-quic: implement http-request timeout
    - BUG/MINOR: quic: Avoid sending truncated datagrams
    - BUG/MINOR: ring/cli: fix a race condition between the writer and the reader
    - BUG/MEDIUM: sink: Set the sink ref for forwarders created during ring parsing
    - BUG/MINOR: sink: fix a race condition between the writer and the reader
    - BUG/MINOR: quic: do not reject datagrams matching minimum permitted size
    - MINOR: quic: Add two new stats counters for sendto() errors
    - BUG/MINOR: quic: Missing Initial packet dropping case
    - MINOR: quic: explicitely ignore sendto error
    - BUG/MINOR: quic: adjust errno handling on sendto
    - BUG/MEDIUM: quic: break out of the loop in quic_lstnr_dghdlr
    - MINOR: threads: report the number of thread groups in build options
    - MINOR: config: automatically preset MAX_THREADS based on MAX_TGROUPS
    - BUILD: SSL: allow to pass additional configure args to QUICTLS
    - CI: enable weekly "m32" builds on x86_64
    - CLEANUP: assorted typo fixes in the code and comments
    - BUG/MEDIUM: fix DH length when EC key is used
    - REGTESTS: ssl: adopt tests to OpenSSL-3.0.N
    - REGTESTS: ssl: adopt tests to OpenSSL-3.0.N
    - REGTESTS: ssl: fix grep invocation to use extended regex in ssl_generate_certificate.vtc
    - BUILD: cfgparse: always defined _GNU_SOURCE for sched.h and crypt.h
2022-08-07 17:28:59 +02:00
Willy Tarreau
2e64472d16 BUILD: cfgparse: always defined _GNU_SOURCE for sched.h and crypt.h
_GNU_SOURCE used to be defined only when USE_LIBCRYPT was set. It's also
needed for sched_setaffinity() to be exported. As a side effect, when
USE_LIBCRYPT is not set, a warning is emitted, as Ilya found and reported
in issue #1815. Let's just define _GNU_SOURCE regardless of USE_LIBCRYPT,
and also explicitly add sched.h, as right now it appears to be inherited
from one of the other includes.

This should be backported to 2.4.
2022-08-07 16:55:07 +02:00
Ilya Shipitsin
b6189bc268 REGTESTS: ssl: fix grep invocation to use extended regex in ssl_generate_certificate.vtc
in 2f2a2884b7 grep should have use regex flag -E, but flag
was lost by mistake
2022-08-06 23:24:13 +02:00
Ilya Shipitsin
2f2a2884b7 REGTESTS: ssl: adopt tests to OpenSSL-3.0.N
on Ubuntu-22.04 openssl-3.0.5 is shipped which has changed ec curve
description to "Server Temp Key: ECDH, secp384r1, 384 bits"
2022-08-06 17:46:10 +02:00
Ilya Shipitsin
0865160b93 REGTESTS: ssl: adopt tests to OpenSSL-3.0.N
on Ubuntu-22.04 openssl-3.0.5 is shipped which has changed ec curve
description to "Server Temp Key: ECDH, prime256v1, 256 bits"
2022-08-06 17:45:55 +02:00
Ilya Shipitsin
52f2ff5b93 BUG/MEDIUM: fix DH length when EC key is used
dh of length 1024 were chosen for EVP_PKEY_EC key type.
let us pick "default_dh_param" instead.

issue was found on Ubuntu 22.04 which is shipped with OpenSSL configured
with SECLEVEL=2 by default. such SECLEVEL value prohibits DH shorter than
2048:

OpenSSL error[0xa00018a] SSL_CTX_set0_tmp_dh_pkey: dh key too small

better strategy for chosing DH still may be considered though.
2022-08-06 17:45:40 +02:00
Ilya Shipitsin
3b64a28e15 CLEANUP: assorted typo fixes in the code and comments
This is 31st iteration of typo fixes
2022-08-06 17:12:51 +02:00
Ilya Shipitsin
4c785f0a1f CI: enable weekly "m32" builds on x86_64
this is build only workflow, catches potential "size_t" mismatches
--
v2 job name added, various markup changes
2022-08-06 17:10:16 +02:00
Ilya Shipitsin
3f59ac5ce2 BUILD: SSL: allow to pass additional configure args to QUICTLS
this allows to pass QUICTLS_EXTRA_ARGS to QUICTLS builds. if no
 additional arg is passed, behaviour is kept unchanged

--
v2 indentation fixed
2022-08-06 17:10:04 +02:00
Willy Tarreau
693688e734 MINOR: config: automatically preset MAX_THREADS based on MAX_TGROUPS
MAX_THREADS was not changed when setting MAX_TGROUPS, which still limits
some possibilities. Let's preset it to 4 * LONGBITS when MAX_TGROUPS is
larger than 1, or LONGBITS when it's set to 1. This means that the new
default value is 256 threads.

The rationale behind this is that the main use of thread groups is
mostly to address NUMA issues and that we don't necessarily need large
thread counts when using many groups, and 256 threads is already plenty
even on quite large systems.

For now it's important not to go too far because some internal structs
are arrays of MAX_THREADS entries, for example accept_queue_ring, which
is around 8kB per thread. Such structures will need to become dynamic
before defaulting to large thread counts (at 4096 threads max the
accept queues would require 32 MB RAM alone).
2022-08-06 16:51:20 +02:00
Willy Tarreau
c80bdb2da6 MINOR: threads: report the number of thread groups in build options
haproxy -vv shows the number of threads but didn't report the number
of groups, let's add it.
2022-08-06 16:45:26 +02:00
Willy Tarreau
f9d4a7dad3 BUG/MEDIUM: quic: break out of the loop in quic_lstnr_dghdlr
The function processes packets sent by other threads in the current
thread's queue. But if, for any reason, other threads write faster
than the current one processes, this can lead to a situation where
the function never returns.

It seems that it might be what's happening in issue #1808, though
unfortunately, this function is one of the rare without traces. But
the amount of calls to functions like qc_lstnr_pkt_rcv() on a single
thread seems to indicate this possibility.

Thanks to Tristan for his efforts in collecting extremely precious
traces!

This likely needs to be backported to 2.6.
2022-08-05 16:12:00 +02:00
Amaury Denoyelle
6715cbf97f BUG/MINOR: quic: adjust errno handling on sendto
qc_snd_buf returned a size_t which means that it was never negative
despite its documentation. Thus the caller who checked for this was
never informed of a sendto error.

Clean this by changing the return value of qc_snd_buf() to an integer.
A 0 is returned on success. Every other values are considered as an
error.

This commit should be backported up to 2.6. Note that to not cause
malfunctions, it must be backported after the previous patch :
  906b058954
  MINOR: quic: explicitely ignore sendto error
This is to ensure that a sendto error does not cause send to be
interrupted which may cause a stalled transfer without a proper retry
mechanism.

The impact of this bug seems null as caller explicitely ignores sendto
error. However this part of code seems to be subject to strange issues
and it may fix them in part. It may be of interest for github issue #1808.
2022-08-05 15:53:16 +02:00
Amaury Denoyelle
906b058954 MINOR: quic: explicitely ignore sendto error
qc_snd_buf() returns an error if sendto has failed. On standard
conditions, we should check for EAGAIN/EWOULDBLOCK errno and if so,
register the file-descriptor in the poller to retry the operation later.

However, quic_conn uses directly the listener fd which is shared for all
QUIC connections of this listener on several threads. Thus, it's
complicated to implement fd supversion via the poller : there is no
mechanism to easily wakeup quic_conn or MUX after a sendto failure.

A quick and simple solution for the moment is to considered a datagram
as properly emitted even on sendto error. In the end, this will trigger
the quic_conn retransmission timer as data will be considered lost on
the network and the send operation will be retried. This solution will
be replaced when fd management for quic_conn is reworked.

In fact, this quick hack was already in use in the current code, albeit
not voluntarily. This is due to a bug caused by an API mismatch on the
return type of qc_snd_buf() which never emits a negative error code
despite its documentation. Thus, all its invocation were considered as a
success. If this bug was fixed, the sending would would have been
interrupted by a break which could cause the transfer to freeze.

qc_snd_buf() invocation is clean up : the break statement is removed.
Send operation is now always explicitely conducted entirely even on
error and buffer data is purged.

A simple optimization has been added to skip over sendto when looping
over several datagrams at the first sendto error. However, to properly
function, it requires a fix on the return type of qc_snd_buf() which is
provided in another patch.

As the behavior before and after this patch seems identical, it is not
labelled as a BUG. However, it should be backported for cleaning
purpose. It may also have an impact on github issue #1808.
2022-08-05 15:45:25 +02:00
Frédéric Lécaille
e7df68a219 BUG/MINOR: quic: Missing Initial packet dropping case
An Initial packet shorter than 1200 bytes must be dropped. The test was there
without the "goto drop"!

Must be backported to 2.6
2022-08-05 15:27:14 +02:00
Frédéric Lécaille
8ecb7363b5 MINOR: quic: Add two new stats counters for sendto() errors
Add "quic_socket_full" new stats counter for sendto() errors with EAGAIN as errno.
and "quic_sendto_err" counter for any other error.
2022-08-05 15:27:14 +02:00
Willy Tarreau
af5138fd07 BUG/MINOR: quic: do not reject datagrams matching minimum permitted size
The dgram length check in quic_get_dgram_dcid() rejects datagrams
matching exactly the minimum allowed length, which doesn't seem
correct. I doubt any useful packet would be that small but better
fix this to avoid confusing debugging sessions in the future.

This might be backported to 2.6.
2022-08-05 10:31:29 +02:00
Willy Tarreau
53bfab080c BUG/MINOR: sink: fix a race condition between the writer and the reader
This is the same issue as just fixed in b8e0fb97f ("BUG/MINOR: ring/cli:
fix a race condition between the writer and the reader") but this time
for sinks. They're also sucking the ring and present the same race at
high write loads.

This must be backported to 2.2 as well. See comments in the aforementioned
commit for backport hints if needed.
2022-08-04 17:21:16 +02:00
Christopher Faulet
96417f392d BUG/MEDIUM: sink: Set the sink ref for forwarders created during ring parsing
A reference to the sink was added in every forwarder by the commit 2ae25ea24
("MINOR: sink: Add a ref to sink in the sink_forward_target structure"). But
this commit is incomplete. It is not performed for the forwarders created
during a ring parsing.

This patch must be backported to 2.6.
2022-08-04 17:10:28 +02:00
Willy Tarreau
b8e0fb97f3 BUG/MINOR: ring/cli: fix a race condition between the writer and the reader
The ring's CLI reader unlocks the read side of a ring and relocks it for
writing only if it needs to re-subscribe. But during this time, the writer
might have pushed data, see nobody subscribed hence woken nobody, while
the reader would have left marking that the applet had no more data. This
results in a dump that will not make any forward progress: the ring is
clogged by this reader which believes there's no data and the writer
will never wake it up.

The right approach consists in verifying after re-attaching if the writer
had made any progress in between, and to report that another call is
needed. Note that a jump back to the beginning would also work but here
we provide better fairness between readers this way.

This needs to be backported to 2.2. The applet API needed to signal the
availability of new data changed a few times since then.
2022-08-04 17:00:21 +02:00
Frédéric Lécaille
48bb875908 BUG/MINOR: quic: Avoid sending truncated datagrams
There is a remaining loop in this ugly qc_snd_buf() function which could
lead haproxy to send truncated UDP datagrams. For now on, we send
a complete UDP datagram or nothing!

Must be backported to 2.6.
2022-08-03 21:09:04 +02:00
Amaury Denoyelle
30e260e2e6 MEDIUM: mux-quic: implement http-request timeout
Implement http-request timeout for QUIC MUX. It is used when the
connection is opened and is triggered if no HTTP request is received in
time. By HTTP request we mean at least a QUIC stream with a full header
section. Then qcs instance is attached to a sedesc and upper layer is
then responsible to wait for the rest of the request.

This timeout is also used when new QUIC streams are opened during the
connection lifetime to wait for full HTTP request on them. As it's
possible to demux multiple streams in parallel with QUIC, each waiting
stream is registered in a list <opening_list> stored in qcc with <start>
as timestamp in qcs for the stream opening. Once a qcs is attached to a
sedesc, it is removed from <opening_list>. When refreshing MUX timeout,
if <opening_list> is not empty, the first waiting stream is used to set
MUX timeout.

This is efficient as streams are stored in the list in their creation
order so CPU usage is minimal. Also, the size of the list is
automatically restricted by flow control limitation so it should not
grow too much.

Streams are insert in <opening_list> by application protocol layer. This
is because only application protocol can differentiate streams for HTTP
messaging from internal usage. A function qcs_wait_http_req() has been
added to register a request stream by app layer. QUIC MUX can then
remove it from the list in qc_attach_sc().

As a side-note, it was necessary to implement attach qcc_app_ops
callback on hq-interop module to be able to insert a stream in waiting
list. Without this, a BUG_ON statement would be triggered when trying to
remove the stream on sedesc attach. This is to ensure that every
requests streams are registered for http-request timeout.

MUX timeout is explicitely refreshed on MAX_STREAM_DATA and STOP_SENDING
frame parsing to schedule http-request timeout if a new stream has been
instantiated. It was already done on STREAM parsing due to a previous
patch.
2022-08-03 15:04:18 +02:00
Amaury Denoyelle
6ec9837fca MINOR: mux-quic: refactor refresh timeout function
Try to reorganize qcc_refresh_timeout() to improve its readability. The
main objective is to reduce the indentation level and if sequences by
using goto statement to the end of the function. Also, backend and
frontend code path should be more explicit with this new version.
2022-08-03 15:04:18 +02:00
Amaury Denoyelle
418ba21461 MINOR: mux-quic: refresh timeout on frame decoding
Refresh the MUX connection timeout in frame parsing functions. This is
necessary as these Rx operation are completed directly from the
quic-conn layer outside of MUX I/O callback. Thus, the timeout should be
refreshed on this occasion.

Note that however on STREAM parsing refresh is only conducted when
receiving the current consecutive data offset.

Timeouts related function have been moved up in the source file to be
able to use them in qcc_decode_qcs().

This commit will be useful for http-request timeout. Indeed, a new
stream may be opened during qcc_decode_qcs() which should trigger this
timeout until a full header section is received and qcs instance is
attached to sedesc.
2022-08-03 15:04:18 +02:00
Amaury Denoyelle
8d818c6eab MINOR: h3: support HTTP request framing state
Store the current step of HTTP message in h3s stream. This reports if we
are in the parsing of headers, content or trailers section. A new enum
h3s_st_req is defined for this.

This field is stored in h3s struct but only used for request stream. It
is left undefined for other streams (control or QPACK streams).

h3_is_frame_valid() has been extended to take into account this state
information. A connection error H3_FRAME_UNEXPECTED is reported if an
invalid frame according to the current state is received; for example a
DATA frame at the beginning of a stream.
2022-08-03 15:04:18 +02:00
Frédéric Lécaille
2c77a5eb8e BUG/MEDIUM: quic: Floating point exception in cubic_root()
It is illegal to call my_flsl() with 0 as parameter value. It is a UB.
This leaded cubic_root() to divide values by 0 at this line:

  x = 2 * x + (uint32_t)(val / ((uint64_t)x * (uint64_t)(x - 1)));

Thank you to Tristan971 for having reported this issue in GH #1808
and Willy for having spotted the root cause of this bug.

Must follow any cubic for QUIC backport (2.6).
2022-08-03 14:27:20 +02:00
Frédéric Lécaille
8ddde4f05e BUG/MINOR: quic: Missing in flight ack eliciting packet counter decrement
The decrement was missing in quic_pktns_tx_pkts_release() called each time a
packet number space is discarded. This is not sure this bug could have an impact
during handshakes. This counter is used to cancel the timer used both for packet
detection and PTO, setting its value to null. So there could be retransmissions
or probing which could be triggered for nothing.

Must be backported to 2.6.
2022-08-03 12:59:59 +02:00
Christopher Faulet
6bb86539db BUG/MEDIUM: proxy: Perform a custom copy for default server settings
When a proxy is initialized with the settings of the default proxy, instead
of doing a raw copy of the default server settings, a custom copy is now
performed by calling srv_settings_copy(). This way, all settings will be
really duplicated. Without this deep copy, some pointers are shared between
several servers, leading to UAF, double-free or such bugs.

This patch relies on following commits:

  * b32cb9b51 REORG: server: Export srv_settings_cpy() function
  * 0b365e3cb MINOR: server: Constify source server to copy its settings

This patch should fix the issue #1804. It must be backported as far as 2.0.
2022-08-03 11:44:34 +02:00