]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
5 months agoBUG/MEDIUM: mux-fcgi: Properly handle read0 on partial records
Christopher Faulet [Mon, 27 Jan 2025 14:18:14 +0000 (15:18 +0100)] 
BUG/MEDIUM: mux-fcgi: Properly handle read0 on partial records

A Read0 event could be ignored by the FCGI multiplexer if it is blocked on a
partial record. Instead of handling the event, it remained blocked, waiting
for the end of the record.

To fix the issue, the same solution than the H2 multiplexer is used. Two
flags are introduced. The first one, FCGI_CF_END_REACHED, is used to
acknowledge a read0. This flag is set when a read0 was received AND the FCGI
multiplexer must handle it. The second one, FCGI_CF_DEM_SHORT_READ, is set
when the demux is interrupted on a partial record. A short read and a read0
lead to set the FCGI_CF_END_REACHED flag.

With these changes, the FCGI mux should be able to properly handle read0 on
partial records.

This patch should be backported to all stable versions after a period of
observation.

5 months agoMEDIUM: htx: prevent <mark> to copy incomplete headers in htx_xfer_blks()
William Lallemand [Fri, 31 Jan 2025 14:31:00 +0000 (15:31 +0100)] 
MEDIUM: htx: prevent <mark> to copy incomplete headers in htx_xfer_blks()

Prevent a partial copy of trailers or headers when using the <mark>
parameter.

When using htx_xfer_blks(), transfering partial headers or trailers are
prevented when restricted by the <count> parameter. However using the
<mark> parameter will still allow to do it.

This patch changes the behavior by checking the <mark> type only after
checking the headers/trailers type, so we can still rollback on partial
transfer.

No impact on the current code, which does not try to do that yet.

5 months agoBUILD: quic: remove GCC undefined error in qc_release_lost_pkts()
Amaury Denoyelle [Thu, 30 Jan 2025 15:24:10 +0000 (16:24 +0100)] 
BUILD: quic: remove GCC undefined error in qc_release_lost_pkts()

Every once in a while, GCC reports issues with qc_release_lost_pkts()
function. It seems that its static analysis is foiled by the code
structuring. The latest warning reports the following issue :

  CC      src/quic_loss.o
src/quic_loss.c: In function ‘qc_release_lost_pkts’:
src/quic_loss.c:313:58: error: potential null pointer dereference [-Werror=null-dereference]
  313 |                         unsigned int period = newest_lost->time_sent_ms - oldest_lost->time_sent_ms;
      |                                               ~~~~~~~~~~~^~~~~~~~~~~~~~

To fix definitely this, change slightly the code. <oldest_lost> and
<newest_lost> are now initialized on the first list entry outside of the
loop. This is enough to guarantee to GCC that they cannot be NULL for
the remainder of the function.

5 months agoDOC: htx: clarify <mark> parameter for htx_xfer_blks()
William Lallemand [Fri, 31 Jan 2025 14:23:47 +0000 (15:23 +0100)] 
DOC: htx: clarify <mark> parameter for htx_xfer_blks()

Clarify the fact that the first <mark> block is transferred before
stopping when using htx_xfer_blks()

5 months agoBUG/MEDIUM: htx: wrong count computation in htx_xfer_blks()
William Lallemand [Fri, 31 Jan 2025 13:41:28 +0000 (14:41 +0100)] 
BUG/MEDIUM: htx: wrong count computation in htx_xfer_blks()

When transfering blocks from an src to another dst htx representation,
htx_xfer_blks() decreases the size of each block removed from the <count>
value passed in parameter, so it can't transfer more than <count>. The
size must also contains the metadata, represented by a simple
sizeof(struct htk_blk).

However, the code was doing a sizeof(dstblk) instead of a
sizeof(*dstblk) which as the consequence of removing only a size_t from
count. Fortunately htx_blk size is 64bits, so that does not provoke any
problem in 64bits. But on 32bits architecture, the count value is not
decreased correctly and the function could try to transfer more blocks
than allowed by the count parameter.

Must be backported in every stable release.

5 months agoMINOR: tevt/dev: Parse tuple of termination events
Christopher Faulet [Thu, 30 Jan 2025 15:19:48 +0000 (16:19 +0100)] 
MINOR: tevt/dev: Parse tuple of termination events

term_events tool is now able to parse tuple of termination events, as returned
by "term_events" sample fetch function.

5 months agoMINOR: tevt/connection: Add support for POLL_HUP/POLL_ERR events
Christopher Faulet [Thu, 30 Jan 2025 10:31:59 +0000 (11:31 +0100)] 
MINOR: tevt/connection: Add support for POLL_HUP/POLL_ERR events

Connection errors can be detected via connect/recv/send syscall, but also
because it was reported by the poller. So dedicated events, at the FD level,
are introduced to make the difference.

term_events tool was updated accordingly.

5 months agoMINOR: tevt/dev: Add term_events tool
Christopher Faulet [Tue, 21 Jan 2025 17:25:25 +0000 (18:25 +0100)] 
MINOR: tevt/dev: Add term_events tool

This development tool can be used to convert a string representing a
termination event logs to its human redable representation. Several string
may be converting at a time. To do so, several arguments can be specified on
the commeand line or they can be provided on STDIN, using "-" argument.

Here is an exemple:

  > term_events f2x2f4x4 m2m4m1 e2e1 s2s1S1 E1 M1 F1
  ### f2x2f4x4 : fd:shutr > xprt:shutr > fd:snd_err > xprt:snd_err
  ### m2m4m1   : muxc:shutr > muxc:snd_err > muxc:shutw
  ### e2e1     : se:eos > se:shutw
  ### s2s1S1   : strm:eos > strm:shutw > STRM:shutw
  ### E1       : SE:shutw
  ### M1       : MUXC:shutw
  ### F1       : FD:shutw

The make target "dev/term_events/term_events" must be used to compile it.

5 months agoREORG: tevt/connection: Move enums at the end of the header file
Christopher Faulet [Tue, 21 Jan 2025 17:19:50 +0000 (18:19 +0100)] 
REORG: tevt/connection: Move enums at the end of the header file

Enums used to report events were placed in the connection header for
conveniance. But it is not specifically related to connection. So, they are
moved at the end of the file to have a better isolation.

5 months agoMINOR: tevt: Improve function to convert a termination events log to string
Christopher Faulet [Tue, 21 Jan 2025 17:16:27 +0000 (18:16 +0100)] 
MINOR: tevt: Improve function to convert a termination events log to string

The function is now responsible to handle empty log because no event was
reported. In that case, an empty string is returned. It is also responsible to
handle case where termination events log is not supported for an given entity
(for instance the quic mux for now). In that case, a dash ("-") is returned.

5 months agoMINOR: tevt: Add a sample to get termination events for all locations
Christopher Faulet [Tue, 21 Jan 2025 06:46:17 +0000 (07:46 +0100)] 
MINOR: tevt: Add a sample to get termination events for all locations

"term_events" is a sample fetche function that can be used to get
termination events for all locations in one call. The format equivalent to:

  {fc_term_events,fc_mux_term_events,fs.term_events,txn.term_events,bs.term_events,bc_mux_term_events,bc_term_events}

If no event was reported for a location, the field is empty. If the feature
is not supported yet, a dash ('-') is printed.

5 months agoMINOR: tevt/applet: Add limited support for termination event logs for applets
Christopher Faulet [Tue, 21 Jan 2025 06:41:33 +0000 (07:41 +0100)] 
MINOR: tevt/applet:  Add limited support for termination event logs for applets

There is no termination events log for applet but events for the SE location
are filled when the endpoint is an applet. Most of them relies on the new
applet API. Only few events are reported for legacy applets.

5 months agoMINOR: tevt: Don't duplicate termination event during reporting
Christopher Faulet [Mon, 20 Jan 2025 14:35:47 +0000 (15:35 +0100)] 
MINOR: tevt: Don't duplicate termination event during reporting

It is hard to never detect the same event several time without painful
tests. In other words, the same termination event can be reported several
time and this must be handled. To do so, "tevt_report_event" macro is
updated to ignore an event if the last reported one is of the same type, for
the same location. Of course, if the same event is reported several times at
different moment, it will not be detected.

5 months agoMEDIUM: tevt/stconn/stream: Add dedicated termination events for stream location
Christopher Faulet [Mon, 20 Jan 2025 08:00:05 +0000 (09:00 +0100)] 
MEDIUM: tevt/stconn/stream: Add dedicated termination events for stream location

If it is the last patch to introduce dedicated termination events for each
location. In this one, events for the stream location are introcued. The old
enum is also removed because it is now unused.

Here, more accurate evets are added. The "intercepted" event was splitted.

5 months agoMINOR: tevt/stconn: Be more accurate to report shutw events
Christopher Faulet [Mon, 20 Jan 2025 07:57:55 +0000 (08:57 +0100)] 
MINOR: tevt/stconn: Be more accurate to report shutw events

In se_shutdown() a SE termination event is reported while the shutw stream
event is reported in sc_app_shut_conn().

