Willy Tarreau [Wed, 27 Nov 2024 13:24:16 +0000 (14:24 +0100)]
BUG/MINOR: debug: COUNT_IF() should return true/false
The COUNT_IF() macro was initially meant to return true/false to be used
in if() conditions but had an extra do { } while(0) that prevents it from
doing so. Let's get rid of the do { } while(0) before the code generalizes
to too many places. There's no impact on existing code, but may have to be
backported if future fixes rely on it.
Define a new function quic_register_build_options(). Its purpose is to
register a build options string for QUIC features which is reported when
using haproxy -vv.
This will allow to easily determine if connection socket-owner mode and
GSO are supported or not. Here is the new filtered output :
$ ./haproxy -vv|grep '^QUIC:'
QUIC: connection socket-owner mode support : yes
QUIC: GSO emission support : yes
Two features are tested on startup via quic_test_socketopts() :
connection socket-owner mode support and GSO. Extract both test in their
separated functions called by quic_test_socketopts().
This patch will allow to reuse easily QUIC features detection for build
options report via haproxy -vv.
MINOR: quic: convert startup check in a freestanding function
quic_test_socketopts() function is used to detect system support for
QUIC network stack. Previously, it relies on an already bound listener
instance, notably to ensure that two UDP sockets can be bound on the
same source address.
Improve quic_test_socketopts() to run without any listener argument. It
now automatically instantiates and manipulates two dummy sockets FDs to
check for multi-bind support. This brings two advantages :
* the function is now called via an initcall
* it will easily be reusable to implement build option description
MINOR: mux-fcgi: use explicit __objt_server on idle conn reinsert
This commit is the counterpart of the previous one for FCGI mux. It
replaces objt_server() by unsafe __objt_server(), as conn target is
guarantee to point to a valid server instance, which can then be used as
_srv_add_idle() argument.
MINOR: mux-spop: use explicit __objt_server on idle conn reinsert
This commit is the counterpart of the previous one for SPOP mux. It
replaces objt_server() by unsafe __objt_server(), as conn target is
guarantee to point to a valid server instance, which can then be used as
_srv_add_idle() argument.
This should fix coverity report from github issue #2811.
MINOR: mux-h2: use explicit __objt_server on idle conn reinsert
This commit is the counterpart of the previous one for H2 mux. It
replaces objt_server() by unsafe __objt_server(), as conn target is
guarantee to point to a valid server instance, which can then be used as
_srv_add_idle() argument.
MINOR: mux-h1: use explicit __objt_server on idle conn reinsert
When dealing with a backend connection, H1 mux IO handler must reinsert
it in its idle list pool if it was extracted from it at the beginning.
This is the case if conn_in_list is true.
On reinsert, idle list pool is retrieved via the server instance
accessible from <conn.target>. Replace objt_server usage with
__objt_server as an idle connection is always attached to a server. This
ensures that there is no issue when using _srv_add_idle() then.
This should fix coverity report from github issue #2810.
in 50322dff ("MEDIUM: server: add init-state"), some examples on how to
use init-state server keyword were added alongside with the keyword
documentation.
However, as reported by Nick Ramirez, there was an error because the
example that stated that haproxy will pass the traffic to the server after
3 successful health checks used the "init-state down" instead of the
"init-state fully-down". Thus the behavior wouldn't match what the
comment said (only 1 successful health check was required).
Here we fix the example in itself to match with the comment. Also the
following example ("# or") was also affected, but it is kind of
redundant as the main purpose of the examples are to illustrate the
feature in itself and not how to use server-template directive, so we
remove it.
CLEANUP: startup: make if condition to kill old pids more readable
Update comment and condition. nb_oldpids it's not a pointer, but a signed int,
which keeps the max number of elements in oldpids array. So, it's a good
practice to check, if it's strictly positive here.
BUG/MINOR: startup: fix error path for master, if can't open pidfile
If master process can't open a pidfile, there is no sense to send SIGTTIN to
oldpids, as it will exit. So, old workers will terminate as well. It's better
to send the last alert to the log about unrecoverable error, because master is
already in its polling loop.
For the standalone mode we should keep the previous logic in this case: send
SIGTTIN to old process and unbind listeners for the new one. So, it's better
to put this error path in main(), as it's done when other configuration settings
can't be applied.
The old master process and its workers do not stop. Since the master-worker
refactoring, the code, which sends USR1/TERM to old pids from -sf, is called
only for the standalone mode. In master-worker mode we should receive the READY
message from the newly forked worker at first, in order to be able to terminate
the previous master.
So, to fix this, let's terminate the previous master in _send_status(), where
we parse the READY message from the newly forked worker. And let's continue to
use oldpids array, as it was in 3.0, in order to stop the workers, launched
before the reload.
BUG/MINOR: mworker: don't save program PIDs in oldpids
After reload, previously launched programs are stopped explicitly in
mworker_ext_launch_all(). So, there is no longer need to save their PIDs in
oldpids array before the master reexec().
This also prepares the fix of "-D -W -sf/-st" modes, as we will need to
loop over this array in the master process context, in order to stop the
previous master, when the new one is ready.
BUG/MINOR: config: Fix parsing of accept-invalid-http-{request,response}
These options are now deprectated, but the proxy capabilities are not
properly checked during the configuration parsing leading to always ignore
these options. This is now fixed by checking the frontend capability for
"accept-invalid-http-request" option and the backend capability for
"accept-invalid-http-response" option.
In addition, the messages about the deprecation of these options are now
emitted with ha_warning() instead of ha_alert() because they are only
warnings and not errors.
This patch should fix the issue #2806. It must be backported to 3.1.
MINOR: mux-quic: clean up zero-copy done_ff callback
Recently, an issue was found with QUIC zero-copy forwarding on 3.0
version. A desynchronization could occur internally in QCS Tx bytes
counters which would cause a BUG_ON() crash on qcs_destroy() when the
stream is detached.
It was silently fixed in version 3.1 by the following patch. As it was
considered as an optimization, it was not scheduled yet for backport.
This mistake has been caused due to some counter-intuitive manipulation
in QUIC zero-copy implementation. Try to streamline this in QUIC MUX
done_ff callback and its application protocol counterpart. Especially
for values exchanged between MUX and application on one side, and MUX
and stconn layer as done_fastfwd return value.
First, application done_ff callback now returns the length of the wholly
encoded frame. For HTTP/3, it means header length + payload length h3
frame. This value can then be reused as qcc_send_stream() argument to
increase QCS Tx soft offset.
As previously, special care has been taken to ensure that QUIC MUX
done_ff only return the transferred data bytes. Thus, any extra offset
for HTTP/3 header is properly excluded. This is mandatory for stconn
layer to consider the transfer has completed.
Secondly, remove duplicated code in application done_ff to reset iobuf
info. This is now factorize in QUIC MUX done_ff itself.
Willy Tarreau [Thu, 5 Dec 2024 14:18:38 +0000 (15:18 +0100)]
BUG/MEDIUM: mux-h2: make sure not to touch dummy streams when sending WU
Since commit 1cc851d9f2 ("MEDIUM: mux-h2: start to update stream when
sending WU") we started storing stream offsets in the h2s struct. These
offsets are updated at a few points, where it's safe to write to the
stream, and in h2c_send_strm_wu(), where the h2s->h2c was not performed.
Due to this, nothing protects the h2s from being updated when sending a
WU for a closed stream, which might only happen when acknowledging a
frame after resetting that stream, which is quite unlikely. In any case
if this happens, it will crash as in issue #2793 since the closed streams
are purposely read-only to catch such bugs.
The fix is trivial, just check h2s->h2c before deciding to update the
stream.
Thanks to @Wahnes for reporting this, and Christopher for spotting the
cause. This needs to be backported to 3.1 only.
CLEANUP: stktable: add some stktable flags polishing
Better late than never, commit 1f73d35 ("MINOR: stktable: implement
"recv-only" table option") implemented stktable flags and initial
definitions, but it lacks some comments plus the flag is stored as
16bits but the SKT_FL_ definition width allows for only 8bits so
it is a bit confusing, let's fix that
When "recv-only" keyword is added on a stick table declaration (in peers
or proxy section), haproxy considers that the table is only used for
data retrieval from a remote location and not used to perform local
updates. As such, it enables the retrieval of local-only values such
as conn_cur that are ignored by default. This can be useful in some
contexts where we want to know about local-values such are conn_cur
from a remote peer.
To do this, add stktable struct flags which default to NONE and enable
the RECV_ONLY flag on the table then "recv-only" keyword is found in the
table declaration. Then, when in peer_treat_updatemsg(), when handling
table updates, don't ignore data updates for local-only values if the flag
is set.
BUG/MINOR: quic: remove startup alert if conn socket-owner unsupported
QUIC relies on several advanced network API features from the kernel to
perform optimally. Checks are performed during startup to ensure that
these features are supported. A fallback is automatically performed for
every incompatible feature.
Besides the automatic fallback mechanism, a message is also reported to
the user at the same time. Previously, alert level was used, but it is
incorrect as it is reserved for unrecoverable errors which should
prevent haproxy to start. Warning level could be used, but this can
annoy users running with zero-warning mode.
This patch removes the alert message when 'socket-owner connection' mode
cannot be activated. Convert the message to a diag level. This allows
users to start without forcing configuration modification to hide a
warning. Besides, several feature fallback such as the polling mechanism
does not emit any warning either, so it's better to adopt a similar
behavior for QUIC features.
Amaury Denoyelle [Thu, 28 Nov 2024 10:27:15 +0000 (11:27 +0100)]
BUG/MEDIUM: mux-quic: remove pacing status when everything is sent
TASK_F_USR1 is used by MUX tasklet when emission has been interrupted
due to pacing. When the tasklet runs again, only qcc_purge_sending()
will be called as an optimization.
Pacing status is only removed via qcc_wakeup(). Until then, TASK_F_USR1
is not cleared. This causes an issue after emission with pacing
completion if the MUX tasklet is woken up for a recv subscribe, as
qcc_wakeup() is not used by quic-conn layer. The tasklet will
incorrectly run only for pacing emission, without handling reception
process. Worst, a crash will occur if QCC tx frames list is empty, due
to a BUG_ON() in qcc_purge_sending().
Recv subscribe is only used for 0-RTT, when QUIC MUX is instantiated
before quic-conn handshake completion. Thus, this bug can only be
reproduced with 0-rtt. Furthermore, MUX must already have emitted at
least a few response bytes with pacing, before QUIC handshake
completion. It cannot easily be reproduced, at least with CLI clients
where the handshake is always already completed before MUX exchanges.
To fix this, remove TASK_F_USR1 when pacing emission has been completed.
At least, this prevents BUG_ON() on qcc_purge_sending() as it won't be
called with an empty QCC Tx frame list anymore. However, this bug has
revealed that MUX tasklet architecture is not suitable when both
handling reception and emission part. This will be improved in a future
serie of patches.
Willy Tarreau [Wed, 4 Dec 2024 14:58:49 +0000 (15:58 +0100)]
BUG/MINOR: init: do not call fork_poller() for non-forked processes
In 3.1-dev10, commit 8dd4efe42f ("MAJOR: mworker: move master-worker
fork in init()") made the fork_poller() code unconditional, while it
is only desirable for processes that have been forked from a parent
(standalone daemon mode) or from a master (master-worker mode). The
call can be expensive in some cases as it will create a new poller,
scan and try to migrate to it all existing FDs till the highest known
one. With very high numbers of FDs, this can take several seconds to
start.
Willy Tarreau [Wed, 4 Dec 2024 07:26:24 +0000 (08:26 +0100)]
BUG/MEDIUM: init: make sure only daemonized processes change their session
Commit 8dd4efe42f ("MAJOR: mworker: move master-worker fork in init()")
introduced some sensitive changes to the startup code (which was
expected), and one sensitive change is that the second call to setsid()
was accidentally made unconditional. As such it even applies to foreground
processes, resulting in foreground processes being detached from the
terminal and no longer responding to Ctrl-C nor Ctrl-Z. An example of
this simply consists in start haproxy -db under sudo. Then a new shell
is required to stop it.
This patch removes this second setsid(), as it is already done in
apply_daemon_mode().
BUG/MINOR: quic: fix bbr_inflight() calls with wrong gain value
This patch fixes two wrong calls to bbr_inflight().
bbr_target_inflight() aim is to compute the number of bytes BBR has to put on
the network as bytes in flight (sent but not acked bytes). It must call
bbr_inflight() with the current window gain value (in place of a wrong fixed 100
gain value here, in percents).
bbr_is_time_to_cruise() also called bbr_inflight() with a wrong gain value
as parameter due to a confusion between the value mentioned by the RFC (1
meaning 100% of the current window) and our implementation which needs value in
percents (so 100 in place of 1 here). Note that bbr_is_time_to_cruise() aim is to
make BBR the decision to leave the probing_bw down state. The bug had as side
effect to make BBR stay in this state during too long periods of time during
which the bottleneck bandwidth is decreasing, leading to big oscillations
between the mininum and maximum bottleneck bandwidth estimations.
This patch must be backported to 3.1 where BBR was first implemented.
Willy Tarreau [Thu, 28 Nov 2024 18:42:22 +0000 (19:42 +0100)]
MINOR: tasklet: set TASK_WOKEN_OTHER on tasklets by default
Now when tasklets are woken up via tasklet_wakeup(), tasklet_wakeup_on()
or tasklet_wakeup_after(), either the optional wakeup flags will be used,
or TASK_WOKEN_OTHER will be used.
This allows tasklet handlers waking up for any given cause to notice
whether or not they were also woken for another reason. For example, a
mux handler could skip heavy parts when seeing that TASK_WOKEN_OTHER is
absent, proving that no standard tasklet_wakeup() was done, for example
in response to a subscribe().
The benefit of the TASK_WOKEN_* flags is that they're purged during the
wakeup, and that they're easy to check for using TASK_WOKEN_ANY.
TASK_F_UEVT1 and TASK_F_UEVT2 are also usable for private use (e.g. wakeup
from a stream to a connection inside a mux).
Probably that in the future, code dealing with subscribe events should
start to place TASK_WOKEN_IO like is done for upper layers.
Pidfile should be created at the latest initialization stage, when we are
sure, that process is able to start successfully, otherwise PID value, written
in this file is no longer valid.
So, for the standalone mode, let's move the block, which opens the pidfile and
let's put it just before applying "chroot". In master-worker mode, master
doesn't perform chroot. So it creates the pidfile, only when the "READY"
message from the newly forked worker is received.
BUG/MINOR: startup: close pidfd and free global.pidfile in handle_pidfile()
After master-worker mode refactoring, global.pidfile is only used in
handle_pidfile(), which opens the provided file and writes the PID into it. So,
it's more appropriate to perform the close(pidfd) and ha_free(&global.pidfile)
also in this function.
This commit prepares the fix of the pidfile creation, as it's created now very
early, when we are not sure, that process has successfully started. In
master-worker mode handle_pidfile() can be called in the master process context.
So, let's make it accessible from other compilation units via global.h.
BUG/MINOR: signal: register default handler for SIGINT in signal_init()
When haproxy is launched in a background and in a subshell (see example below),
according to POSIX standard (2.11. Signals and Error Handling), it inherits
from the subshell SIG_IGN signal handler for SIGINT and SIGQUIT.
$ (./haproxy -f env4.cfg &)
So, when haproxy is lanched like this, it doesn't stop upon receiving
the SIGINT. This can be a root cause of some unexpected timeouts, when haproxy
is started under VTest, as VTest sends to the process SIGINT in order to
terminate it. To fix this, let's explicitly register the default signal
handler for the SIGINT in signal_init() initcall.
MINOR: hlua: fix ambiguous hlua usage in hlua_filter_delete()
In GH #2804, @Bbulatov reported that the result of hlua_stream_ctx_get()
was used and de-referenced without checking if it's NULL in
hlua_filter_delete() while other functions used to check for NULL before
de-referencing it.
In fact hlua_stream_ctx_get() can only return NULL if
hlua_stream_ctx_prepare() failed or was not called on the current stream.
Now because of the filter's API, since hlua_filter_delete() is mapped as
detach method and hlua_filter_new() as attach method, and since
hlua_filter_new() is responsible for calling hlua_stream_ctx_prepare(),
there's no reason hlua_filter_delete() should be called if
hlua_filter_new() failed or wasn't called. Thus we can assume that hlua
can never be NULL in hlua_filter_delete(), so we add a BUG_ON() to ensure
it is always the case and remove the ambiguity.
BUG/MINOR: listener: fix potential null pointer dereference in listener_release()
As reported by @Bbulatov on GH #2804, fe is found at multiple places in
listener_release(): in some places it is first checked against NULL before
being de-referenced while in some other places it is not, which is
ambiguous and could hide a bug.
In practise, fe cannot be NULL for now, but it might not be the case in
the future as we want to keep the possibility to run isolated listeners
(that is, without proxy attached).
We've already ensured this was the case with a57786e ("BUG/MINOR:
listener: null pointer dereference suspected by coverity"), but
this promise was recently broken by 65ae134 ("BUG/MINOR: listener: Wake
proxy's mngmt task up if necessary on session release").
Let's fix that by conditionning the block with an "else if" statement
instead of a regular "else".
No need for backport except if multi-connection protocols (ie: FTP) were
to be backported as well.
BUILD: quic: fix a build error about an non initialized timestamp
This is to please a non identified compilers which complains about an hypothetic
<time_ns> variable which would be not initialized even if this is the case only
when it is not used.
This build issue arrived with this commit:
BUG/MINOR: improve BBR throughput on very fast links
Should be backported to 3.1 with this previous commit.
BUG/MINOR: h1-htx: Use default reason if not set when formatting the response
When the response status line is formatted before sending it to the client,
if there is no reason set, HAProxy should add one that matches the status
code, as stated in the configuration manual. However it is not performed.
It is possible to hit this bug when the response comes from a H2 server,
because there is no reason field in HTTP/2 and above.
This patch should fix the issue #2798. It should be backported to all stable
versions.
BUG/MEDIUM: http-ana: Reset request flag about data sent to perform a L7 retry
It is possible to loose the request after several L7 retries, leading to
crashes, because the request channel flag stating some data were sent is not
properly reset.
When a L7 retry is performed, some flags on different entities must be reset
to be sure a new connection will be properly retried, just like it was the
first one, mainly because there was no connection establishment failure. One
of them, on the request channel, is not reset. The flag stating some data
were already sent. It is annoying because this flag is used during the
connection establishment to know if an error is triggered at the connection
level or at the data level. In the last case, the error must be handled by
the HTTP response analyzer, to eventually perform another L7 retry.
Because CF_WROTE_DATA flag is not removed when a L7 retry is performed, a
subsequent connection establishment error may be handled as a L7 error while
in fact the request was never sent. It also means the request was never
saved in the buffer used to performed L7 retries. Thus, on the next L7
retires, the request is just lost. This forecefully leads to a bunch of
undefined behavior. One of them is a crash, when the request is used to
perform the load-balancing.
This patch should fix issue #2793. It must be backported to all stable
versions.
Amaury Denoyelle [Fri, 29 Nov 2024 13:28:09 +0000 (14:28 +0100)]
BUG/MEDIUM: quic: prevent stream freeze on pacing
On snd_buf completion, QUIC MUX tasklet is scheduled if newly data has
been transferred from the stream layer. Thanks to qcc_wakeup(), pacing
status is removed from tasklet, which ensure next emission will reset Tx
frames and use the new data.
Tasklet is not scheduled if MUX is already subscribed on send due to a
previous blocking condition. This is an optimization to prevent an
unneeded IO handler execution. However, this causes a bug if an emission
is currently delayed due to pacing. As pacing status is not removed on
snd_buf, next emission process will continue emission with older data
without refreshing the newly transferred one.
This causes a transfer freeze. Unless there is some activity on the
connection, the transfer will be eventually aborted due to idle timeout.
To fix this, remove TASK_F_USR1 if tasklet wakeup is not called due to
send subscription. Note that this code is also duplicated in done_ff for
zero-copy transfer.
BUG/MEDIUM: event_hdl: fix uninitialized value in async mode when no data is provided
In _event_hdl_publish(), when we prepare the asynchronous event and no
<data> was provided (set to NULL), we forgot to initialize the _data
event_hdl_async_event struct member to NULL, which leads to uninitialized
reads in event_hdl_async_free_event() when the event is freed:
==1002331== Conditional jump or move depends on uninitialised value(s)
==1002331== at 0x35D9D1: event_hdl_async_free_event (event_hdl.c:224)
==1002331== by 0x1CC8EC: hlua_event_runner (hlua.c:9917)
==1002331== by 0x39AD3F: run_tasks_from_lists (task.c:641)
==1002331== by 0x39B7B4: process_runnable_tasks (task.c:883)
==1002331== by 0x314B48: run_poll_loop (haproxy.c:2976)
==1002331== by 0x315218: run_thread_poll_loop (haproxy.c:3190)
==1002331== by 0x18061D: main (haproxy.c:3747)
The bug severity was set to MEDIUM because of its nature, and it's best
if this patch can be backported up to 2.8. But in practise it can only be
triggered with events that don't provide optional data: since PAT_REF
events are the first native events making use of this feature, this bug
shouldn't be an issue before f72a66e ("MINOR: pattern: publish event_hdl
events on pat_ref updates")
BUG/MINOR: hlua_fcn: fix Patref:set() force parameter
Patref:set(key, val[, force]) takes optional "force" parameter (defaults
to false) to force the entry to be created if it doesn't already exist
To retrieve the value, lua_tointeger() was used in place of
lua_toboolean(), and because of that force is not enabled if "true"
is passed as parameter (only numbers were recognized) despite the
documentation mentioning that "force" is a boolean.
To fix the issue, we replace lua_tointeger by lua_toboolean.
Also, the doc was updated to rename "bool" to "boolean" for the "force"
parameter to stay consistent with historical naming in the file.
No backport needed unless 9ee37de5c ("MINOR: hlua_fcn: add Patref:set()")
is.
DOC: lua: prefer Patref:{set,add}() over legacy methods for acl and maps
Patref:set() can achieve the same thing as core.set_map()
Patref:add() can achieve the same thing as core.add_acl()
Patref:del() can achieve the same thing as core.del_map() and
core.del_acl()
As a bonus, Patref:{set,add} are more efficient than their core
legacy equivalent, because they don't require systematic pattern
reference lookup for each individual operation.
Let's mention that in the doc to encourage Patref methods adoption.
Just like we did for server events, in this patch we expose the PAT_REF
event family (see "MINOR: event_hdl: add PAT_REF events") in Lua.
Unlike server events, Patref events don't provide additional event data,
and the registration can only take place from a Patref object (ie: not
globally).
Thanks to this commit it now becomes possible to trigger actions when
updates are performed on a map (or acl list) being monitor, without
the need to loop or use inefficient workarounds.
There is no cli equivalent for this one. It is similar to Patref:add()
excepts thay it takes a table as parameter (for acl: table of keys, for
maps: table of keys:values). The goal is to add multiple entries at once
to limit locking time to the strict minimum. It is recommended to use this
one over Patref:add() when adding multiple entries at once.
Just like "set map" on the cli, the Patref:set() method (only relevant
for maps) can be used to modify an existing entry's value in the pattern
reference pointed to by the Lua Patref object. Lookup is performed on the
key. The update will target the live pattern reference version, unless
Patref:prepare() is ongoing.
Just like "del map" and "del acl" on the cli, the Patref:del() method can
be used to delete an existing entry in the pattern reference pointed to
by the Lua Patref object. The update will target the live pattern
reference version, unless Patref:prepare() is ongoing.
Just like "add map" and "add acl" on the cli, the Patref:add() method can
be used to add a new entry to the pattern reference pointed to by the
Lua Patref object. The update will target the live pattern reference
version, unless Patref:prepare() is ongoing.
If Patref:commit() was used and the new version (generation) isn't going
to be committed, calling Patref:giveup() will allow allocated resources
to be freed and reused. It is a good habit to call this if commit()
isn't called after a prepare().
It is a special Lua Patref method: it bypasses the commit/prepare logic
and purges the whole pattern reference items pointed to by Patref Lua
object (all versions, not just the current one). It doesn't have a cli
equivalent: it leverages pat_ref_purge_range().
Just like the "prepare map" or "prepare acl" on the cli, but for Lua:
it leverages the pattern API to create a subset (ie: a new generation id)
that will automatically be used as target for following Patref operations
(add/set/del...) until the "commit" method is invoked to atomically push
the pending updates.
commit() method may be used to commit pending updates on the local patref
object:
hlua_patref flags were added:
HLUA_PATREF_FL_GEN means the patref object has been updated
and it is associated to a new revision (curr_gen) in order to prepare
and commit the pending updates.
upon commit, the pattern API is leveraged with curr_gen as revision to
commit new object items. Once commit is performed, previous (pending)
revisions that are older than the committed one are cleaned up (similar
to what's done with commit on the cli). Also, Patref function APIs now
take into account curr_gen to perform lookups.
MINOR: pattern: add pat_ref_may_commit() helper function
pat_ref_may_commit() may be used to know if a given generation ID id still
valid, which means it may still be committed at some point. Else it means
that another pending generation ID older than the tested one was already
committed and thus other generations ID below this one are stale and must
be regenerated.
MINOR: hlua_fcn: wrap pat_ref struct for patref class
In order to extend the patref class features, let's wrap the pat_ref struct
into hlua_patref struct. This way we may add additional data alongside the
pat_ref pointer to store additional context required for pat_ref data
manipulation from lua.
Since the wrapper (hlua_patref) is an allocated object, we declare the _gc
metamethod for patref class in order to properly cleanup resources when
they are out of scope.
core.get_patref() method may be used to get a reference to a pattern
object (pat_ref struct which is used for maps and acl storage) from
Lua by providing the reference name (filename for files, or prefix+name
for opt or virtual pattern references).
Implement patref class to expose pat_ref struct internal pattern struct
in lua. This is some prerequisite work needed to be able to manipulate
exisiting generic pattern object lists (acl/map) from Lua, because the Map
class can only be used to perform matching ops on Map files.
MINOR: pattern: publish event_hdl events on pat_ref updates
Now that PAT_REF events were defined in previous commit, let's actually
publish them from pattern API where relevant. Unlike server events,
pattern reference events are only published in the pat_ref subscriber's
list on purpose, because in some setups patref updates (updates performed
on a map for instance from action or cli) are very frequent, and we don't
want to impact pattern API performance just for that.
Moreover, as the main use case is to be able to subscribe to maps updates
from Lua, allowing a per-pattern reference registration is already enough.
No additional data is provided for such events (also for performance reason)
Care was taken not to publish events when the update doesn't affect the
live subset (the one targeted by curr_gen).
This is some prerequisite work for implementing PAT_REF events.
In this commit we define the PAT_REF event_hdl family (which gets family
slot id #2), with the following supported events:
- EVENT_HDL_SUB_PAT_REF_ADD: element was added to the current version of
the pattern ref
- EVENT_HDL_SUB_PAT_REF_DEL: element was deleted from the current
version of the pattern ref
- EVENT_HDL_SUB_PAT_REF_SET: element was modified in the current version
of the pattern ref
- EVENT_HDL_SUB_PAT_REF_COMMIT: pending element(s) was/were commited in
the current version of the pattern ref
- EVENT_HDL_SUB_PAT_REF_CLEAR: all elements were cleared from the
current version of the pattern ref
The goal is to be able to track a pat_ref struct in order to be notified
when it is updated. For performance reasons, events from this family won't
provide any additional info, and will only be published in the pat_ref
subscription list. Indeed, pat_ref may be updated at a relatively high
frequency (or worse, batch work), so we cannot afford doing expensive
treatment for each update.
BUG/MINOR: improve BBR throughput on very fast links
This patch fixes the loss of information when computing the delivery rate
(quic_cc_drs.c) on links with very low latency due to usage of 32bits
variables with the millisecond as precision.
Initialize the quic_conn task with TASK_F_WANTS_TIME flag ask it to ask
the scheduler to update the call date of this task. This allows this task to get
a nanosecond resolution on the call date calling task_mono_time(). This is enabled
only for congestion control algorithms with delivery rate estimation support
(BBR only at this time).
Store the send date with nanosecond precision of each TX packet into
->time_sent_ns new quic_tx_packet struct member to store the date a packet was
sent in nanoseconds thanks to task_mono_time().
Make use of this new timestamp by the delivery rate estimation algorithm (quic_cc_drs.c).
Rename current ->time_sent member from quic_tx_packet struct to ->time_sent_ms to
distinguish the unit used by this variable (millisecond) and update the code which
uses this variable. The logic found in quic_loss.c is not modified at all.
MINOR: log: always consider "+M" option in lf_text_len()
Historically, when lf_text_len() or lf_text() were called with a NULL
string and "+M" option was set, "-" would be printed.
However, if the input string was simply an empty one with len > 0, then
nothing would be printed. This can happen if lf_text() is called with
an empty string because in this case len is set to size (indeed, for
performance reasons we don't pre-compute the length, we stop as soon
as we encounter a NULL-byte)
In practise, a lot of call places making use of lf_text() or lf_text_len()
try their best to avoid calling lf_text() with an empty string, and
instead explicitly call lf_text_len() with NULL as parameter to consider
the "+M" option.
But this is not enough, as shown in GH #2797, there could still be places
where lf_text() is called with an empty string. In such case, instead of
ignoring the "+M" option, let's check after _lf_text_len() if the returned
pointer differs from the original one. If both are equal, then it means
that nothing was printed (ie: result of empty string): in that case we
check the "+M" option to print "-" when possible.
While this commit seems harmless, it's probably better to avoid
backporting it since it could break existing applications relying on the
historical behavior.
BUG/MINOR: log: fix lf_text() behavior with empty string
As reported by Baptiste in GH #2797, if a logformat alias leveraging
lf_text() ends up printing nothing (empty string), the whole logformat
evaluation stops, leading garbage log message.
This bug was introduced during 3.0 cycle in fcb7e4b ("MINOR: log: add
lf_rawtext{_len}() functions"). At that time I genuinely thought that
if strlcpy2() returned 0, it was due to a lack of space, actually
forgetting that the function may simply be called with an empty string.
Because of that, lf_text() would return NULL if called with an empty
string, and since all lf_*() helpers are expected to return NULL on
error, this explains why the logformat evaluation immediately stops in
this case.
To fix the issue, let's simply consider that strlcpy2() returning 0 is
not an error, like it was already the case before.
It should be backported in 3.1 and 3.0 with fcb7e4b.
BUG/MEDIUM: sock: Remove FD_POLL_HUP during connect() if FD_POLL_ERR is not set
epoll_wait() may return EPOLLUP and/or EPOLLRDHUP after an asynchronous
connect(), to indicate that the peer accepted the connection then
immediately closed before epoll_wait() returned. When this happens,
sock_conn_check() is called to check whether or not the connection correctly
established, and after that the receive channel of the socket is assumed to
already be closed. This lets haproxy send the request at best (if RDHUP and
not HUP) then immediately close.
Over the last two years, there were a few reports about this spuriously
happening on connections where network captures proved that the server had
not closed at all (and sometimes even received the request and responded to
it after haproxy had closed). The logs show that a successful connection is
immediately reported on error after the request was sent. After
investigations, it appeared that a EPOLLUP, or eventually a EPOLLRDHUP, can
be reported by epool_wait() during the connect() but in sock_conn_check(),
the connect() reports a success. So the connection is validated but the HUP
is handled on the first receive and an error is reported.
The same behavior could be observed on health-checks, leading HAProxy to
consider the server as DOWN while it is not.
The only explanation at this point is that it is a kernel bug, notably
because it does not even match the documentation for connect() nor epoll. In
addition for now it was only observed with Ubuntu kernels 5.4 and 5.15 and
was never reproduced on any other one.
We have no reproducer but here is the typical strace observed:
Some ressources about this issue:
- https://www.spinics.net/lists/netdev/msg876470.html
- https://github.com/haproxy/haproxy/issues/1863
- https://github.com/haproxy/haproxy/issues/2368
So, to workaround the issue, we have decided to remove FD_POLL_HUP flag on
the FD during the connection establishement if FD_POLL_ERR is not reported
too in sock_conn_check(). This way, the call to connect() is able to
validate or reject the connection. At the end, if the HUP or RDHUP flags
were valid, either connect() would report the error itself, or the next
recv() would return 0 confirming the closure that the poller tried to
report. EPOLL_RDHUP is only an optimization to save a syscall anyway, and
this pattern is so rare that nobody will ever notice the extra call to
recv().
Please note that at least one reporter confirmed that using poll() instead
of epoll() also addressed the problem, so that can also be a temporary
workaround for those discovering the problem without the ability to
immediately upgrade.
The event is accounted via a COUNT_IF(), to be able to spot it in future
issue. Just in case.
This patch should fix the issue #1863 and #2368. It may be related
to #2751. It should be backported as far as 2.4. In 3.0 and below, the
COUNT_IF() must be removed.
MEDIUM: pattern: always consider gen_id for pat_ref lookup operations
Historically, pat_ref lookup operations were performed on the whole
pat_ref elements list. As such, set, find and delete operations on a given
key would cause any matching element in pat_ref to be considered.
When prepare/commit operations were added, gen_id was impelemnted in
order to be able to work on a subset from pat_ref without impacting
the current (live) version from pat_ref, until a new subset is committed
to replace the current one.
While the logic was good, there remained a design flaw from the historical
implementation: indeed, legacy functions such as pat_ref_set(),
pat_ref_delete() and pat_ref_find_elt() kept performing the lookups on the
whole set of elements instead of considering only elements from the current
subset. Because of this, mixing new prepare/commit operations with legacy
operations could yield unexpected results.
-> the result would be that "key" entry doesn't exist anymore after the
commit, while we would expect the new value to be there instead.
Thanks to the previous commits, we may finally fix this issue: for set,
find_elt and delete operations, the current generation id is considered.
With the above example, it means that the "del map #0 key" would only
target elements from the current subset, thus elements in "version 1" of
the map would be immune to the delete (as we would expect it to work).
MEDIUM: pattern: consider gen_id in pat_ref_set_from_node()
Don't set all duplicates from a given node if they don't have the same
gen_id. Indeed, now we consider the gen_id to only work on the same
pattern ref revision.
pat_ref_gen_set(ref, gen_id, value, err) modifies to <value> the sample
of all patterns matching <key> and belonging to <gen_id> (generation id)
under <ref>
The goal is to be able to target a single subset from <ref>
split pat_ref_set() function in 2 distinct functions. Indeed, since 0844bed7d3 ("MEDIUM: map/acl: Improve pat_ref_set() efficiency (for
"set-map", "add-acl" action perfs)"), pat_ref_set() prototype was updated
to include an extra <elt> argument. But the logic behind is not explicit
because the function will not only try to set <elt>, but also its
duplicate (unlike pat_ref_set_elt() which only tries to update <elt>).
Thus, to make it clearer and better distinguish between the key-based
lookup version and the elt-based one, restotre pat_ref_set() previous
prototype and add a dedicated pat_ref_set_elt_duplicate() that takes
<elt> as argument and tries to update <elt> and all duplicates.
Willy Tarreau [Tue, 26 Nov 2024 14:24:10 +0000 (15:24 +0100)]
[RELEASE] Released version 3.1.0
Released version 3.1.0 with the following main changes :
- BUG/MAJOR: mux-h1: Properly handle wrapping on obuf when dumping the first-line
- BUILD: activity/memprofile: fix a build warning in the posix_memalign handler
- BUG/MINOR: quic: Avoid BUG_ON() on ->on_pkt_lost() BBR callback call
- CI: update to the latest AWS-LC version
- CI: update to the latest WolfSSL version
- DOC: ot: mention planned deprecation of the OT filter
- Revert "CI: update to the latest WolfSSL version"
- CI: github: add a WolfSSL job which tries the latest version
- BUILD: systemd: fix usage of reserved name "sun" in the address field
- BUILD: init: use the more portable FD_CLOEXEC for /dev/null
- CI: github: improve the Wolfssl job
- CI: github: improve the AWS-LC job
- BUG/MINOR: mux-quic: fix show quic report of QCS prepared bytes
- BUG/MEDIUM: quic: fix sending performance due to qc_prep_pkts() return
- MINOR: mux-quic: use sched call time for pacing
- CI: github: allow to run the Illumos job manually
- BUILD: tcp_sample: var_fc_counter defined but not used
- CI: github: add 'workflow_dispatch' on remaining build jobs
- DOC: config: refine a little bit the text on QUIC pacing
- MINOR: proto_sockpair: send_fd_uxst: init iobuf, cmsghdr, cmsgbuf to zeros
- MINOR: startup: rename on_new_child_failure to mworker_on_new_child_failure
- REORG: startup: move on_new_child_failure in mworker.c
- MINOR: startup: prefix prepare_master and run_master with mworker_*
- REORG: startup: move mworker_prepare_master in mworker.c
- MINOR: startup: keep updating verbosity modes only in haproxy.c
- REORG: startup: move mworker_run_master and mworker_loop in mworker.c
- REORG: startup: move mworker_reexec and mworker_reload in mworker.c
- MINOR: startup: prefix apply_master_worker_mode with mworker_*
- REORG: startup: move mworker_apply_master_worker_mode in mworker.c
- MINOR: cfgparse-quic: strengthen quic-cc-algo parsing
- BUG/MAJOR: quic: fix wrong packet building due to already acked frames
- DEV: lags/show-sess-to-flags: Properly handle fd state on server side
- BUG/MEDIUM: http-ana: Don't release too early the L7 buffer
- MINOR: quic: make bbr consider the max window size setting
- DOC: quic: Amend the pacing information about BBR.
- BUG/MEDIUM: quic: prevent EMSGSIZE with GSO for larger bufsize
- MINOR: cli: Add a "help" keyword to show sess
- MINOR: cli/quic: Add a "help" keyword to show quic
- DOC: management: mention "show sess help" and "show quic help"
- DOC: install: update the list of supported versions
- MINOR: version: mention that 3.1 is stable now
Amaury Denoyelle [Tue, 26 Nov 2024 10:03:30 +0000 (11:03 +0100)]
BUG/MEDIUM: quic: prevent EMSGSIZE with GSO for larger bufsize
A UDP datagram cannot be greater than 65535 bytes, as UDP length header
field is encoded on 2 bytes. As such, sendmsg() will reject a bigger
input with error EMSGSIZE. By default, this does not cause any issue as
QUIC datagrams are limited to 1.252 bytes and sent individually.
However, with GSO support, value bigger than 1.252 bytes are specified
on sendmsg(). If using a bufsize equal to or greater than 65535, syscall
could reject the input buffer with EMSGSIZE. As this value is not
expected, the connection is immediately closed by haproxy and the
transfer is interrupted.
This bug can easily reproduced by requesting a large object on loopback
interface and using a bufsize of 65535 bytes. In fact, the limit is
slightly less than 65535, as extra room is also needed for IP + UDP
headers.
Fix this by reducing the count of datagrams encoded in a single GSO
invokation via qc_prep_pkts(). Previously, it was set to 64 as specified
by man 7 udp. However, with 1252 datagrams, this is still too many.
Reduce it to a value of 52. Input to sendmsg will thus be restricted to
at most 65.104 bytes if last datagram is full.
If there is still data available for encoding in qc_prep_pkts(), they
will be written in a separate batch of datagrams. qc_send_ppkts() will
then loop over the whole QUIC Tx buffer and call sendmsg() for each
series of at most 52 datagrams.
MINOR: quic: make bbr consider the max window size setting
Limit the BBR congestion control window size as this is done for all the others
congestion control algorithms with tune.quic.frontend.default-max-window-size
or as first argument passed to "bbr" option for "quic-cc-algo".
BUG/MEDIUM: http-ana: Don't release too early the L7 buffer
In some cases, the buffer used to store the request to be able to perform a
L7 retry is released released too early, leading to a crash because a retry
is performed with an empty request.
First, there is a test on invalid 101 responses that may be caught by the
"junk-response" retry policy. Then, it is possible to get an error
(empty-response, bad status code...) after an interim response. In both
cases, the L7 buffer is already released while it should not.
To fix the issue, the L7 buffer is now released at the end of the
AN_RES_WAIT_HTTP analyser, but only when a response was successfully
received and processed. In all error cases, the stream is quickly released,
with the L7 buffer. So there is no leak and it is safer this way.
This patch may fix the issue #2793. It must be as far as 2.4.
BUG/MAJOR: quic: fix wrong packet building due to already acked frames
If a packet build was asked to probe the peer with frames which have just
been acked, the frames build run by qc_build_frms() could be cancelled by
qc_stream_frm_is_acked() whose aim is to check that current frames to
be built have not been already acknowledged. In this case the packet build run
by qc_do_build_pkt() is not interrupted, leading to the build of an empty packet
which should be ack-eliciting.
This is a bug detected by the BUG_ON() statement in qc_do_build_pk():
quic-cc-algo is a bind keyword which is used to specify the congestion
control algorithm. It is parsed via function bind_parse_quic_cc_algo().
The parsing function was too laxed as it used strncmp for algo token
matching. This could cause surprise if specifying an invalid algorithm
but starting identically to another entry. Especially if extra
parameters are specified in parenthesis, as in this case parameters
value will be completely ignored and default value used instead.
To fix this, convert algo argument to ist. Then, use istsplit() to
extract algo token from the optional extra arguments and compare the
whole value with isteq().
REORG: startup: move mworker_reexec and mworker_reload in mworker.c
Let's move mworker_reexec() and mworker_reload() in mworker.c. mworker_reload()
is called only within the functions, which are already in mworker.c. So, this
reorganization allows to declare mworker_reload() as a static.
REORG: startup: move mworker_run_master and mworker_loop in mworker.c
mworker_run_master() is called only in master mode. mworker_loop() is static
and called only in mworker_run_master(). So let's move these both functions in
mworker.c.
We also need here to make run_thread_poll_loop() accessible from other units,
as it's used in mworker_loop().
MINOR: startup: keep updating verbosity modes only in haproxy.c
This commit prepares the move of mworker_run_master() in mworker.c.
Let's remove from it's definition the code, which adjusts verbosity in
dependency of other global run time modes (daemon or foreground). This part
should stay in main(), where all verbosity modes are handeled for
different mode combinations.
REORG: startup: move mworker_prepare_master in mworker.c
mworker_prepare_master() performs some preparation routines for the new worker
process, which will be forked during the startup. It's called only in
master-worker mode, so let's move it in mworker.c.
REORG: startup: move on_new_child_failure in mworker.c
mworker_on_new_child_failure() performs some routines for the worker process,
if it has failed the reload. As it's called only in mworker_catch_sigchld()
from mworker.c, let's move mworker_on_new_child_failure() in mworker.c as well.
Like this it could also be declared as a static.
MINOR: startup: rename on_new_child_failure to mworker_on_new_child_failure
This patch prepares the moving of on_new_child_failure definition into
mworker.c. So, let's rename it accordingly and let's also update its
description.
MINOR: proto_sockpair: send_fd_uxst: init iobuf, cmsghdr, cmsgbuf to zeros
In master-worker mode, worker process uses now send_fd_uxst() to send
'_send_status' command to master. Since refactoring, this started to trigger
the following Valgrind reports:
==810584== Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
==810584== at 0x4AAC99D: __libc_sendmsg (sendmsg.c:28)
==810584== by 0x4AAC99D: sendmsg (sendmsg.c:25)
==810584== by 0x56350F: send_fd_uxst (proto_sockpair.c:271)
==810584== by 0x3AA25C: main (haproxy.c:4151)
==810584== Address 0x1ffefffbfe is on thread 1's stack
==810584== in frame #1, created by send_fd_uxst (proto_sockpair.c:241)
==810584==
==810584== Syscall param sendmsg(msg.msg_control) points to uninitialised byte(s)
==810584== at 0x4AAC99D: __libc_sendmsg (sendmsg.c:28)
==810584== by 0x4AAC99D: sendmsg (sendmsg.c:25)
==810584== by 0x56350F: send_fd_uxst (proto_sockpair.c:271)
==810584== by 0x3AA25C: main (haproxy.c:4151)
==810584== Address 0x1ffefffc14 is on thread 1's stack
==810584== in frame #1, created by send_fd_uxst (proto_sockpair.c:241)
==810584==
So, let's initialize with zeros all buffers, which are passed to sendmsg
syscall(), used in send_fd_uxst() to avoid these Valgrind messages. They
increase Valgrind output and could make unnoticeable some other, more important
reports.
Willy Tarreau [Mon, 25 Nov 2024 13:30:15 +0000 (14:30 +0100)]
DOC: config: refine a little bit the text on QUIC pacing
The QUIC pacing options changed a few times during their development.
For example the unit is now in datagrams not bytes. Also a few
sentences were slightly ambiguous so let's reword this.
BUILD: tcp_sample: var_fc_counter defined but not used
var_fc_counter is not used on Illumos and emit a warning
src/tcp_sample.c:291:12: warning: ‘var_fc_counter’ defined but not used [-Wunused-function]
291 | static int var_fc_counter(struct arg *args, char **err)
| ^~~~~~~~~~~~~~
Amaury Denoyelle [Thu, 21 Nov 2024 15:20:15 +0000 (16:20 +0100)]
MINOR: mux-quic: use sched call time for pacing
QUIC pacing was recently implemented to limit burst and improve overall
bandwidth. This is used only for MUX STREAM emission. Pacing requires
nanosecond resolution. As such, it used now_cpu_time() which relies on
clock_gettime() syscall.
The usage of clock_gettime() has several drawbacks :
* it is a syscall and thus requires a context-switch which may hurt
performance
* it is not be available on all systems
* timestamp is retrieved multiple times during a single task execution,
thus yielding different values which may tamper pacing calculation
Improve this by using task_mono_time() instead. This returns task call
time from the scheduler thread context. It requires the flag
TASK_F_WANTS_TIME on QUIC MUX tasklet to force the scheduler to update
call time with now_mono_time(). This solves every limitations listed
above :
* syscall invokation is only performed once before tasklet execution,
thus reducing context-switch impact
* on non compatible system, a millisecond timer is used as a fallback
which should ensure that pacing works decently for them
* timer value is now guaranteed to be fixed duing task execution
Amaury Denoyelle [Fri, 22 Nov 2024 14:43:16 +0000 (15:43 +0100)]
BUG/MEDIUM: quic: fix sending performance due to qc_prep_pkts() return
qc_prep_pkts() is a QUIC transport level function which encodes one or
several datagrams in a buffer before sending them. It returns the number
of encoded datagram. This is especially important when pacing is used to
limit packet bursts.
This datagram accounting was not trivial as qc_prep_pkts() used several
code paths depending on the condition of the current encoded packet.
Thus, there were several places were the local variable dgram_cnt could
have been incremented. This was implemented by the following commit :
However, there is a bug due to a missing increment when all frames from
the current QEL have been encoded. In this case, the encoding continue
in the same datagram to coalesce a futur packet. However, if this is the
last QEL, encoding loop will then break. As first_pkt is not NULL,
qc_txb_store() is called outside but dgram_cnt is yet not incremented.
In particular, this causes qc_prep_pkts() to return 0 when there is only
small STREAM frames to emit for application QEL. In qc_send(), this is
interpreted as a value which prevents further emission for the current
invokation. Thus, it may hurts performance, both without and with
pacing.
To fix this, removing multiple dgram_cnt increment. Now, it is modified
only in a single place which should cover every case, and render the
code easier to validate.
The most notable case where the bug is visible is when using cubic with
pacing without any burst, with quic-cc-algo cubic(,1). First, transfer
bandwidth in average was suboptimal, with significant variation. Worst,
it could sometimes fall dramatically for a particular stream without
recovering before returning to an expected level on the next one.
Amaury Denoyelle [Thu, 21 Nov 2024 14:18:41 +0000 (15:18 +0100)]
BUG/MINOR: mux-quic: fix show quic report of QCS prepared bytes
On show quic, each MUX streams are listed with their various indicator
for buffering on Rx and Tx. In particular, txoff displays in parenthesis
the current level of data prepared by the upper stream instance not yet
emitted by QUIC transport layer.
This value is only accessible after a substract operation. However,
there was a typo which caused the result to be always 0. Fix this by
reusing the correct offsets in the calculation.
projects / thirdparty / haproxy.git / log
