Commit Graph

21170 Commits

Author SHA1 Message Date
Willy Tarreau
a63e016d27 MINOR: activity: report profiling duration and age in "show profiling"
Seeing counters in "show profiling" is not always very helpful without
an indication of how long the analysis lasted nor if it's still active
or not. Let's add a pair of start/stop timers for tasks and memory so
that we can now indicate how long the measurements lasted and when they
ended (or 0 if still running).

Note that for tasks profiling set to "auto", the measurement is considered
enabled since it can automatically switch on and off on a per-thread
basis.
2023-11-14 11:46:37 +01:00
Christopher Faulet
af7db3a43c REGTESTS: http: Improve script testing abortonclose option
We now take care to properly handle the abortonclose close option if it is
set on the backend and be sure we ignore it when it is set on the frontend
(inherited from the defaults section).
2023-11-14 11:01:51 +01:00
Christopher Faulet
ec3ea6f698 MINOR: stconn: Use SC to detect frontend connections in sc_conn_recv()
In sc_conn_recv(), instead of using the connection to know we are on the
frontend side, we now use the SC flags. It changes nothing but it is
cleaner.
2023-11-14 11:01:51 +01:00
Christopher Faulet
5ff7d22767 BUG/MEDIUM: stream: Properly handle abortonclose when set on backend only
Since the 2.2 and the commit dedd30610 ("MEDIUM: h1: Don't wake the H1 tasklet
if we got the whole request."), we avoid to subscribe for reads if the H1
message is fully received. However, this broke the abortonclose option. To fix
the issue, a CO_RFL flag was added to instruct the mux it should still wait for
read events to properly handle read0. Only the H1 mux was concerned.

But since then, most of time, the option is only handled if it is set on the
frontend proxy because the request is fully received before selecting the
backend. If the backend is selected before the end of the request there is no
issue. But otherwise, because the backend is not known yet, we are unable to
properly handle the option and we miss to subscribe for reads.

Of course the option cannot be set on a frontend proxy. So concretly it means
the option is properly handled if it is enabled in the defaults section (if
common to frontend and backend) or a listen proxy, but it is ignored if it is
set on backend only.

Thanks to previous patches, we can now instruct the mux it should subscribe for
reads if not already done. We use this mechanism in process_stream() when the
connection is set up, ie when backend SC is set to SC_ST_REQ state.

This patch relies on following patches:
  * MINOR: connection: Add a CTL flag to notify mux it should wait for reads again
  * MEDIUM: mux-h1: Handle MUX_SUBS_RECV flag in h1_ctl() and susbscribe for reads

This patch should be the issue #2344. All the series must be backported as far
as 2.2.
2023-11-14 11:01:51 +01:00
Christopher Faulet
450ff71c95 MEDIUM: mux-h1: Handle MUX_SUBS_RECV flag in h1_ctl() and susbscribe for reads
The H1 mux now handle MUX_SUBS_RECV flag in h1_ctl(). If it is not already
subscribed for reads, it does so. This patch will be mandatory to properly
handle abortonclose option.
2023-11-14 11:01:51 +01:00
Christopher Faulet
e5cffa8ace MINOR: connection: Add a CTL flag to notify mux it should wait for reads again
MUX_SUBS_RECV ctl flag is added to instruct the mux it should wait for read
events. This flag will be pretty useful to handle abortonclose option.
2023-11-14 11:01:51 +01:00
Christopher Faulet
9327e7efa7 BUG/MINOR: stconn: Handle abortonclose if backend connection was already set up
abortonclose option is a backend option, it should not be handle on frontend
side. Of course a frontend can also be a backend but the option should not
be handled too early because it is not necessarily the selected backend
(think about a listen proxy routing requests to another backend).

It is especially an issue when the abortonclose option is enabled in the
defaults section and disabled by the selected backend. Because in this case,
the option may still be enabled while it should not.

Thus, now we wait the backend connection was set up to handle the option. To
do so, we check the backend SC state. The option is ignored if it is in
ST_CS_INI state. For all other states, it means the backend was already
selected.

This patch could be backported as far as 2.2.
2023-11-14 11:01:51 +01:00
Willy Tarreau
6a4591c3d0 BUG/MEDIUM: connection: report connection errors even when no mux is installed
An annoying issue was met when testing the reverse-http mechanism, by
which failed connection attempts would apparently not be attempted again
when there was no connect timeout. It turned out to be more generalized
than the rhttp system, and actually affects all outgoing connections
relying on NPN or ALPN to choose the mux, on which no mux is installed
and for which the subscriber (ssl_sock) must be notified instead.

The problem appeared during 2.2-dev1 development. First, commit
062df2c23 ("MEDIUM: backend: move the connection finalization step to
back_handle_st_con()") broke the error reporting by testing CO_FL_ERROR
only under CO_FL_CONNECTED. While it still worked OK for cases where a
mux was present, it did not for this specific situation because no
single error path would be considered when no mux was present. Changing
the CO_FL_CONNECTED test to also include CO_FL_ERROR did work, until
a few commits later with 477902bd2 ("MEDIUM: connections: Get ride of
the xprt_done callback.") which removed the xprt_done callback that was
used to indicate success or failure of the transport layer setup, since,
as the commit explains, we can report this via the mux. What this last
commit says is true, except when there is no mux.

For this, however, the sock_conn_iocb() function (formerly conn_fd_handler)
is called for such errors, evaluates a number of conditions, none of which
is matched in this error condition case, since sock_conn_check() instantly
reports an error causing a jump to the leave label. There, the mux is
notified if installed, and the function returns. In other error condition
cases, readiness and activity are checked for both sides, the tasklets
woken up and the corresponding subscriber flags removed. This means that
a sane (and safe) approach would consist in just notifying the subscriber
in case of error, if such a subscriber still exists: if still there, it
means the event hasn't been caught earlier, then it's the right moment
to report it. And since this is done after conn_notify_mux(), it still
leaves all control to the mux once it's installed.

This commit should be progressively backported as far as 2.2 since it's
where the problem was introduced. It's important to clearly check the
error path in each function to make sure the fix still does what it's
supposed to.
2023-11-14 08:49:23 +01:00
Frédéric Lécaille
3741e4bf90 BUG/MINOR: quic: maximum window limits do not match the doc
This bug arrived with this commit:
     MINOR: quic: Add a max window parameter to congestion control algorithms

The documentation was been modified with missing/wrong modifications in the code part.
The 'g' suffix must be accepted to parse value in gigabytes. And exctly 4g is
also accepted.

No need to backport.
2023-11-13 19:56:28 +01:00
Frédéric Lécaille
8df7018736 DOC: quic: Maximum congestion control window configuration
Document the optional parameter which may be supplied after the congestion
control algorithm name to set the maximum congestion control window.
2023-11-13 18:17:43 +01:00
Frédéric Lécaille
d9bf1b6c41 DOC: quic: Wrong syntax for "quic-cc-algo" keyword.
As the argument to "quic-cc-algo" is mandatory, brackets must be used here
in the documentation.

Must be backported as far as 2.6.
2023-11-13 18:14:16 +01:00
Frédéric Lécaille
9021e8935e MINOR: quic: Maximum congestion control window for each algo
Make all the congestion support the maximum congestion control window
set by configuration. There is nothing special to explain. For each
each algo, each time the window is incremented it is also bounded.
2023-11-13 17:53:18 +01:00
Frédéric Lécaille
028a55a1d0 MINOR: quic: Add a max window parameter to congestion control algorithms
Add a new ->max_cwnd member to bind_conf struct to store the maximum
congestion control window value for each QUIC binding.
Modify the "quic-cc-algo" keyword parsing to add an optional parameter
to its value: the maximum congestion window value between parentheses
as follows:

      ex: quic-cc-algo cubic(10m)

This value must be bounded, greater than 10k and smaller than 1g.
2023-11-13 17:53:18 +01:00
Frédéric Lécaille
840af0928b BUG/MEDIUM: quic: Non initialized CRYPTO data stream deferencing
This bug arrived with this commit:
   BUG/MINOR: quic: Useless use of non-contiguous buffer for in order CRYPTO data

Before this commit qc->cstream was tested before entering qc_treat_rx_crypto_frms().
This patch restablishes this behavior. Furthermore, it simplyfies
qc_ssl_provide_all_quic_data() which is a little bit ugly: the CRYPTO data
frame may be freed asap in the list_for_each_entry_safe() block after
having store its data pointer and length in local variables.
Also interrupt the CRYPTO data process as soon as qc_ssl_provide_quic_data()
or qc_treat_rx_crypto_frms() fail.

No need to be backported.
2023-11-13 16:00:25 +01:00
William Lallemand
59b313832a REGTESTS: startup: -conf-OK requires -V with current VTest
Current version of VTest tests the output of "haproxy -c" instead of the
return code. Since we don't output anymore when the configuration is
valid, this broke the test. (a06f621).

This fixes the issue by adding the -V when doing a -conf-OK. But this
must fixed in VTest.
2023-11-13 14:57:26 +01:00
Christopher Faulet
cb560bf3d7 DOC: config: Fix name for tune.disable-zero-copy-forwarding global param
"disable-" prefix was missing. This param was correctly named in the list of
supported keywords in the global section, but not in the keyword
description.

No backport needed.
2023-11-13 14:31:14 +01:00
Amaury Denoyelle
954b5b756a BUG/MEDIUM: quic: fix FD for quic_cc_conn
Since following commit, quic_conn closes its owned socket before
transition to quic_cc_conn for closing state. This allows to save FDs as
quic_cc_conn could use the listener socket for their I/O.

  commit 150c0da889
  MEDIUM: quic: release conn socket before using quic_cc_conn

This patch is incomplete as it removes initialization of <fd> member for
quic_cc_conn. Thus, if sending is done on closing state, <fd> value is
undefined which in most cases will result in a crash. Fix this by simply
initializing <fd> member with qc_init_fd() in qc_new_cc_conn().

This bug should fix recent issue from #2095. Thanks to Tristan for its
reporting and then testing of this patch.

No need to backport.
2023-11-13 11:55:07 +01:00
Amaury Denoyelle
78d244e9f7 BUG/MINOR: quic: fix decrement of half_open counter on qc alloc failure
Half open counter is used to comptabilize QUIC connections waiting for
address validation. It was recently reworked to adjust its scope. With
each decrement operation, a BUG_ON() was added to ensure the counter
never wraps.

This BUG_ON() could be triggered if an allocation fails for one of
quic_conn members in qc_new_conn(). This is because half open counter is
incremented at the end of qc_new_conn(). However, in case of alloc
failure, quic_conn_release() is called immediately to ensure the counter
is decremented if a connection is freed before peer address has been
validated.

To fix this, increment half open counter early in qc_new_conn() prior to
every quic_conn members allocations.

This issue was reproduced using -dMfail argument.

This issue has been introduced by
  commit 278808915b
  MINOR: quic: reduce half open counters scope

No need to backport.
2023-11-13 11:16:41 +01:00
Amaury Denoyelle
92da3accfd BUG/MINOR: quic: fix crash on qc_new_conn alloc failure
A new counter was recently introduced to comptabilize the current number
of active QUIC handshakes. This counter is stored on the listener
instance.

This counter is incremented at the beginning of qc_new_conn() to check
if limit is not reached prior to quic_conn allocation. If quic_conn or
one of its inner member allocation fails, special care is taken to
decrement the counter as the connection instance is released. However,
it relies on <l> variable which is initialized too late to cover
pool_head_quic_conn allocation failure.

To fix this, simply initialize <l> at the beginning of qc_new_conn().

This issue was reproduced using -dMfail argument.

This issue was introduced by the following commit
  commit 3df6a60113
  MEDIUM: quic: limit handshake per listener

No need to backport.
2023-11-13 11:16:41 +01:00
Aurelien DARRAGON
76acde9107 BUG/MINOR: log: keep the ref in dup_logger()
This bug was introduced with 969e212 ("MINOR: log: add dup_logsrv() helper
function")

When duplicating an existing log entry, we must take care to inherit from
its original ->ref if it is set, because not doing so would make 28ac0999
("MINOR: log: Keep the ref when a log server is copied to avoid duplicate entries")
ineffective given that global log directives will lose their original
reference when duplicated resursively (at least twice), which is what
happens when global log directives are first inherited to defaults which
are then inherited to a regular proxy at the end of the chain.

This can be easily reproduced using the following configuration:

   |global
   |  log stdout format raw local0
   |
   |defaults
   |  log global
   |
   |frontend test
   |  log global
   |  ...

Logs from "test" proxy will be duplicated because test incorrectly
inherited from global "log" directives twice, which 28ac0999 would
normally detect and prevent.

No backport needed unless 969e212 gets backported.
2023-11-13 11:06:05 +01:00
Christopher Faulet
33a1fc883a BUG/MINOR: sample: Fix bytes converter if offset is bigger than sample length
When the bytes converter was improved to be able to use variables (915e48675
["MEDIUM: sample: Enhances converter "bytes" to take variable names as
arguments"]), the behavior of the sample slightly change. A failure is
reported if the given offset is bigger than the sample length. Before, a
empty binary sample was returned.

This patch fixes the converter to restore the original behavior. The
function was also refactored to properly handle failures by removing
SMP_F_MAY_CHANGE flag. Because the converter now handles variables, the
conversion to an integer may fail. In this case SMP_F_MAY_CHANGE flag must
be removed to be sure the caller will not retry.

This patch should fix the issue #2335. No backport needed except if commit
above is backported.
2023-11-13 11:06:05 +01:00
William Lallemand
a06f6212c9 MEDIUM: startup: 'haproxy -c' is quiet when valid
MODE_CHECK does not output "Configuration file is valid" by default
anymore. To display this message the -V option must be used with -c.

However the warning and errors are still output by default if they
exist.

This allows to clean the output of the systemd unit file with is doing a
-c.
2023-11-13 09:59:34 +01:00
Willy Tarreau
cf07cb96be BUG/MEDIUM: proxy: always initialize the default settings after init
The proxy's initialization is rather odd. First, init_new_proxy() is
called to zero all the lists and certain values, except those that can
come from defaults, which are initialized by proxy_preset_defaults().
The default server settings are also only set there.

This results in these settings not to be set for a number of internal
proxies that do not explicitly call proxy_preset_defaults() after
allocation, such as sink and log forwarders.

This was revealed by last commit 79aa63823 ("MINOR: server: always
initialize pp_tlvs for default servers") which crashes in log parsers
when applied to certain proxies which did not initialize their default
servers.

In theory this should be backported, however it would be desirable to
wait a bit before backporting it, in case certain parts would rely on
these elements not being initialized.
2023-11-13 09:17:05 +01:00
Willy Tarreau
79aa638238 MINOR: server: always initialize pp_tlvs for default servers
In commit 6f4bfed3a ("MINOR: server: Add parser support for
set-proxy-v2-tlv-fmt") a suspicious check for a NULL srv_tlv was placed
in the list_for_each_entry(), that should not be needed. In practice,
it's caused by the list head not being initialized, hence the first
element is NULL, as shown by Alexander's reproducer below which crashes
if the test in the loop is removed:

  backend dummy
    default-server send-proxy-v2 set-proxy-v2-tlv-fmt(0xE1) %[fc_pp_tlv(0xE1)]
    server dummy_server 127.0.0.1:2319

The right place to initialize this field is proxy_preset_defaults().
We'd really need a function to initialize a server :-/

The check in the loop was removed. No backport is needed.
2023-11-13 08:53:28 +01:00
Frdric Lcaille
dfda884633 BUG/MINOR: quic: Useless use of non-contiguous buffer for in order CRYPTO data
This issue could be reproduced with a TLS client certificate verificatio to
generate enough CRYPTO data between the client and haproxy and with dev/udp/udp-perturb
as network perturbator. Haproxy could crash thanks to a BUG_ON() call as soon as
in disorder data were bufferized into a non-contiguous buffer.

There is no need to pass a non NULL non-contiguous to qc_ssl_provide_quic_data()
from qc_ssl_provide_all_quic_data() which handles in order CRYPTO data which
have not been bufferized. If not, the first call to qc_ssl_provide_quic_data()
to process the first block of in order data leads the non-contiguous buffer
head to be advanced to a wrong offset, by <len> bytes which is the length of the
in order CRYPTO frame. This is detected by a BUG_ON() as follows:

FATAL: bug condition "ncb_ret != NCB_RET_OK" matched at src/quic_ssl.c:620
  call trace(11):
  | 0x5631cc41f3cc [0f 0b 8b 05 d4 df 48 00]: qc_ssl_provide_quic_data+0xca7/0xd78
  | 0x5631cc41f6b2 [89 45 bc 48 8b 45 b0 48]: qc_ssl_provide_all_quic_data+0x215/0x576
  | 0x5631cc3ce862 [48 8b 45 b0 8b 40 04 25]: quic_conn_io_cb+0x19a/0x8c2
  | 0x5631cc67f092 [e9 1b 02 00 00 83 45 e4]: run_tasks_from_lists+0x498/0x741
  | 0x5631cc67fb51 [89 c2 8b 45 e0 29 d0 89]: process_runnable_tasks+0x816/0x879
  | 0x5631cc625305 [8b 05 bd 0c 2d 00 83 f8]: run_poll_loop+0x8b/0x4bc
  | 0x5631cc6259c0 [48 8b 05 b9 ac 29 00 48]: main-0x2c6
  | 0x7fa6c34a2ea7 [64 48 89 04 25 30 06 00]: libpthread:+0x7ea7
  | 0x7fa6c33c2a2f [48 89 c7 b8 3c 00 00 00]: libc:clone+0x3f/0x5a

Thank you to @Tristan971 for having reported this issue in GH #2095.

No need to backport.
2023-11-10 18:16:14 +01:00
Aurelien DARRAGON
078ebde870 CLEANUP: sink: useless leftover in sink_add_srv()
Removing a useless leftover which has been introduced with 31e8a003a5
("MINOR: sink: function to add new sink servers")
2023-11-10 17:49:57 +01:00
Aurelien DARRAGON
2694621151 CLEANUP: sink: bad indent in sink_new_from_logger()
Fixing bad indent in sink_new_from_logger() which was recently introduced
2023-11-10 17:49:57 +01:00
Aurelien DARRAGON
d710dfbacc BUG/MINOR: sink: don't learn srv port from srv addr
Since 04276f3d ("MEDIUM: server: split the address and the port into two
different fields") we should not use srv->addr to store server's port
and rely on srv->svc_port instead.

For sink servers, we correctly set >svc_port upon server creation but
we didn't use it when initializing address for the connection.

As a result, FQDN resolution will not work properly with sink servers.
Hopefully, this used to work by accident because sink servers were
resolved using the PA_O_RESOLVE flag in str2sa_range(), which made the
srv->addr contain the port in addition to the address.

But this will fail to work when FQDN resolution is postponed because only
->svc_port will contain the proper server port upon resolution.

For instance, FQDN resolution with servers from log backends (which are
resolved as regular servers, that is, without the PA_O_RESOLVE) will fail
to work because of this.

This may be backported as far as 2.2 even though the bug didn't have
noticeable effects for versions below 2.9

[In 2.2, sink_forward_session_init() didn't exist it should be applied in
 sink_forward_session_create()]
2023-11-10 17:49:57 +01:00
Aurelien DARRAGON
64e0b63442 BUG/MEDIUM: server: invalid address (post)parsing checks
This bug was introduced with 29b76ca ("BUG/MEDIUM: server/log: "mode log"
after server keyword causes crash ")

Indeed, we cannot safely rely on addr_proto being set when str2sa_range()
returns in parse_server() (even if SRV_PARSE_PARSE_ADDR is set), because
proto lookup might be bypassed when FQDN addresses are involved.

Unfortunately, the above patch wrongly assumed that proto would always
be set when SRV_PARSE_PARSE_ADDR was passed to parse_server() (so when
str2sa_range() was called), resulting in invalid postparsing checks being
performed, which could as well lead to crashes with log backends
("mode log" set) because some postparsing init was skipped as a result of
proto not being set and this wasn't expected later in the init code.

To fix this, we now make use of the previous patch to perform server's
address compatibility checks on hints that are always set when
str2sa_range() succesfully returns.

For log backend, we're also adding a complementary test to check if the
address family is of expected type, else we report an error, plus we're
moving the postinit logic in log api since _srv_check_proxy_mode() is
only meant to check proxy mode compatibility and we were abusing it.

This patch depends on:
 - "MINOR: tools: make str2sa_range() directly return type hints"

No backport required unless 29b76ca gets backported.
2023-11-10 17:49:57 +01:00
Aurelien DARRAGON
12582eb8e5 MINOR: tools: make str2sa_range() directly return type hints
str2sa_range() already allows the caller to provide <proto> in order to
get a pointer on the protocol matching with the string input thanks to
5fc9328a ("MINOR: tools: make str2sa_range() directly return the protocol")

However, as stated into the commit message, there is a trick:
   "we can fail to return a protocol in case the caller
    accepts an fqdn for use later. This is what servers do and in this
    case it is valid to return no protocol"

In this case, we're unable to return protocol because the protocol lookup
depends on both the [proto type + xprt type] and the [family type] to be
known.

While family type might not be directly resolved when fqdn is involved
(because family type might be discovered using DNS queries), proto type
and xprt type are already known. As such, the caller might be interested
in knowing those address related hints even if the address family type is
not yet resolved and thus the matching protocol cannot be looked up.

Thus in this patch we add the optional net_addr_type (custom type)
argument to str2sa_range to enable the caller to check the protocol type
and transport type when the function succeeds.
2023-11-10 17:49:57 +01:00
Christopher Faulet
ebf90ca550 BUG/MEDIUM: applet: Remove appctx from buffer wait list on release
For now, the appctx is removed from the buffer wait list when it is
freed. However, when it is released, it is not necessarily freed
immediately. But it is detached from the SC. If it is still registered in
the buffer wait list, it could then be woken up to get a buffer. At this
stage it is totally unexpected, especially because we must access the SC.

The fix is obvious, the appctx must be removed from the buffer wait list on
release.

Note this bug exists because the appctx was moved at the mux level.

This patch must be backported as far as 2.6.
2023-11-10 17:49:57 +01:00
Willy Tarreau
9530e7dcd3 DOC: config: use the word 'backend' instead of 'proxy' in 'track' description
User @nwehrman reported in issue #2328 that the used of "proxy" instead
of "backend" in the argument of the "track" server keyword is confusing.
Admittedly, all other places in the doc use "backend/server" instead of
"proxy/server", so let's update it for the sake of consistency.
2023-11-10 16:29:02 +01:00
Amaury Denoyelle
150c0da889 MEDIUM: quic: release conn socket before using quic_cc_conn
After emission/reception of a CONNECTION_CLOSE, a connection enters the
CLOSING state. In this state, only minimal exchanges occurs as only the
packets which containted the CONNECTION_CLOSE frame can be reemitted. In
conformance with the RFC, most resources are released and quic_conn
instance is converted to the lighter quic_cc_conn.

Push further this optimization by closing quic_conn socket FD before
switching to a quic_cc_conn. This means that quic_cc_conn will rely on
listener socket for its send/recv operation. This should not impact
performance as as stated input/output are minimal on this state.

This patch should improve FD consumption as prior to this a socket FD
was kept during the closing delay which could cause maxsock to be
reached for other connections.

Note that fd member is kept in QUIC_CONN_COMMON and not removed from
quic_cc_conn. This is because quic_cc_conn relies on qc_snd_buf() which
access this field.

As a side-effect to this change, jobs accounting for quic_conn is also
updated. quic_cc_conn instances are now not counted as jobs. Indeed, the
main objective of jobs is to prevent haproxy process to be stopped with
data truncation. However, this relies on the connection to uses its
owned socket as the listener socket is shut down inconditionaly on
shutdown.

A consequence of the jobs handling change is that haproxy process will
be closed if only quic_cc_conn instances are present, thus preventing to
respect the closing state. In case of a reload, if a client missed a
CONNECTION_CLOSE frame just before process shutdown, it will probably
received a Stateless Reset on sending retry.

This change is considered safe as, for now, haproxy only emits
CONNECTION_CLOSE on error conditions (such as protocol violation or
timeout). It is considered as expected to suffer from data truncation
from this. However, if connection closing is reused by haproxy to
implement clean shutdown, it should be necessary to delay
CONNECTION_CLOSE frame emission to ensure no data truncation happens
here.
2023-11-10 15:27:45 +01:00
Amaury Denoyelle
f549eb2b34 MEDIUM: quic: respect closing state even on soft-stop
Prior to this patch, a special condition was set when idle timer was
rearmed for closing connections during haproxy process stopping. In this
case, the timeout was ditched and the idle task woken up immediatly.

The objective was to release quickly closing connections to not prevent
the process stopping to be too long. However, it is not conform with RFC
9000 recommandations and may cause some clients to miss a
CONNECTION_CLOSE in case of a packet loss.

A recent fix was set to use a shorter timeout for closing state. Now a
connection should only be left in this state for one second or less.
This reduces greatly the importance of stopping special condition. Thus,
this patch removes it completely.
2023-11-10 15:26:03 +01:00
Amaury Denoyelle
75e36c57f0 BUG/MINOR: quic: remove dead code in error path
In quic_rx_pkt_retrieve_conn(), err label is now only used if qc is
NULL. Thus, condition on qc can be removed.

No need to backport.

This issue was reported by coverity on github.
This should fix issue #2338.
2023-11-10 15:26:03 +01:00
Willy Tarreau
0a7ab7067f OPTIM: mux-h2: don't allocate more buffers per connections than streams
When an H2 mux works with a slow downstream connection and without the
mux-mux mode, it is possible that a single stream will allocate all 32
buffers in the connection. This is not desirable at all because 1) it
brings no value, and 2) it allocates a lot of memory per connection,
which, in addition to using a lot of memory, tends to degrade performance
due to cache thrashing.

This patch improves the situation by refraining from sending data frames
over a connection when more mbufs than streams are allocated. On a test
featuring 10k connections each with a single stream reading from the
cache, this patch reduces the RAM usage from ~180k buffers to ~20k bufs,
and improves the bandwidth. This may even be backported later to recent
versions to improve memory usage. Note however that it is efficient only
when combined with e16762f8a ("OPTIM: mux-h2: call h2_send() directly
from h2_snd_buf()"), and tends to slightly reduce the single-stream
performance without it, so in case of a backport, the two need to be
considered together.
2023-11-09 17:24:00 +01:00
Willy Tarreau
a13f8425f0 MINOR: task/debug: make task_queue() and task_schedule() possible callers
It's common to see process_stream() being woken up by wake_expired_tasks
in the profiling output, without knowing which timeout was set to cause
this. By making it possible to record the call places of task_queue()
and task_schedule(), and by making wake_expired_tasks() explicitly not
replace it, we'll be able to know which task_queue() or task_schedule()
was triggered for a given wakeup.

For example below:
  process_stream                51200   311.4ms   6.081us   34.59s    675.6us <- run_tasks_from_lists@src/task.c:659 task_queue
  process_stream                19227   70.00ms   3.640us   9.813m    30.62ms <- sc_notify@src/stconn.c:1136 task_wakeup
  process_stream                 6414   102.3ms   15.95us   8.093m    75.70ms <- stream_new@src/stream.c:578 task_wakeup

It's visible that it's the run_tasks_from_lists() which in fact applies
on the task->expire returned by the ->process() function itself.
2023-11-09 17:24:00 +01:00
Willy Tarreau
0eb0914dba MINOR: task/debug: explicitly support passing a null caller to wakeup functions
This is used for tracing and profiling. By permitting to have a NULL
caller, we allow a caller to explicitly pass zero to state that the
current caller must not be replaced. This will soon be used by
wake_expired_tasks() to avoid replacing a caller in the expire loop.
2023-11-09 17:24:00 +01:00
Amaury Denoyelle
4dee110f56 BUG/MINOR: quic: fix retry token check inconsistency
A client may send multiple INITIAL packets if ClientHello is too big for
only one. In case a Retry token is used, the client must reuse it for
every INITIAL packets.

On the haproxy server side, there was an inconsistency to handle these
packets depending on the socket mode :
* when using listener socket, token is always revalidated.
* when using connection socket, token check is bypassed. This is because
  quic_conn instance is known through its socket and thus
  quic_rx_pkt_retrieve_conn() is not necessary.

RFC 9000 does not seems to mandate retry token validation after the
first INITIAL packet per connection. Thus, this patch chooses to bypass
the check every time the connection instance is known, as this indicates
that a previous token was already validated.

This should be backported up to 2.7.
2023-11-09 16:57:37 +01:00
Amaury Denoyelle
bb28215d9b MEDIUM: quic: define an accept queue limit
QUIC connections are pushed manually into a dedicated listener queue
when they are ready to be accepted. This happens after handshake
finalization or on 0-RTT packet reception. Listener is then woken up to
dequeue them with listener_accept().

This patch comptabilizes the number of connections currently stored in
the accept queue. If reaching a certain limit, INITIAL packets are
dropped on reception to prevent further QUIC connections allocation.
This should help to preserve system resources.

This limit is automatically derived from the listener backlog. Half of
its value is reserved for handshakes and the other half for accept
queues. By default, backlog is equal to maxconn which guarantee that
there can't be no more than maxconn connections in handshake or waiting
to be accepted.
2023-11-09 16:24:00 +01:00
Amaury Denoyelle
3df6a60113 MEDIUM: quic: limit handshake per listener
Implement a limit per listener for concurrent number of QUIC
connections. When reached, INITIAL packets for new connections are
automatically dropped until the number of handshakes is reduced.

The limit value is automatically based on listener backlog, which itself
defaults to maxconn.

This feature is important to ensure CPU and memory resources are not
consume if too many handshakes attempt are started in parallel.

Special care is taken if a connection is released before handshake
completion. In this case, counter must be decremented. This forces to
ensure that member <qc.state> is set early in qc_new_conn() before any
quic_conn_release() invocation.
2023-11-09 16:23:52 +01:00
Amaury Denoyelle
278808915b MINOR: quic: reduce half open counters scope
Accounting is implemented for half open connections which represent QUIC
connections waiting for handshake completion. When reaching a certain
limit, Retry mechanism is automatically activated prior to instantiate
new connections.

The issue with this behavior is that two notions are mixed : QUIC
connection handshake phase and Retry which is mechanism against
amplification attacks. As such, only peer address validation should be
taken into account to activate Retry protection.

This patch chooses to reduce the scope of half_open_conn. Now only
connection waiting to validate the peer address are now accounted for.
Most notably, connections instantiated with a validated Retry token
check are not accounted.

One impact of this patch is that it should prevent to activate Retry
mechanism too early, in particular in case if multiple handshakes are
too slow. Another limitation should be implemented to protect against
this scenario.
2023-11-09 16:23:52 +01:00
Amaury Denoyelle
d38bb7f8a7 MEDIUM: quic: adjust address validation
When a new QUIC connection is created, server considers peer address as
not yet validated. The server must limit its sending up to 3 times the
content already received. This is a defensive measure to avoid flooding
a remote host victim of address spoofing.

This patch adjust the condition to consider the peer address as
validated. Two conditions are now considered :
* successful handling of a received HANDSHAKE packet. This was already
  done before although implemented in a different way.
* validation of a Retry token. This was not considered prior this patch
  despite RFC recommandation.

This patch also adjusts how a connection is internally labelled as using
a validated peer address. Before, above conditions were checked via
quic_peer_validated_addr(). Now, a flag QUIC_FL_CONN_PEER_VALIDATED_ADDR
is set to labelled this. It already existed prior this patch but was
only used for quic_cc_conn. This should now be more explicit.
2023-11-09 16:23:52 +01:00
Christopher Faulet
3a051ca0c8 BUG/MEDIUM: mux-h1: Exit early if fast-forward is not supported by opposite SC
The commit 4be0c7c65 ("MEDIUM: stconn/muxes: Loop on data fast-forwarding to
forward at least a buffer") introduced a regression. In h1_fastfwd(), if
data fast-forwarding is not supported by the opposite SC, we must exit
without calling se_donn_ff(). Otherwise a BUG_ON() will be triggered because
the opposite mux has no .done_fastfwd() callback function.

No backport needed.
2023-11-09 15:18:43 +01:00
William Lallemand
3ac3a06963 MEDIUM: mworker: -W is mandatory when using -S
Defining a master CLI without the master-worker mode emits a warning
since version 1.8. This patch enforce the behavior by forbiding the
usage of the -S option without the master-worker mode.
2023-11-09 15:07:15 +01:00
William Lallemand
705a72fd19 DOC: management: -q is quiet all the time
The documentation about -q seems wrong, it does not output messages
after the startup, it disables all messages. It was always quiet with
the stdio_quiet() function.

Must be backported in all stable versions.
2023-11-09 14:39:11 +01:00
William Lallemand
da24b462c3 MEDIUM: errors: move the MODE_QUIET test in print_message()
Move the MODE_QUIET and MODE_VERBOSE test in print_message() so we
always output in the startup-logs even with MODE_QUIET.

ha_warning(), ha_alert() and ha_notice() does not check the MODE_QUIET
and MODE_VERBOSE anymore, it is done before doing the fprintf() in
print_message().
2023-11-09 14:39:11 +01:00
William Lallemand
59d699c0c4 MINOR: errors: does not check MODE_STARTING for log emission
ha_alert(), ha_warning() and ha_notice() shouldn't check MODE_STARTING
for log emission. Let's remove the check.

This shouldn't do much since the stdio_quiet() function mute the output
in main().
2023-11-09 14:39:11 +01:00
William Lallemand
b959b752f9 MINOR: errors: ha_alert() and ha_warning() uses warn_exec_path()
Move the code to display the haproxy version and path during starting
mode, which is called by the first ha_alert() or ha_warning().
2023-11-09 14:39:11 +01:00
Christopher Faulet
78021ee9ef BUG/MEDIUM: stconn: Don't update stream expiration date if already expired
The commit 08d7169f4 ("MINOR: stconn: Don't queue stream task in past in
sc_notify()") tried to fix issues with epiration date set in past for the
stream in sc_notify(). However it remains some cases where the stream
expiration date may already be expired before recomputing it. This happens
when an event is reported by the mux exactly when a timeout is triggered. In
this case, depending on the scheduling, the SC may be woken up before the
stream. For these cases, we fall into the BUG_ON() preventing to queue in
the past.

So, it remains unexpected to queue a task in the past. The BUG_ON() is
correct at this place. We must just avoid to recompute the stream expiration
date if it is already expired. At worst, the stream will be woken up for
nothing. But it is not really a big deal because it will only happen on
timeouts from time to time. It is so sporadic that we can ignore it from a
performance point of view.

This patch must be backpoted to 2.8. Be careful to remove the BUG_ON() on
the 2.8.
2023-11-09 12:08:59 +01:00