5 months agoMEDIUM: tevt/muxes: Add dedicated termination events for muxc/se locations
Christopher Faulet [Mon, 20 Jan 2025 07:52:46 +0000 (08:52 +0100)] 
MEDIUM: tevt/muxes: Add dedicated termination events for muxc/se locations

Termination events dedicated to mux connection and stream-endpoint
descriptors are added in this patch. Specific events to these locations are
thus added. Changes for the H1 and H2 multiplexers are reviewed to be more
accurate.

5 months agoMINOR: tevt/connection: Add dedicated termination events for lower locations
Christopher Faulet [Mon, 20 Jan 2025 07:35:36 +0000 (08:35 +0100)] 
MINOR: tevt/connection: Add dedicated termination events for lower locations

To be able to add more accurate termination events for each location, the
enum will be splitted by location. Indeed, there are at most 16 possbile
events. It will be pretty confusing to use same termination events for the
different locations. So the best is to split them.

In this patch, the termination events for the fd, hs and xprt locations are
introduced. For now some holes are added to keep similar events aligned
across enums. But this may change in future.

5 months agoMINOR: tevt/mux-pt: Add support for termination event logs
Christopher Faulet [Mon, 23 Dec 2024 14:48:36 +0000 (15:48 +0100)] 
MINOR: tevt/mux-pt: Add support for termination event logs

A termination event logs is added to the mux-pt context and appropriate
events are reported for the muxc location. There is no SE events for this
mux.

5 months agoMINOR: tevt/muxes: Add CTL and SCTL command to get the termination event logs
Christopher Faulet [Mon, 23 Dec 2024 13:56:33 +0000 (14:56 +0100)] 
MINOR: tevt/muxes: Add CTL and SCTL command to get the termination event logs

MUX_CTL_TEVTS command is added to get the termination event logs of a mux
connection and MUX_SCTL_TEVTS command to get the termination event logs of a
mux stream.

5 months agoMINOR: tevt/mux-h1/mux-h2: Add termination events log when dumping mux info
Christopher Faulet [Mon, 23 Dec 2024 13:52:15 +0000 (14:52 +0100)] 
MINOR: tevt/mux-h1/mux-h2: Add termination events log when dumping mux info

The termiantion events logs of the multiplexer connection and stream are now
dumped when corresponding mux info are dumped. The termination event logs of
the underlying connection is also dumped in the debug string.

5 months agoMINOR: tevt/conn: Report intercepted event for L4 rules
Christopher Faulet [Mon, 23 Dec 2024 13:39:58 +0000 (14:39 +0100)] 
MINOR: tevt/conn: Report intercepted event for L4 rules

When a L4 rules interrupts the processing, a termination event is reported
for the connection, with the "fd" location.

5 months agoMINOR: tevt/stream/stconn: Report termination events for stream and sc
Christopher Faulet [Mon, 23 Dec 2024 13:30:33 +0000 (14:30 +0100)] 
MINOR: tevt/stream/stconn: Report termination events for stream and sc

In this patch, events for the stream location are reported. These events are
first reported on the corresponding stream-connector. So front events on scf
and back event on scb. Then all events are both merged in the stream. But
only 4 events are saved on the stream.

Several internal events are for now grouped with the type
"tevt_type_intercepted". More events will be added to have a better
resolution. But at least the place to report these events are identified.

For now, when a event is reported on a SC, it is also reported on the stream
and vice versa.

5 months agoMINOR: tevt/mux-h2: Report termination events for the H2C
Christopher Faulet [Mon, 23 Dec 2024 13:25:05 +0000 (14:25 +0100)] 
MINOR: tevt/mux-h2: Report termination events for the H2C

shutdown for reads (read0), receive errors, shutdown for writes and timeouts
are reported, but only for the H2 connection for now.

As for the H1 multiplexer, more events must be added to report protocol
errors, goaways and rst-streams. And of course, all events for the H2
streams must be reported too.

5 months agoMINOR: tevt/mux-h1: Report termination events for the H1C and H1S
Christopher Faulet [Mon, 23 Dec 2024 10:27:25 +0000 (11:27 +0100)] 
MINOR: tevt/mux-h1: Report termination events for the H1C and H1S

shutdown for reads (read0), receive errors, shutdown for writes and timeouts
are reported. It is not too hard to know where to report events generated by
HAProxy (timeouts and shutw). For detected events (shutr and receive error),
it is not so simple. These events must not be reported when they are
detected but when the mux can handle them. For instance, some unprocessed
input data may block a read0. So, the experience will tell us if these
events are reported at the rigth time and on the right conditions.

For now, no internal errors (parsing errors, protocol errors, intenral
errors...) are reported because these event types have not yet been added.

5 months agoMINOR: tevt/stconn: Add a termination events log in the SE descriptor
Christopher Faulet [Mon, 23 Dec 2024 10:24:29 +0000 (11:24 +0100)] 
MINOR: tevt/stconn: Add a termination events log in the SE descriptor

This termination events log will be used to report events from the mux
streams. The location will be "tevt_loc_se" and the muxes will be
responsible to report the corresponding events.

5 months agoMINOR: tevt: Add the termination events log's fundations
Christopher Faulet [Mon, 23 Dec 2024 09:43:54 +0000 (10:43 +0100)] 
MINOR: tevt: Add the termination events log's fundations

Termination events logs will be used to report the events that led to close
a connection. Unlike flags, that reflect a state, the idea here is to store
a log to preserve the order of the events. Most of time, when debugging an
issue, the order of the events is crucial to be able to understand the root
cause of the issue. The traces are trully heplful to do so. But it is not
always possible to active them because it is pretty verbose. On heavily
loaded platforms, it is not acceptable. We hope that the termination events
logs will help us in that situations.

One termination events log will be be store at each layer (connection, mux
connection, mux stream...) as a 32-bits integer. Each event will be store on
8 bits, 4 bits for the location and 4 bits for the type. So the first four
events will be stored only for each layer. It should be enough why a
connection is closed.

In this patch, the enums defining the termination event locations and types
are added. The macro to report a new event is also added and a function to
convert a termination events log to a string that could be display in log
messages for instance.

5 months agoBUG/MINOR: mux-h1: Only report a SE error on demux error
Christopher Faulet [Mon, 23 Dec 2024 10:42:08 +0000 (11:42 +0100)] 
BUG/MINOR: mux-h1: Only report a SE error on demux error

When a demux error is reported by the H1S, an error must be reported on the
SE and not an end-of-input or an end-of-stream. So SE_FL_ERROR flag must be
set and not SE_FL_EOI/SE_FL_EOS.

It seems this bug has no impact. So there is no reason to backport it.

5 months agoMINOR: mux-h1: Add masks to group H1S DEMUX and MUX errors
Christopher Faulet [Mon, 23 Dec 2024 10:20:35 +0000 (11:20 +0100)] 
MINOR: mux-h1: Add masks to group H1S DEMUX and MUX errors

It is just a small patch to clean up mux/demux functions. Instead of listing
the H1S errors that must be handled during demux of mux operations, masks of
flags are used. It is more readable.

5 months agoMEDIUM: epoll: skip reports of stale file descriptors
Willy Tarreau [Thu, 30 Jan 2025 15:32:35 +0000 (16:32 +0100)] 
MEDIUM: epoll: skip reports of stale file descriptors

Now that we can see that some events are reported for older instances
of a file descriptor, let's skip these ones instead of reporting
dangerous events on them. It might possibly qualify as a bug if it
helps fixing strange issues in certain environments, in which case it
can make sense to backport it along with the following recent patches:

  DEBUG: fd: add a counter of takeovers of an FD since it was last opened
  MINOR: fd: add a generation number to file descriptors
  DEBUG: epoll: store and compare the FD's generation count with reported event

5 months agoDEBUG: epoll: store and compare the FD's generation count with reported event
Willy Tarreau [Thu, 30 Jan 2025 15:28:33 +0000 (16:28 +0100)] 
DEBUG: epoll: store and compare the FD's generation count with reported event

There have been some reported cases where races between threads in epoll
were causing wrong reports of close or error events. Since the epoll_event
data is 64 bits, we can store the FD's generation counter in the upper
bits to verify if we're speaking about the same instance of the FD as the
current one or a stale one. If the generation number does not match, then
we classify these into 3 conditions and increment the relevant COUNT_IF()
counters (stale report for closed FD, stale report of harmless event on
reopened FD, stale report of HUP/ERR on reopened FD). Tests have shown that
with heavy concurrency, a very small maxconn (typically 1 per thread),
http-reuse always and a server closing connections first but randomly
(httpterm with /C=2r), such events can happen at a pace of a few per second
for the closed FDs, and a few per minute for the other ones, so there's value
in leaving this accessible for troubleshooting. E.g after a few minutes:

  Count     Type Location function(): "condition" [comment]
  5541       CNT ev_epoll.c:296 _do_poll(): "1" [epoll report of event on a just closed fd (harmless)]
  10         CNT ev_epoll.c:294 _do_poll(): "1" [epoll report of event on a closed recycled fd (rare)]
  42         CNT ev_epoll.c:289 _do_poll(): "1" [epoll report of HUP on a stale fd reopened on the same thread (suspicious)]
  212        CNT ev_epoll.c:279 _do_poll(): "1" [epoll report of HUP/ERR on a stale fd reopened on another thread (harmless)]
  1          CNT mux_h1.c:3911 h1_send(): "b_data(&h1c->obuf)" [connection error (send) with pending output data]

This one with the following setup, whicih abuses threads contention by
starting 64 threads on two cores:
- config:
    global
        nbthread 64
        stats socket /tmp/sock1 level admin
        stats timeout 1h
    defaults
        timeout client 5s
        timeout server 5s
        timeout connect 5s
        mode http
    listen p2
        bind :8002
        http-reuse always
        server s1 127.0.0.1:8000 maxconn 4

- haproxy forcefully started on 2C4T:

    $ taskset -c 0,1,4,5 ./haproxy -db -f epoll-dbg.cfg

- httpterm on port 8000, cpus 2,3,6,7 (2C4T)

- h1load with responses larger than a single buffer, and randomly
  closing/keeping alive:

    $ taskset -c 2,3,6,7 h1load -e -t 4 -c 256 -r 1 0:8002/?s=19k/C=2r

5 months agoMINOR: fd: add a generation number to file descriptors
Willy Tarreau [Thu, 30 Jan 2025 15:25:40 +0000 (16:25 +0100)] 
MINOR: fd: add a generation number to file descriptors

This patch adds a counter of close() on file descriptors in the fdtab.
The goal is to better detect if reported events concern the current or
a previous file descriptor. For now the counter is only added, and is
showed in "show fd" as "gen". We're reusing unused space at the end of
the struct. If it's needed for something more important later, this
patch can be reverted.

5 months agoDEBUG: fd: add a counter of takeovers of an FD since it was last opened
Willy Tarreau [Thu, 30 Jan 2025 14:59:11 +0000 (15:59 +0100)] 
DEBUG: fd: add a counter of takeovers of an FD since it was last opened

That's essentially in order to help with debugging strange cases like
the occasional epoll issues/races, by keeping a counter of how many
times an FD was taken over since last inserted. The room is available
so let's use it. If it's needed later, this patch can easily be reverted.
The counter is also reported in "show fd" as "tkov".

5 months agoBUILD: quic: fix overflow in global tune
Amaury Denoyelle [Thu, 30 Jan 2025 17:01:53 +0000 (18:01 +0100)] 
BUILD: quic: fix overflow in global tune

A new global option was recently introduced to disable pacing. However,
the value used (1<<31) caused issue with some compiler as options field
used for storage is declared as int. Move pacing deactivation flag
outside into the newly defined quic_tune to fix this.

This should be backported up to 3.1 after a period of observation. Note
that it relied on the previous patch which defined new quic_tune type.

5 months agoMINOR: quic: define quic_tune
Amaury Denoyelle [Thu, 30 Jan 2025 16:58:20 +0000 (17:58 +0100)] 
MINOR: quic: define quic_tune

Define a new structure quic_tune. It will be useful to regroup various
configuration settings and tunable related to QUIC, instead of defining
them into the global structure.

5 months agoMINOR: quic: mark BBR as stable
Amaury Denoyelle [Thu, 30 Jan 2025 13:57:27 +0000 (14:57 +0100)] 
MINOR: quic: mark BBR as stable

Pacing has recently been moved out of experimental status and is
activated by default. This is a mandatory requirement for BBR.
Furthermore, BBR is now considered stable. As such, removes its
experimental status with this commit.

5 months agoMAJOR: quic: mark pacing as stable and enable it by default
Amaury Denoyelle [Thu, 30 Jan 2025 13:56:35 +0000 (14:56 +0100)] 
MAJOR: quic: mark pacing as stable and enable it by default

Remove pacing experimental status, so it's not required anymore to use
expose-experimental-directives to enable it.

Along this change, pacing is now activated by default. As such, pacing
configuration is transformed into its final form. The global on/off
setting is turned into a disable setting without argument.

5 months agoMINOR: quic: transform pacing settings into a global option
Amaury Denoyelle [Thu, 30 Jan 2025 13:50:19 +0000 (14:50 +0100)] 
MINOR: quic: transform pacing settings into a global option

Pacing support was previously activated on each bind line individually,
via an optional argument of quic-cc-algo keyword. Remove this optional
argument and introduce a global setting to enable/disable pacing. Pacing
activation is still flagged as experimental.

One important change is that previously BBR usage automatically
activated pacing support. This is not the case anymore, so users should
now always explicitely activate pacing if BBR is selected. A new warning
message will be displayed if this is not the case.

Another consequence of this change is that now pacing_inter callback is
always defined for every quic_cc_algo types. As such, QUIC MUX uses
global.tune.options to determine if pacing is required.

This should be backported up to 3.1, after a period of observation.

5 months agoMINOR: quic: allow BBR testing without pacing
Amaury Denoyelle [Thu, 30 Jan 2025 10:58:27 +0000 (11:58 +0100)] 
MINOR: quic: allow BBR testing without pacing

Pacing is activated per bind line via an optional boolean argument of
quic-cc-algo keyword. Contrary to the default usage, pacing is
automatically activated when BBR is chosen. This is because this
algorithm is expected to run on top of pacing, else its behavior is
undefined.

Previously, pacing argument was thus ignored when BBR was selected.
Change this to support explicit deactivation of pacing with it. This
could be useful to test BBR without pacing when debugging some issues.

This should be backported up to 3.1, after a period of observation.

5 months agoMINOR: quic: remove references to burst in quic-cc-algo parsing
Amaury Denoyelle [Thu, 30 Jan 2025 10:57:55 +0000 (11:57 +0100)] 
MINOR: quic: remove references to burst in quic-cc-algo parsing

Pacing activation configuration has been recently revamped. Previously,
pacing related quic-cc-algo argument was used to specify a burst size.
It evolved into a boolean value as burst size is dynamically calculated
now. As such, removes any references to the old burst value in config
parsing code for cleaner code.

This should be backported up to 3.1, after a period of observation.

5 months agoBUG/MEDIUM: chunk: make sure to flush the trash pool before resizing
Willy Tarreau [Wed, 29 Jan 2025 16:51:12 +0000 (17:51 +0100)] 
BUG/MEDIUM: chunk: make sure to flush the trash pool before resizing

Late in 3.1 we've added an integrity check to make sure we didn't keep
trash objects allocated before resizing the trash with commit 0bfd36e7b8
("MINOR: chunk: add a BUG_ON upon the next init_trash_buffer()"), but
it turns out that the counter that is being checked includes the number
of objects left in local thread caches. As such it can trigger despite
no object being allocated. This precisely happens when setting
tune.memory.hot-size to a few megabytes because some temporarily used
trash objects will remain in cache.

In order to address this, let's first flush the pool before running
the check. That was previously done by pool_destroy() but the check
had to be inserted before it. So now we first flush the trash pool,
then verify it's no longer used, and finally we can destroy it.

This needs to be backported to 3.1. Thanks to Christian Ruppert for
reporting this bug.

5 months agoBUILD: ssl: more cleaner approach to WolfSSL without renegotiation
William Lallemand [Tue, 28 Jan 2025 19:55:20 +0000 (20:55 +0100)] 
BUILD: ssl: more cleaner approach to WolfSSL without renegotiation

Patch discussed in https://github.com/wolfSSL/wolfssl/issues/6834

When building Wolfssl without renegotiation options, WolfSSL still
defines the macros about it, which warns during the build.

This patch completes the previous one by undefining the macros so
haproxy could build without any warning.

5 months agoBUILD: ssl: allow to build without the renegotiation API of WolfSSL
William Lallemand [Tue, 28 Jan 2025 17:27:31 +0000 (18:27 +0100)] 
BUILD: ssl: allow to build without the renegotiation API of WolfSSL

In ticket https://github.com/wolfSSL/wolfssl/issues/6834, it was
suggested to push --enable-haproxy within --enable-distro.

WolfSSL does not want to include the renegotiation support in
--enable-distro.

To achieve this, let haproxy build without SSL_renegotiate_pending()
when wolfssl does not define HAVE_SECURE_RENEGOCIATION or
HAVE_SERVER_RENEGOCIATION_INFO.

5 months agoBUILD: queues: Use unsigned int when needed
Olivier Houchard [Tue, 28 Jan 2025 17:03:11 +0000 (18:03 +0100)] 
BUILD: queues: Use unsigned int when needed

Use unsigned int instead of int when calculating which thread group we
should dequeue from next, as the difference in signedness makes clang
unhappy.

5 months agoMINOR: queues: use __ha_cpu_relax() on failed CAS.
Olivier Houchard [Tue, 28 Jan 2025 15:18:40 +0000 (16:18 +0100)] 
MINOR: queues: use __ha_cpu_relax() on failed CAS.

Make sure we call __ha_cpu_relax() if we fail a CAS, to help with
contention.

5 months agoBUILD: tools: fix build on BSD by dropping the ETIME check
Willy Tarreau [Tue, 28 Jan 2025 14:58:57 +0000 (15:58 +0100)] 
BUILD: tools: fix build on BSD by dropping the ETIME check

Commit 44537379fc ("MINOR: tools: add errname to print errno macro
name") brought a facility to report errno using a symbolic string
when known instead of showing only the value. However, among the
listed options, ETIME is mentioned but is unknown from FreeBSD where
it breaks the build. Let's simply drop it, we don't use ETIME anyway
and even if it would be reported, the default code path still reports
the numeric value so there's no harm. If other ones fail to build in
the future, they could be handled the same way.

5 months agoMEDIUM: stream: No longer use TASK_F_UEVT* to shut a stream down
Christopher Faulet [Mon, 27 Jan 2025 18:11:02 +0000 (19:11 +0100)] 
MEDIUM: stream: No longer use TASK_F_UEVT* to shut a stream down

Thanks to the previous patch, it is now possible to explicitly rely on
stream's events to shut it down. The right event is set in
stream_shutdown(), before waking up the stream, via an atomic operation. In
process_stream(), this event will be handled as expected.

Thus, TASK_F_UEVT* are no longer used, but not removed since still usable
for other tasks.

This patch depends on "MEDIUM: stream: Map task wake up reasons to dedicated
stream events".

5 months agoMEDIUM: stream: Map task wake up reasons to dedicated stream events
Christopher Faulet [Mon, 27 Jan 2025 17:38:40 +0000 (18:38 +0100)] 
MEDIUM: stream: Map task wake up reasons to dedicated stream events

To fix thread-safety issues when a stream must be shut, three new task
states were added. These states are generic (UEVT1, UEVT2 and UEVT3), the
task callback function is responsible to know what to do with them. However,
it is not really scalable.

The best is to use an atomic field in the stream structure itself to deal
with these dedicated events. There is already the "pending_events" field
that save wake up reasons (TASK_WOKEN_*) to not loose them if
process_stream() is interrupted before it had a chance to handle them.

So the idea is to introduce a new field to handle streams dedicated events
and merged them with the task's wake up reasons used by the stream. This
means a mapping must be performed between some task wake up reasons and
streams events. Note that not all task wake up reasons will be mapped.

In this patch, the "new_events" field is introduced. It is an atomic
bit-field. Streams events (STRM_EVT_*) are also introduced to map the task
wake up reasons used by process_stream(). Only TASK_WOKEN_TIMER and
TASK_WOKEN_MSG are mapped, in addition to TASK_F_UEVT* flags. In
process_stream(), "pending_events" field is now filled with new stream
events and the mapping of the wake up reasons.

5 months agoBUG/MINOR: stream: Properly handle "on-marked-up shutdown-backup-sessions"
Christopher Faulet [Mon, 27 Jan 2025 15:17:27 +0000 (16:17 +0100)] 
BUG/MINOR: stream: Properly handle "on-marked-up shutdown-backup-sessions"

shutdown-backup-sessions action for on-marked-up directive does not work anymore
since the stream_shutdown() function was modified to be async-safe.

When stream_shutdown() was modified to be async-safe, dedicated task events were
added to map the reasons to shut a stream down. SF_ERR_DOWN was mapped to
TASK_F_EVT1 and SF_ERR_KILLED was mapped to TASK_F_EVT2. The reverse mapping was
performed by process_stream() to shut the stream with the appropriate reason.

However, SF_ERR_UP reason, used by shutdown-backup-sessions action to shut a
stream down because a preferred server became available, was not mapped in the
same way. So since commit b8e3b0a18d ("BUG/MEDIUM: stream: make
stream_shutdown() async-safe"), this action is ignored and does not work
anymore.

To fix an issue, and being able to bakcport the fix, a third task event was
added. TASK_F_EVT3 is now mapped on SF_ERR_UP.

This patch should fix the issue #2848. It must be backported as far as 2.6.

5 months agoMEDIUM: servers/proxies: Switch to using per-tgroup queues.
Olivier Houchard [Wed, 15 Jan 2025 15:44:05 +0000 (16:44 +0100)] 
MEDIUM: servers/proxies: Switch to using per-tgroup queues.

For both servers and proxies, use one connection queue per thread-group,
instead of only one. Having only one can lead to severe performance
issues on NUMA machines, it is actually trivial to get the watchdog to
trigger on an AMD machine, having a server with a maxconn of 96, and an
injector that uses 160 concurrent connections.
We now have one queue per thread-group, however when dequeueing, we're
dequeuing MAX_SELF_USE_QUEUE (currently 9) pendconns from our own queue,
before dequeueing one from another thread group, if available, to make
sure everybody is still running.

5 months agoMINOR: proxies/servers: Calculate queueslength and use it.
Olivier Houchard [Wed, 15 Jan 2025 15:37:35 +0000 (16:37 +0100)] 
MINOR: proxies/servers: Calculate queueslength and use it.

For both proxies and servers, properly calculates queueslength, which is
the total number of element in each queues (as they currently are only
using one queue, it is equivalent to the number of element of that
queue), and use it instead of the queue's length.

5 months agoMINOR: Add fields to the per-thread group field in struct server.
Olivier Houchard [Wed, 15 Jan 2025 15:16:18 +0000 (16:16 +0100)] 
MINOR: Add fields to the per-thread group field in struct server.

Add a per-thread group queue and associated fields in per-thread group
field in struct server, as well as a new field, queues length.
This is currently unused, so should change nothing.

5 months agoMINOR: proxies: Add a per-thread group field to struct proxy.
Olivier Houchard [Wed, 15 Jan 2025 15:10:43 +0000 (16:10 +0100)] 
MINOR: proxies: Add a per-thread group field to struct proxy.

Add a per-thread group field to struct proxy, that will contain a struct
queue, as well as a new field, "queueslength".
This is currently unused, so should change nothing.
Please note that proxy_init_per_thr() must now be called for each proxy
once the thread groups number is known.

5 months agoMINOR: epoll: permit to mask certain specific events
Willy Tarreau [Mon, 27 Jan 2025 14:41:26 +0000 (15:41 +0100)] 
MINOR: epoll: permit to mask certain specific events

A few times in the past we've seen cases where epoll was caught reporting
a wrong event that caused trouble (e.g. spuriously reporting HUP or RDHUP
after a successful connect()). The new tune.epoll.mask-events directive
permits to mask events such as ERR, HUP and RDHUP and convert them to IN
events that are processed by the regular receive path. This should help
better diagnose and troubleshoot issues such as this one, as well as rule
out such a cause when similar issues are reported:

   https://github.com/haproxy/haproxy/issues/2368
   https://www.spinics.net/lists/netdev/msg876470.html

It should be harmless to backport this if necessary.

5 months agoCLEANUP: tree-wide: define and use acl_match_cond() helper
Aurelien DARRAGON [Wed, 22 Jan 2025 13:15:47 +0000 (14:15 +0100)] 
CLEANUP: tree-wide: define and use acl_match_cond() helper

acl_match_cond() combines acl_exec_cond() + acl_pass() and a check on the
condition->pol (to check if the cond is inverted) in order to return
either 0 if the cond doesn't match or 1 if it matches (or NULL).

Thanks to this we can actually simplify some redundant constructs that
iterate over rules and evaluate if the condition matches or not.

Conditions for tcp-request inspect-content and tcp-response
inspect-content couldn't be simplified because they perform an extra
check for missing data, and thus still need to leverage acl_exec_cond()

It's best to display the patch using "-w", like "git show xxxx -w",
because some blocks had to be re-indented after the cleanup, which
makes the patch hard to review by default.

5 months agoCLEANUP: ssl: move ssl_sock_gencert_load_ca declaration in ssl_gencert.h
Valentine Krasnobaeva [Thu, 23 Jan 2025 14:58:57 +0000 (15:58 +0100)] 
CLEANUP: ssl: move ssl_sock_gencert_load_ca declaration in ssl_gencert.h

As ssl_sock_gencert_load_ca and ssl_sock_gencert_free_ca are compiled only if
SSL_NO_GENERATE_CERTIFICATES is not defined, let's align it and move these
declarations in ssl_gencert.h.

5 months agoCLEANUP: ssl: rename ssl_sock_load_ca to ssl_sock_gencert_load_ca
Valentine Krasnobaeva [Thu, 23 Jan 2025 14:58:15 +0000 (15:58 +0100)] 
CLEANUP: ssl: rename ssl_sock_load_ca to ssl_sock_gencert_load_ca

ssl_sock_load_ca is defined in ssl_gencert.c and compiled only if
SSL_NO_GENERATE_CERTIFICATES is not defined. It's name is a bit confusing, as
we may think at the first glance, that it's a generic function, which is also
used to load CA file, provided via 'ca-file' keyword.
ssl_set_verify_locations_file is used in this case.

So let's rename ssl_sock_load_ca into ssl_sock_gencert_load_ca. Same is
applied to ssl_sock_free_ca.

5 months agoBUG/MINOR: ssl: put ssl_sock_load_ca under SSL_NO_GENERATE_CERTIFICATES
Valentine Krasnobaeva [Thu, 23 Jan 2025 12:46:46 +0000 (13:46 +0100)] 
BUG/MINOR: ssl: put ssl_sock_load_ca under SSL_NO_GENERATE_CERTIFICATES

ssl_sock_load_ca and ssl_sock_free_ca definitions are compiled only, if
SSL_NO_GENERATE_CERTIFICATES is not set. In case, when we set this define and
build haproxy, linker throws an error. So, let's fix this.

This should be backported in all stable versions.

5 months ago[RELEASE] Released version 3.2-dev4 v3.2-dev4
Willy Tarreau [Fri, 24 Jan 2025 10:01:06 +0000 (11:01 +0100)] 
[RELEASE] Released version 3.2-dev4

Released version 3.2-dev4 with the following main changes :
    - BUG/MINOR: stktable: fix big-endian compatiblity in smp_to_stkey()
    - MINOR: stktable: add stkey_to_smp() helper
    - MINOR: stktable: add stksess_getkey() helper
    - MINOR: stktable: add sc[0-2]_key fetches
    - BUG/MEDIUM: queues: Adjust the proxy counters when appropriate
    - MINOR: trace: add help message for -dt argument
    - MINOR: trace: ensure -dt priority over traces config section
    - MINOR: trace: support all source alias on -dt
    - BUG/MINOR: quic: reject NEW_TOKEN frames from clients
    - MINOR: stktable: fix potential build issue in smp_to_stkey
    - BUG/MEDIUM: stktable: fix missing lock on some table converters
    - BUG/MEDIUM: promex: Use right context pointers to dump backends extra-counters
    - MINOR: stktable: fix potential build issue in smp_to_stkey (2nd try)
    - MINOR: stktable: add smp_fetch_stksess() helper function
    - MEDIUM: stktable: split src-based key smp_fetch_sc functions
    - MEDIUM: stktable: split sc_ and src_ fetch lookup logics
    - MEDIUM: stktable: leverage smp_fetch_* helpers from sample conv
    - DOC: config: unify sample conv|fetches optional arguments syntax
    - DOC: config: stick-table converters support implicit <table> argument
    - DOC: config: stick-table converter do accept ANY-typed input
    - DOC: config: clarify return type for some stick-table converters
    - DOC: config: refer to canonical sticktable converters for src_* fetches
    - CLEANUP: stktable: move sample_conv_table_bytes_out_rate()
    - MINOR: stktable: add table_{inc,clr}_gpc* converters
    - BUG/MAJOR: quic: reject too large CRYPTO frames
    - BUG/MAJOR: log/sink: possible sink collision in sink_new_from_srv()
    - BUG/MINOR: init: set HAPROXY_STARTUP_VERSION from the variable, not the macro
    - REORG: version: move the remaining BUILD_* stuff from haproxy.c to version.c
    - BUG/MINOR: quic: ensure a detached coalesced packet can't access its neighbours
    - MINOR: quic: Add a BUG_ON() on quic_tx_packet refcount
    - BUILD: quic: Move an ASSUME_NONNULL() for variable which is not null
    - BUG/MEDIUM: mux-h1: Properly close H1C if an error is reported before sending data
    - CLEANUP: quic: remove unused prototype
    - MINOR: quic: rename pacing_rate cb to pacing_inter
    - BUG/MINOR: quic: do not increase congestion window if app limited
    - MINOR: mux-quic: increment pacing retry counter on expired
    - MEDIUM: quic: implement credit based pacing
    - MEDIUM: mux-quic: reduce pacing CPU usage with passive wait
    - MEDIUM: quic: use dynamic credit for pacing
    - MINOR: quic: remove unused pacing burst in bind_conf/quic_cc_path
    - MINOR: quic: adapt credit based pacing to BBR
    - MINOR: tools: add errname to print errno macro name
    - MINOR: debug: debug_parse_cli_show_dev: use errname
    - MINOR: debug: show boot and runtime process settings in table

5 months agoMINOR: debug: show boot and runtime process settings in table
Valentine Krasnobaeva [Sun, 22 Sep 2024 10:38:35 +0000 (12:38 +0200)] 
MINOR: debug: show boot and runtime process settings in table

Let's reformat output of "show dev" in order to show some boot and runtime
process settings in a table. This makes the output less crowded.

5 months agoMINOR: debug: debug_parse_cli_show_dev: use errname
Valentine Krasnobaeva [Sat, 21 Sep 2024 16:20:19 +0000 (18:20 +0200)] 
MINOR: debug: debug_parse_cli_show_dev: use errname

Let's use errname, introduced in the previous commit in the output of
"show dev". This output is destined to engineers. So, no need to provide a
long descriptions of errnos given by strerror.

5 months agoMINOR: tools: add errname to print errno macro name
Valentine Krasnobaeva [Fri, 20 Sep 2024 21:14:41 +0000 (23:14 +0200)] 
MINOR: tools: add errname to print errno macro name

Add helper to print the name of errno's corresponding macro, for example
"EINVAL" for errno=22. This may be helpful for debugging and for using in
some CLI commands output. The switch-case in errname() contains only the
errnos currently used in the code. So, it needs to be extended, if one starts
to use new syscalls.

5 months agoMINOR: quic: adapt credit based pacing to BBR
Amaury Denoyelle [Thu, 23 Jan 2025 14:24:09 +0000 (15:24 +0100)] 
MINOR: quic: adapt credit based pacing to BBR

Credit based pacing has been further refined to be able to calculate
dynamically burst size based on congestion parameter. However, BBR
algorithm already provides pacing rate and burst size (labelled as
send_quantum) for 1ms of emission.

Adapt quic_pacing_reload() to use BBR values to compute pacing credit.
This is done via pacing_burst callback which is now only defined for
BBR. For other algorithms, determine the burst size over 1ms with the
congestion window size and RTT.

This should be backported up to 3.1.

5 months agoMINOR: quic: remove unused pacing burst in bind_conf/quic_cc_path
Amaury Denoyelle [Thu, 23 Jan 2025 16:06:04 +0000 (17:06 +0100)] 
MINOR: quic: remove unused pacing burst in bind_conf/quic_cc_path

Pacing burst size is now dynamic. As such, configuration value has been
removed and related fields in bind_conf and quic_cc_path structures can
be safely removed.

This should be backported up to 3.1.

5 months agoMEDIUM: quic: use dynamic credit for pacing
Amaury Denoyelle [Wed, 22 Jan 2025 16:26:13 +0000 (17:26 +0100)] 
MEDIUM: quic: use dynamic credit for pacing

Major improvements have been introduced in pacing recently. Most
notably, QMUX schedules emission on a millisecond resolution, which
allow to use passive wait to be much CPU friendly.

However, an issue remains with the pacing max credit. Unless BBR is
used, it is fixed to the configured value from quic-cc-algo bind
statement. This is not practical as if too low, it may drastically
reduce performance due to 1ms sleep resolution. If too high, some
clients will suffer from too much packet loss.

This commit fixes the issue by implementing a dynamic maximum credit
value based on the network condition specific to each clients.
Calculation is done to fix a maximum value which should allow QMUX
current tasklet context to emit enough data to cover the delay with the
next tasklet invokation. As such, avg_loop_us is used to detect the
process load. If too small, 1.5ms is used as minimal value, to cover the
extra delay incurred by the system which will happen for a default 1ms
sleep.

This should be backported up to 3.1.

5 months agoMEDIUM: mux-quic: reduce pacing CPU usage with passive wait
Amaury Denoyelle [Wed, 22 Jan 2025 16:31:10 +0000 (17:31 +0100)] 
MEDIUM: mux-quic: reduce pacing CPU usage with passive wait

Pacing algorithm has been revamped in the previous commit to implement a
credit based solution. This is a far more adaptative solution, in
particular which allow to catch up in case pause between pacing emission
was longer than expected.

This allows QMUX to remove the active loop based on tasklet wake-up.
Instead, a new task is used when emission should be paced. The main
advantage is that CPU usage is drastically reduced.

New pacing task timer is reset each time qcc_io_send() is invoked. Timer
will be set only if pacing engine reports that emission must be
interrupted. In this case timer is set via qcc_wakeup_pacing() to the
delay reported by congestion algorithm, or 1ms if delay is too short. At
the end of qcc_io_cb(), pacing task is queued if timer has been set.

Pacing task execution is simple enough : it immediately wakes up QCC I/O
handler.

Note that to have decent performance, it requires to have a large enough
burst defined in configuration of quic-cc-algo. However, this value is
common to every listener clients, which may cause too much loss under
network conditions. This will be address in a future patch.

This should be backported up to 3.1.

5 months agoMEDIUM: quic: implement credit based pacing
Amaury Denoyelle [Fri, 10 Jan 2025 16:18:54 +0000 (17:18 +0100)] 
MEDIUM: quic: implement credit based pacing

Implement a new method for QUIC pacing emission based on credit. This
represents the number of packets which can be emitted in a single burst.
After emission, decrement from the credit the number of emitted packets.
Several emission can be conducted in the same sequence until the credit
is completely decremented.

When a new emission sequence is initiated (i.e. under a new QMUX tasklet
invokation), credit is refilled according to the delay which occured
between the last and current emission context.

This new mechanism main advantage is that it allows to conduct several
emission in the same task context without having to wait between each
invokation. Wait is only forced if pacing is expired, which is now
equivalent to having a null credit.

Furthermore, if delay between two emissions sequence would have been
smaller than expected, credit is only partially refilled. This allows to
restart emission without having to wait for the whole credit to be
available.

On the implementation side, a new field <credit> is avaiable in
quic_pacer structure. It is automatically decremented on
quic_pacing_sent_done() invokation. Also, a new function
quic_pacing_reload() must be used by QUIC MUX when a new emission
sequence is initiated to refill credit. <next> field from quic_pacer has
been removed.

For the moment, credit is based on the burst configured via quic-cc-algo
keyword, or directly reported by BBR.

This should be backported up to 3.1.

5 months agoMINOR: mux-quic: increment pacing retry counter on expired
Amaury Denoyelle [Fri, 10 Jan 2025 16:20:55 +0000 (17:20 +0100)] 
MINOR: mux-quic: increment pacing retry counter on expired

A field <paced_sent_ctr> from quic_pacer structure is used to report the
number of occurences where emission has been interrupted due to pacing.
However, it was not incremented when QUIC MUX had to pause immediately
emission as pacing was still not yet expired.

Fix this by incrementing <paced_sent_ctr> in qcc_io_send() prior to
emission if pacing is expired. Note that incrementation is only done
once if the tasklet is then repeatdely woken up until the timer is
expired.

This should be backported up to 3.1.

5 months agoBUG/MINOR: quic: do not increase congestion window if app limited
Amaury Denoyelle [Thu, 9 Jan 2025 16:26:37 +0000 (17:26 +0100)] 
BUG/MINOR: quic: do not increase congestion window if app limited

Previously, congestion window was increased any time each time a new
acknowledge was received. However, it did not take into account the
window filling level. In a network condition with negligible loss, this
will cause the window to be incremented until the maximum value (by
default 480k), even though the application does not have enough data to
fill it.

In most cases, this issue is not noticeable. However, it may lead to
excessive memory consumption when a QUIC connection is suddendly
interrupted, as in this case haproxy will fill the window with
retransmission. It even has caused OOM crash when thousands of clients
were interrupted at once on a local network benchmark.

Fix this by first checking window level prior to every incrementation
via a new helper function quic_cwnd_may_increase(). It was arbitrarily
decided that the window must be at least 50% full when the ACK is
handled prior to increment it. This value is a good compromise to keep
window in check while still allowing fast increment when needed.

Note that this patch only concerns cubic and newreno algorithm. BBR has
already its notion of application limited which ensures the window is
only incremented when necessary.

This should be backported up to 2.6.

5 months agoMINOR: quic: rename pacing_rate cb to pacing_inter
Amaury Denoyelle [Thu, 23 Jan 2025 09:28:57 +0000 (10:28 +0100)] 
MINOR: quic: rename pacing_rate cb to pacing_inter

Rename one of the congestion algorithms pacing callback from pacing_rate
to pacing_inter. This better reflects that this function returns a delay
(in nanoseconds) which should be applied between each packet emission to
fill the congestion window with a perfectly smoothed emission.

This should be backported up to 3.1.

5 months agoCLEANUP: quic: remove unused prototype
Amaury Denoyelle [Wed, 22 Jan 2025 15:16:17 +0000 (16:16 +0100)] 
CLEANUP: quic: remove unused prototype

Remove undefined quic_pacing_send() function prototype from quic_pacing
module.

This should be backported up to 3.1.

5 months agoBUG/MEDIUM: mux-h1: Properly close H1C if an error is reported before sending data
Christopher Faulet [Thu, 23 Jan 2025 09:31:41 +0000 (10:31 +0100)] 
BUG/MEDIUM: mux-h1: Properly close H1C if an error is reported before sending data

It is possible to have front H1 connections waiting for the client timeout
while they should be closed because a conneciton error was reported before
sebding an error message to the client. It is not a leak because the
connections are closed when the timeout expires but it is a waste of
ressources, especially if the client timeout is high.

When an early error message must be sent to the client, if an error was
already detected, no data are sent and the output buffer is released. At
this stage, the H1 connection is in CLOSING state and it must be
released. But because of a bug, this is not performed. The client timeout is
rearmed and the H1 connection is only closed when it expires.

To fix the issue, the condition to close a H1C must also be evaluated when
an error is detected before sending data.

It is only an issue with idle client connections, because there is no H1
stream in that case and the error message is generated by the mux itself.

This patch must be backported as far as 2.8.

5 months agoBUILD: quic: Move an ASSUME_NONNULL() for variable which is not null
Frederic Lecaille [Tue, 21 Jan 2025 15:26:42 +0000 (16:26 +0100)] 
BUILD: quic: Move an ASSUME_NONNULL() for variable which is not null

Some new compilers warn that <oldest_lost> variable can be null even this cannot be
the case as mentioned by the comment about an already present ASSUME_NONNULL()
call comment as follows:

src/quic_loss.c: In function ‘qc_release_lost_pkts’:
src/quic_loss.c:307:86: error: potential null pointer dereference [-Werror=null-dereference]
  307 |   unsigned int period = newest_lost->time_sent_ms - oldest_lost->time_sent_ms;
      |                                                     ~~~~~~~~~~~^~~~~~~~~~~~~~

Move up this ASSUME_NONNULL() statement to please these compiler.

Must be backported as far as 2.6 to easy any further backport around this code part.

5 months agoMINOR: quic: Add a BUG_ON() on quic_tx_packet refcount
Frederic Lecaille [Tue, 21 Jan 2025 15:12:05 +0000 (16:12 +0100)] 
MINOR: quic: Add a BUG_ON() on quic_tx_packet refcount

This is definitively a bug to call quic_tx_packet_refdec() to decrement the reference
counter of a TX packet calling quic_tx_packet_refdec(), and possibly to release its
memory when it is negative or null.

This counter is incremented when a TX frm is attached to it with some allocated memory
and when the packet is inserted into a data structure, if needed (list or tree).

Should be easily backported as far as 2.6 to ease any further backport around
this code part.

5 months agoBUG/MINOR: quic: ensure a detached coalesced packet can't access its neighbours
Frederic Lecaille [Tue, 21 Jan 2025 14:49:51 +0000 (15:49 +0100)] 
BUG/MINOR: quic: ensure a detached coalesced packet can't access its neighbours

Reset ->prev and ->next fields of a coalesced TX packet to ensure it cannot access
several times its neighbours after it is supposed to be detached from them calling
quic_tx_packet_dgram_detach().

There are two cases where a packet can be coalesced to another previous built one:
this is when it is built into the same datagrame without GSO (and flagged flag with
QUIC_FL_TX_PACKET_COALESCED) or when sent from the same sendto() syscall with GOS
(not flagged with QUIC_FL_TX_PACKET_COALESCED).

This fix may be in relation with GH #2839.

Must be backported as far as 2.6.

5 months agoREORG: version: move the remaining BUILD_* stuff from haproxy.c to version.c
Willy Tarreau [Mon, 20 Jan 2025 16:49:55 +0000 (17:49 +0100)] 
REORG: version: move the remaining BUILD_* stuff from haproxy.c to version.c

version.c tries to centralize all variables conveying version information,
but there's still an issue with the BUILD_* variables which are only
passed to haproxy.o and are only updated when that one is rebuilt. This
is not very logical given that we can end up with values there which
contradict info from version.c.

Better move all of these to version.c which is systematically rebuilt.
Most of these variables only end up as string concatenation at the
moment. Some of them are even duplicated. In version.c we now have one
variable (or constant) for each of them and haproxy.c references them
in messages. This is much more logical and easier to maintain in a
consistent state.

The patch looks a bit large but it really only moves the ifdefed string
assignment from one file to another, placing them into variables.

5 months agoBUG/MINOR: init: set HAPROXY_STARTUP_VERSION from the variable, not the macro
Willy Tarreau [Mon, 20 Jan 2025 16:21:19 +0000 (17:21 +0100)] 
BUG/MINOR: init: set HAPROXY_STARTUP_VERSION from the variable, not the macro

This environment variable was added by commit d4c0be6b20 ("MINOR: startup:
HAPROXY_STARTUP_VERSION contains the version used to start"). However, it's
set from the macro that is passed during the build process instead of being
set from the variable that's kept up to date in version.c. The difference
is visible only during debugging/bisecting because only changed files and
version.o are rebuilt, but not necessarily haproxy.o, which is where the
environment variable is set. This means that the version exposed in the
environment is not necessarily the same as the one presented in
"haproxy -v" during such debugging sessions.

This should be backported to 2.8. It has no impact at all on regularly
built binaries.

5 months agoBUG/MAJOR: log/sink: possible sink collision in sink_new_from_srv()
Aurelien DARRAGON [Mon, 20 Jan 2025 09:42:42 +0000 (10:42 +0100)] 
BUG/MAJOR: log/sink: possible sink collision in sink_new_from_srv()

sink_new_from_srv() leverages sink_new_buf() with the server id as name,
sink_new_buf() then calls __sink_new() with the provided name.

Unfortunately sink_new() is designed in such a way that it will first look
up in the list of existing sinks to check if a sink already exists with
given name, in which case the existing sink is returned. While this
behavior may be error-prone, it is actually up to the caller to ensure
that the provided name is unique if it really expects a unique sink
pointer.

Due to this bug in sink_new_from_srv(), multiple tcp servers with the same
name defined in distinct log backends would end up sharing the same sink,
which means messages sent to one of the servers would also be forwarded to
all servers with the same name across all log backend sections defined in
the config, which is obviously an issue and could even raise security
concerns.

Example:

  defaults
    log backend@log-1 local0

  backend log-1
    mode log
    server s1 127.0.0.1:514
  backend log-2
    mode log
    server s1 127.0.0.1:5114

With the above config, logs sent to log-1/s1 would also end up being sent
to log-2/s1 due to server id "s1" being used for tcp servers in distinct
log backends.

To fix the issue, we now prefix the sink ame with the backend name:
back_name/srv_id combination is known to be unique (backend name
serves as a namespace)

This bug was reported by GH user @landon-lengyel under #2846.

UDP servers (with udp@ prefix before the address) are not affected as they
don't make use of the sink facility.

As a workaround, one should manually ensure that all tcp servers across
different log backends (backend with "mode log" enabled) use unique names

This bug was introduced in e58a9b4 ("MINOR: sink: add sink_new_from_srv()
function") thus it exists since the introduction of log backends in 2.9,
which means this patch should be backported up to 2.9.

5 months agoBUG/MAJOR: quic: reject too large CRYPTO frames
Amaury Denoyelle [Mon, 20 Jan 2025 09:15:12 +0000 (10:15 +0100)] 
BUG/MAJOR: quic: reject too large CRYPTO frames

Received CRYPTO frames are inserted in a ncbuf to handle out-of-order
reception via ncb_add(). They are stored on the position relative to the
frame offset, minus a base offset which corresponds to the in-order data
length already handled.

Previouly, no check was implemented on the frame offset value prior to
ncb_add(), which could easily trigger a crash if relative offset was too
large. Fix this by ensuring first that the frame can be stored in the
buffer before ncb_add() invokation. If this is not the case, connection
is closed with error CRYPTO_BUFFER_EXCEEDED, as required by QUIC
specification.

This should fix github issue #2842.

This must be backported up to 2.6.

5 months agoMINOR: stktable: add table_{inc,clr}_gpc* converters
Aurelien DARRAGON [Wed, 15 Jan 2025 20:14:21 +0000 (21:14 +0100)] 
MINOR: stktable: add table_{inc,clr}_gpc* converters

As discussed in GH #2423, there are some cases where src_{inc,clr}_gpc*
is not sufficient because we need to perform the lookup on a specific
key. Indeed, just like we did in e642916 ("MEDIUM: stktable: leverage
smp_fetch_* helpers from sample conv"), we can easily implement new
table converters based on existing fetches. This is what we do in
this patch.

Also the doc was updated so that src_{inc,clr}_gpc* fetches now point to
their generic equivalent table_{inc,clr}_gpc*. Indeed, src_{inc,clr}_gpc*
are simply aliases.

This should fix GH #2423.

5 months agoCLEANUP: stktable: move sample_conv_table_bytes_out_rate()
Aurelien DARRAGON [Wed, 15 Jan 2025 20:14:48 +0000 (21:14 +0100)] 
CLEANUP: stktable: move sample_conv_table_bytes_out_rate()

sample_conv_table_bytes_out_rate() was defined in the middle of other
stick-table sample convs without any ordering logic. Let's put it
where it belongs, right after sample_conv_table_bytes_in_rate().

5 months agoDOC: config: refer to canonical sticktable converters for src_* fetches
Aurelien DARRAGON [Wed, 15 Jan 2025 16:59:24 +0000 (17:59 +0100)] 
DOC: config: refer to canonical sticktable converters for src_* fetches

When available, to prevent doc duplication, let's make src_* fetches
point to equivalent table_* converters, as they are in fact aliases
for src,table_* converters.

5 months agoDOC: config: clarify return type for some stick-table converters
Aurelien DARRAGON [Wed, 15 Jan 2025 19:37:27 +0000 (20:37 +0100)] 
DOC: config: clarify return type for some stick-table converters

Some stick-table converters such as "table_gpt" erroneously suggest that
the returned type is a boolean while in fact it is integer type, as
properly documented for the sample fetch equivalents.

5 months agoDOC: config: stick-table converter do accept ANY-typed input
Aurelien DARRAGON [Wed, 15 Jan 2025 19:30:55 +0000 (20:30 +0100)] 
DOC: config: stick-table converter do accept ANY-typed input

Since 2d17db58 ("MINOR: stick-table: change all stick-table converters'
inputs to SMP_T_ANY"), all stick-table converters accept ANY input
type as parameter, this means that it does no longer restrict the key as
a string representation of the input. However the doc wasn't updated when
the change was made. Moreover, some converters document the updated behavior
while others don't, which is kind of confusing, let's fix that.

5 months agoDOC: config: stick-table converters support implicit <table> argument
Aurelien DARRAGON [Wed, 15 Jan 2025 18:59:58 +0000 (19:59 +0100)] 
DOC: config: stick-table converters support implicit <table> argument

As with stick-table sample fetches, the <table> argument is not strictly
needed and defaults to the current proxy's stick-table when not provided

Let's update the doc and prototype to reflect the current behavior.

5 months agoDOC: config: unify sample conv|fetches optional arguments syntax
Aurelien DARRAGON [Wed, 15 Jan 2025 21:13:09 +0000 (22:13 +0100)] 
DOC: config: unify sample conv|fetches optional arguments syntax

The most common way (and proper way it seems) to declare optional
arguments in sample fetch or converters' prototype is to declare
them between square brackets, including the leading coma (because the
coma should be omitted if the argument is not provided). Also, when
multiple optional arguments are found, we should apply the same logic
but recursively.

In this patch we fix prototypes that include optional arguments and don't
follow this syntax. This improves readibility and sets the norm for
upcoming sample fetches/converters.

5 months agoMEDIUM: stktable: leverage smp_fetch_* helpers from sample conv
Aurelien DARRAGON [Tue, 14 Jan 2025 21:25:23 +0000 (22:25 +0100)] 
MEDIUM: stktable: leverage smp_fetch_* helpers from sample conv

In this patch we try to prevent code duplication: some fetches and sample
converters do the exact same thing, except that the converter takes the
argument as input data. Until now, both the converter and the fetch
had their own implementation (copy pasted), with the fetch specific or
converter specific lookup part.

Thanks to previous commits, we now have generic sample fetch helpers
that take the stkctr as argument, so let's leverage them directly
from the converter functions when available. This allows to remove
a lot of code duplication and should make code maintenance easier in the
future.

5 months agoMEDIUM: stktable: split sc_ and src_ fetch lookup logics
Aurelien DARRAGON [Tue, 14 Jan 2025 09:11:54 +0000 (10:11 +0100)] 
MEDIUM: stktable: split sc_ and src_ fetch lookup logics

While this patch actually adds more insertions than deletions, it actually
tries to simplify the lookup logic for sc_ and src_ sticktable fetches.

Indeed, smp_create_src_stkctr() and smp_fetch_sc_stkctr() combination
was used everywhere the fetch supports sc_ and src_ form, and
smp_fetch_sc_stkctr() even integrated some of the src-oriented fetch logic.

Not only this was confusing, but it made the task of adding new generic
fetches even more complex.

Thus in this patch we completely dedicate smp_fetch_sc_stkctr() to sc_
oriented fetches, while smp_create_src_stkctr() is now renamed to
smp_fetch_src_stkctr() and can now work on its own for src_ oriented
fetches. It takes an additional paramater, "create" to tell the function
if the entry should be created if it doesn't exist yet.

Now it's up to the calling function to know if it should be using the
sc_ oriented fetch or the src_ oriented one based on the input keyword.

5 months agoMEDIUM: stktable: split src-based key smp_fetch_sc functions
Aurelien DARRAGON [Mon, 13 Jan 2025 15:09:56 +0000 (16:09 +0100)] 
MEDIUM: stktable: split src-based key smp_fetch_sc functions

In this patch we split several sample fetch functions that are leveraged
by the "src-" fetches such as smp_fetch_sc_inc_gpc().

Indeed, for all of them, we add an intermediate helper function that takes
a stkctr pointer as parameter and performs the logic, leaving the lookup
part in the calling function. Before this patch existing functions were
doing the lookup + the fetch logic. Thanks to this patch it will become
easier to add generic converters taking lookup key as input.

List of targeted functions:
 - smp_fetch_sc_inc_gpc()
 - smp_fetch_sc_inc_gpc0()
 - smp_fetch_sc_inc_gpc1()
 - smp_fetch_sc_clr_gpc()
 - smp_fetch_sc_clr_gpc0()
 - smp_fetch_sc_clr_gpc1()
 - smp_fetch_sc_conn_cnt()
 - smp_fetch_sc_conn_rate()
 - smp_fetch_sc_updt_conn_cnt()
 - smp_fetch_sc_conn_curr()
 - smp_fetch_sc_glitch_cnt()
 - smp_fetch_sc_glitch_rate()
 - smp_fetch_sc_sess_cnt()
 - smp_fetch_sc_sess_rate()
 - smp_fetch_sc_http_req_cnt()
 - smp_fetch_sc_http_req_rate()
 - smp_fetch_sc_http_err_cnt()
 - smp_fetch_sc_http_err_rate()
 - smp_fetch_sc_http_fail_cnt()
 - smp_fetch_sc_http_fail_rate()
 - smp_fetch_sc_kbytes_in()
 - smp_fetch_sc_bytes_in_rate()
 - smp_fetch_kbytes_out()
 - smp_fetch_sc_gpc1_rate()
 - smp_fetch_sc_gpc0_rate()
 - smp_fetch_sc_gpc_rate()
 - smp_fetch_sc_get_gpc1()
 - smp_fetch_sc_get_gpc0()
 - smp_fetch_sc_get_gpc()
 - smp_fetch_sc_get_gpt0()
 - smp_fetch_sc_get_gpt()
 - smp_fetch_sc_bytes_out_rate()

Please note that this patch doesn't render any good using "git show" or
"git diff". For all the functions listed above, a new helper function was
defined right above it, with the same name without "_sc". These new
functions perform the fetch part, while the original ones (with "_sc")
now simply perform the lookup and then leverage the corresponding fetch
helper.

5 months agoMINOR: stktable: add smp_fetch_stksess() helper function
Aurelien DARRAGON [Thu, 9 Jan 2025 17:59:35 +0000 (18:59 +0100)] 
MINOR: stktable: add smp_fetch_stksess() helper function

smp_fetch_stksess(table, smp, create) performs a lookup in <table> by
using <smp> as a key. It returns matching entry on success and NULL on
failure. <create> can be set to 1 to force the entry creation.

We then use this helper everywhere relevant to prevent code duplication

5 months agoMINOR: stktable: fix potential build issue in smp_to_stkey (2nd try)
Aurelien DARRAGON [Wed, 15 Jan 2025 08:43:51 +0000 (09:43 +0100)] 
MINOR: stktable: fix potential build issue in smp_to_stkey (2nd try)

As discussed in GH #2838, the previous fix f399dbf
("MINOR: stktable: fix potential build issue in smp_to_stkey") which
attempted to remove conversion ambiguity and prevent build warning proved
to be insufficient.

This time, we implement Willy's suggestion, which is to use an union to
perform the conversion.

Hopefully this should fix GH #2838. If that's the case (and only in that
case), then this patch may be backported with f399dbf (else the patch
won't apply) anywhere b59d1fd ("BUG/MINOR: stktable: fix big-endian
compatiblity in smp_to_stkey()") was backported.

5 months agoBUG/MEDIUM: promex: Use right context pointers to dump backends extra-counters
Christopher Faulet [Tue, 14 Jan 2025 06:39:48 +0000 (07:39 +0100)] 
BUG/MEDIUM: promex: Use right context pointers to dump backends extra-counters

When backends extra counters are dumped, the wrong pointer was used in the
promex context to retrieve the stats module. p[1] must be used instead of
p[2]. Because of this typo, a infinite loop could be experienced if the
output buffer is full during this stage. But in all cases an overflow is
possible leading to a memory corruption.

This patch may be related to issue #2831. It must be backported as far as
3.0.

5 months agoBUG/MEDIUM: stktable: fix missing lock on some table converters
Aurelien DARRAGON [Tue, 14 Jan 2025 10:18:24 +0000 (11:18 +0100)] 
BUG/MEDIUM: stktable: fix missing lock on some table converters

In 819fc6f563
("MEDIUM: threads/stick-tables: handle multithreads on stick tables"),
sample fetch and action functions were properly guarded with stksess
read/write locks for read and write operations respectively, but the
sample_conv_table functions leveraged by "table_*" converters were
overlooked.

This bug was not known to cause issues in existing deployments yet (at
least it was not reported), but due to its nature it can theorically lead
to inconsistent values being reported by "table_*" converters if the value
is being updated by another thread in parallel.

It should be backported to all stable versions.

[ada: for versions < 3.0, glitch_cnt and glitch_rate samples should be
 ignored as they first appeared in 3.0]

5 months agoMINOR: stktable: fix potential build issue in smp_to_stkey
Aurelien DARRAGON [Fri, 10 Jan 2025 22:56:34 +0000 (23:56 +0100)] 
MINOR: stktable: fix potential build issue in smp_to_stkey

smp_to_stkey() uses an ambiguous cast from 64bit integer to 32 bit
unsigned integer. While it is intended, let's make the cast less
ambiguous by explicitly casting the right part of the assignment to the
proper type.

This should fix GH #2838

6 months agoBUG/MINOR: quic: reject NEW_TOKEN frames from clients
Amaury Denoyelle [Tue, 7 Jan 2025 17:22:00 +0000 (18:22 +0100)] 
BUG/MINOR: quic: reject NEW_TOKEN frames from clients

As specified by RFC 9000, reject NEW_TOKEN frames emitted by clients.
Close the connection with error code PROTOCOL_VIOLATION.

This must be backported up to 2.6.

6 months agoMINOR: trace: support all source alias on -dt
Amaury Denoyelle [Tue, 7 Jan 2025 17:29:23 +0000 (18:29 +0100)] 
MINOR: trace: support all source alias on -dt

Command line argument -dt can be used to activate traces during startup.
Via its optional argument, it is possible to change settings for a
particular trace source. It is also possible to update every registered
sources by specifying an empty name.

Support the trace source alias "all". This is an alternative to the
empty name to update every sources.

6 months agoMINOR: trace: ensure -dt priority over traces config section
Amaury Denoyelle [Tue, 7 Jan 2025 16:57:54 +0000 (17:57 +0100)] 
MINOR: trace: ensure -dt priority over traces config section

Traces can be activated on startup either via -dt command line argument
or via the traces configuration section. This can caused confusion as it
may not be clear as trace source can be completed or overriden by one or
the other.

Fix the precedence to give the priority to the command line argument.
Now, each trace source configured via -dt is first resetted to a default
state before applying new settings. Then, it is impossible to change a
trace source via the configuration file if it was already targetted via
-dt argument.

6 months agoMINOR: trace: add help message for -dt argument
Amaury Denoyelle [Tue, 7 Jan 2025 15:17:18 +0000 (16:17 +0100)] 
MINOR: trace: add help message for -dt argument

Traces can be activated on startup via -dt command line argument. To
facilitate its usage, display a usage description and examples when
"help" is specified.

6 months agoBUG/MEDIUM: queues: Adjust the proxy counters when appropriate
Olivier Houchard [Thu, 9 Jan 2025 16:43:41 +0000 (17:43 +0100)] 
BUG/MEDIUM: queues: Adjust the proxy counters when appropriate

In process_srv_queue(), if we manage to successfully run an extra task,
don't forget to adjust the proxy's totpend and served counters accordingly.
Having an inaccurate served could lead to various subtle bugs, as it is
used when making load balancing decisions.

This should not be backported, unless cda7275ef5d5e49fb2ea2373ea3b1ba63fc927c3
is backported too.

6 months agoMINOR: stktable: add sc[0-2]_key fetches
Aurelien DARRAGON [Mon, 30 Dec 2024 18:26:36 +0000 (19:26 +0100)] 
MINOR: stktable: add sc[0-2]_key fetches

As discussed in GH #1750, we were lacking a sample fetch to be able to
retrieve the key from the currently tracked counter entry. To do so,
sc_key fetch can now be used. It returns a sample with the correct type
(table key type) corresponding to the tracked counter entry (from previous
track-sc rules).

If no entry is currently tracked, it returns nothing.

It can be used using the standard form "sc_key(<sc_number>)" or the legacy
form: "sc0_key", "sc1_key", "sc2_key"

Documentation was updated.

6 months agoMINOR: stktable: add stksess_getkey() helper
Aurelien DARRAGON [Mon, 30 Dec 2024 19:15:50 +0000 (20:15 +0100)] 
MINOR: stktable: add stksess_getkey() helper

stksess_getkey(t, ts) returns a stktable_key struct pointer filled with
data from input <ts> entry in <t> table. Returned pointer uses the
static_table_key variable. Indeed, stktable_key struct is more convenient
to manipulate than having to deal with the key extraction from stktsess
struct directly.