]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
15 months agoBUG/MAJOR: ring: free the ring storage not the ring itself when using maps
Willy Tarreau [Tue, 26 Mar 2024 14:12:19 +0000 (15:12 +0100)] 
BUG/MAJOR: ring: free the ring storage not the ring itself when using maps

A recent issue was uncovered by the CI which started to randomly report
segfaults on a few tests, and more systematically on FreeBSD. It turn
out that it was introduced by recent commit 03816ccfa9 ("MAJOR: ring:
insert an intermediary ring_storage level"), which overlooked the munmap()
path of the sink and startup logs: once the ring and its storage were
split, it was no longer correct to munmap() the ring, only its storage
area needs to be unmapped, and the ring must always be freed separately.

Thanks to Christopher and William for their help at trying to reproduce
it and figure the circumstances that triggers it.

No backport is needed.

15 months agoBUG/MINOR: server: 'source' interface ignored from 'default-server' directive
Aurelien DARRAGON [Tue, 26 Mar 2024 09:42:48 +0000 (10:42 +0100)] 
BUG/MINOR: server: 'source' interface ignored from 'default-server' directive

Sebastien Gross reported that 'interface' keyword ('source' subargument)
is silently ignored when used from 'default-server' directive despite the
documentation implicitly stating that the keyword should be supported
there.

When support for 'source' keyword was added to 'default-server' directive
in dba97077 ("MINOR: server: Make 'default-server' support 'source'
keyword."), we properly duplicated the conn iface_name from the default-
server but we forgot to copy the conn iface_len which must be set as well
since it is used as setsockopt()'s 'optlen' argument in
tcp_connect_server().

It should be backported to all stable versions.

15 months agoBUILD: ssl: fix build error on older compilers with openssl-3.2
Willy Tarreau [Mon, 25 Mar 2024 20:21:47 +0000 (21:21 +0100)] 
BUILD: ssl: fix build error on older compilers with openssl-3.2

OpenSSL 3.2 triggers the code part added by commit 25da217 ("MINOR: ssl:
Update ssl_fc_curve/ssl_bc_curve to use SSL_get0_group_name") which
contains a variable declaration in the for() statement and breaks on
older compilers, as reported in GH issues #2501.

Let's just declare it normally to fix the problem. This must be
backported wherever the commit above is (at least 2.9).

15 months agoCLEANUP: ring: use only curr_cell and not next_cell in the main write loop
Willy Tarreau [Sun, 17 Mar 2024 15:54:36 +0000 (16:54 +0100)] 
CLEANUP: ring: use only curr_cell and not next_cell in the main write loop

It turns out that we can reduce by one variable in the loop and this
clobbers one less register, making it slightly faster on Cortex A72.

15 months agoOPTIM: ring: use relaxed stores to release the threads
Willy Tarreau [Fri, 22 Mar 2024 15:47:17 +0000 (16:47 +0100)] 
OPTIM: ring: use relaxed stores to release the threads

We don't care in what order the threads are released, so we can write
their sent value using relaxed atomic stores. This brings a 3-5% perf
boost on ARM with 80 cores, reaching 7.25M/s, and doesn't change
anything on x86 since it keeps using strict ordering.

15 months agoMINOR: ring: avoid writes to cells during copy
Willy Tarreau [Fri, 15 Mar 2024 15:10:55 +0000 (16:10 +0100)] 
MINOR: ring: avoid writes to cells during copy

It has been found that performing a first pass consisting in copying
all messages, and a second one to notify about releases is more efficient
on AMD than updating all of them on the fly using a CAS, despite making
writers wait longer to be released.

Maybe it's related to the ability for the CPU to prefetch the contents
during a simple load while it wouldn't do it for an XCHG, it's unsure
at this point. This will also mater permit to use relaxed stores to
release threads.

On ARM the performance increased to 7.0M/s. If this patch is applied
before the dropping of the intermediary step, instead it drops to
3.9M/s. This shows the dependency between such changes that strive to
limit the number of writes on the fast path.

On x86_64, the EPYC at 3C6T saw a small drop from 4.57M to 4.45M, but
the 24C48T setup saw a nice 33% boost from 3.33M to 4.44M, i.e. we
get stable perf at 3 and 24 cores, despite having 8 CCX involved and
fighting with each other.

Other possibilities are:
  - use of HA_ATOMIC_XCHG() instead of FETCH_OR()
    => slightly faster (4.62/7.37 vs 4.58/7.34). Pb: requires to
       modify the readers to wait much longer since the tail value
       won't be valid in this case during updates, and it will have
       to wait by looping over it.
  - use other conditions to release a cell
    => to be tested

15 months agoMINOR: ring: it's not x86 but all non-ARMv8.1 which needs the read before OR
Willy Tarreau [Sun, 17 Mar 2024 15:55:09 +0000 (16:55 +0100)] 
MINOR: ring: it's not x86 but all non-ARMv8.1 which needs the read before OR

Archs relying on CAS benefit from a read prior to FETCH_OR, so it's
not just x86 that benefits from this. Let's just change the condition
to only exclude __ARM_FEATURE_ATOMICS which is the only one faster
without.

15 months agoCLEANUP: ring: further simplify the write loop
Willy Tarreau [Sun, 17 Mar 2024 11:19:29 +0000 (12:19 +0100)] 
CLEANUP: ring: further simplify the write loop

The loop was cleaned up a little bit so that the inner loops are more
readable and that the ifdef'd parts are whole blocks and not just an
"if" condition. A few conditions were adjusted to benefit from "break"
and "continue".

15 months agoMINOR: ring: simplify the write loop a little bit
Willy Tarreau [Sun, 17 Mar 2024 11:09:30 +0000 (12:09 +0100)] 
MINOR: ring: simplify the write loop a little bit

This is mostly a cleanup in that it turns the two-level loop into a
single one, but it also simplifies the code a little bit and brings
some performance savings again, which are mostly noticeable on ARM,
but don't change anything for x86.

15 months agoMEDIUM: ring: improve speed in the queue waiting loop on x86_64
Willy Tarreau [Sun, 17 Mar 2024 09:20:56 +0000 (10:20 +0100)] 
MEDIUM: ring: improve speed in the queue waiting loop on x86_64

x86_64 doesn't have a native atomic FETCH_OR(), it's implemented using
a CAS, which will always cause a write cycle. Here we know we can just
wait as long as the lock bit is held so better loop on a load, and only
attempt the CAS on success. This requires a tiny ifdef and brings nice
benefits. This brings the performance back from 3.33M to 3.75M at 24C48T
while doing no change at 3C6T.

15 months agoMEDIUM: ring: significant boost in the loop by checking the ring queue ptr first
Willy Tarreau [Sun, 17 Mar 2024 09:20:56 +0000 (10:20 +0100)] 
MEDIUM: ring: significant boost in the loop by checking the ring queue ptr first

By doing that and placing the cpu_relax at the right places, the ARM
reaches 6.0M/s on 80 threads. On x86_64, at 3C6T the EPYC sees a small
increase from 4.45M to 4.57M but at 24C48T it sees a drop from 3.82M
to 3.33M due to the write contention hidden behind the CAS that
implements the FETCH_OR(), that we'll address next.

15 months agoMAJOR: ring: implement a waiting queue in front of the ring
Willy Tarreau [Mon, 11 Mar 2024 13:57:37 +0000 (14:57 +0100)] 
MAJOR: ring: implement a waiting queue in front of the ring

The queue-based approach consists in forcing threads to wait away from
the work area so as not to disturb the current writer, and to prepare
the work by grouping them in a queue. The last arrived takes the head
of the queue by placing its preinitialized ring cell there, becomes the
queue's leader, informs itself about the amount of previously accumulated
bytes so that when its turn comes, it immediately knows how much room is
needed to be released.

It can then take the whole queue with it, leaving an empty one for new
threads to come while it's releasing the room needed to copy everything.

By doing so we're cascading contention areas so that multiple parts can
work in parallel.

Note that we must never leave a write counter set to 0xFF at tail, and
this happens when a message cannot fit and we give up, because in this
case we're writing back tail_ofs, and only later we restore the counter.

The solution here is to make a special case when we're going to drop
the messages, and to write the readers count before restoring tail.

This already shows a tremendous performance gain on ARM (385k -> 4.8M),
thanks to the fact that now all waiting threads wait on the queue's
head instead of polluting the tail lock. On x86_64, the EPYC sees a big
boost at 24C48T (1.88M -> 3.82M) and a slowdown at 3C6T (6.0->4.45)
though this one is much less of a concern as so few threads need less
bandwidth than bigger counts.

15 months agoMINOR: ring: make the number of queues configurable
Willy Tarreau [Thu, 14 Mar 2024 07:57:02 +0000 (08:57 +0100)] 
MINOR: ring: make the number of queues configurable

Now the rings have one wait queue per group. This should limit the
contention on systems such as EPYC CPUs where the performance drops
dramatically when using more than one CCX.

Tests were run with different numbers and it was showed that value
6 outperforms all other ones at 12, 24, 48, 64 and 80 threads on an
EPYC, a Xeon and an Ampere CPU. Value 7 sometimes comes close and
anything around these values degrades quickly. The value has been
left tunable in the global section.

This commit only introduces everything needed to set up the queue count
so that it's easier to adjust it in the forthcoming patches, but it was
initially added after the series, making it harder to compare.

It was also shown that trying to group the threads in queues by their
thread groups is counter-productive and that it was more efficient to
do that by applying a modulo on the thread number. As surprising as it
seems, it does have the benefit of well balancing any number of threads.

15 months agoMINOR: ring: add the definition of a ring waiting cell
Willy Tarreau [Mon, 11 Mar 2024 13:27:42 +0000 (14:27 +0100)] 
MINOR: ring: add the definition of a ring waiting cell

This is what will be used to describe one waiting thread, its message
in the queues, and the aggregation of pending messages after it.

15 months agoMINOR: ring: keep a few frequently used pointers in the local stack
Willy Tarreau [Fri, 22 Mar 2024 13:46:12 +0000 (14:46 +0100)] 
MINOR: ring: keep a few frequently used pointers in the local stack

Code disassembly shows that ring->storage->tail and ring->queue are
accessed a lot and reloaded a lot due to aliasing. Let's just have
variables for them in the local stack. It makes the code smaller and
slightly faster.

15 months agoOPTIM: ring: have only one thread at a time wake up all readers
Willy Tarreau [Sat, 2 Mar 2024 10:09:37 +0000 (11:09 +0100)] 
OPTIM: ring: have only one thread at a time wake up all readers

It's inefficient and counter-productive that each ring writer iterates
over all readers to wake them up. Let's just have one in charge of this,
it strongly limits contention. The only thing is that since the thread
is iterating over a list, we want to be sure that if the first readers
have already completed their job, they will be woken up again. For this
we keep a counter of messages delivered after the wakeup started, and
the waking thread will check it before going back to sleep. In order to
avoid looping forever, it will also drop its waking flag soon enough to
possibly let another one take it.

There used to be a few cases of watchdogs before this on a 24-core AMD
EPYC platform on the list iteration those never appeared anymore.
The perf has dropped a bit on 3C6T on the EPYC, from 6.61 to 6.0M but
remains unchanged at 24C48T.

15 months agoOPTIM: ring: don't even try to update offset when failed to read
Willy Tarreau [Thu, 29 Feb 2024 10:57:28 +0000 (11:57 +0100)] 
OPTIM: ring: don't even try to update offset when failed to read

If there's nothing to read, it's pointless for a reader to try to update
the offset pointer, that's two atomic ops to replace a value by itself
twice. Let's just stop this.

15 months agoMAJOR: ring: drop the now unneeded lock
Willy Tarreau [Wed, 28 Feb 2024 16:06:39 +0000 (17:06 +0100)] 
MAJOR: ring: drop the now unneeded lock

It was only used to protect the list which is now an mt_list so it
doesn't provide any required protection anymore. It obviously also
used to provide strict ordering between the writer and the reader
when the writer started to update the messages, but that's now
covered by the oredered tail updates and updates to the readers
count to protect the area.

The message rate on small thread counts (up to 12) saw a boost of
roughly 5% while on large counts while for large counts it lost
about 2% due to some contention now becoming visible elsewhere.
Typical measures are 6.13M -> 6.61M at 3C6T, and 1.88 -> 1.92M at
24C48T on the EPYC.

15 months agoMINOR: ring: make sure ring_dispatch waits when facing a changing message
Willy Tarreau [Thu, 29 Feb 2024 10:55:22 +0000 (11:55 +0100)] 
MINOR: ring: make sure ring_dispatch waits when facing a changing message

The writer is using tags 0xFF instead of readers count at the front of
messages that are undergoing an update, while the tail has already been
updated. The reader needs to take care of this because it can face these
messages and mistakenly parse data that's still being written, leading
to corruption (especially if this happens while the size is changing).

Let's just stop reading when facing reserved codes, since they indicate
that the end of usable messages was reached.

15 months agoMEDIUM: ring: protect the initialization of the initial reader offset
Willy Tarreau [Wed, 28 Feb 2024 16:42:56 +0000 (17:42 +0100)] 
MEDIUM: ring: protect the initialization of the initial reader offset

Since we're going to remove the lock, there's no more way to prevent the
ring from being fed while we're attaching a client to it. We need to
freeze the buffer while looking at its head so that we can attach there
and have a trustable one. We could do it by setting the lock bit on the
tail offset but quite frankly we don't need to bother with that, attaching
a client is rare enough to permit a thread_isolate().

15 months agoMEDIUM: ring/applet: turn the wait_entry list to an mt_list instead
Willy Tarreau [Wed, 28 Feb 2024 16:04:40 +0000 (17:04 +0100)] 
MEDIUM: ring/applet: turn the wait_entry list to an mt_list instead

Rings are keeping a lock only for the list, which apparently doesn't
need anything more than an mt_list, so let's first turn it into that
before dropping the lock. There should be no visible effect.

15 months agoMINOR: ring: don't take the readers lock if there are no readers
Willy Tarreau [Wed, 28 Feb 2024 11:07:51 +0000 (12:07 +0100)] 
MINOR: ring: don't take the readers lock if there are no readers

There's no point looking for freshly attached readers if there are none,
taking this lock requires an atomic write to a shared area, something we
clearly want to avoid.

A general test with 213-byte messages on different thread counts shows
how the performance degrades across CCX and how this patch improves the
situation:
                   Before          After
    3C6T/1CCX:    6.39 Mmsg/s     6.35 Mmsg/s
   6C12T/2CCX:    2.90 Mmsg/s     3.16 Mmsg/s
  12C24T/4CCX:    2.14 Mmsg/s     2.33 Mmsg/s
  24C48T/8CCX:    1.75 Mmsg/s     1.92 Mmsg/s

This tends to confirm that the queues will really be needed and that
they'll have to be per-ccx hence per thread-group. They will amortize
the number of updates on head & tail (one per multiple messages).

15 months agoMEDIUM: ring: unlock the ring's tail earlier
Willy Tarreau [Wed, 28 Feb 2024 08:57:00 +0000 (09:57 +0100)] 
MEDIUM: ring: unlock the ring's tail earlier

We know we can continue to protect the message area so we can unlock the
tail as soon as we know its new value. Now we're seeing ~6.4M msg/s vs
5.4M previously on 3C6T of a 3rd gen EPYC, and 1.88M vs 1.54M for 24C48T
threads, which is a significant gain!

This requires to carefully write the new head counter before releasing
the writers, and to change the calculation of the work area from
tail..head to tail...new_tail while writing the message.

15 months agoMEDIUM: move the ring's lock to only protect the readers list
Willy Tarreau [Wed, 28 Feb 2024 08:52:55 +0000 (09:52 +0100)] 
MEDIUM: move the ring's lock to only protect the readers list

Now the lock is only taken around the readers list. With careful
ordering of writes to head/tail, the ring remains protected.

The perf is a bit better, though (1.54M msg/s vs 1.4M at 48T on
a 3rd gen EPYC, and 5.4M vs 5.3M for a 3C6T setup).

15 months agoMEDIUM: ring: use the topmost bit of the tail as a lock
Willy Tarreau [Wed, 28 Feb 2024 08:37:47 +0000 (09:37 +0100)] 
MEDIUM: ring: use the topmost bit of the tail as a lock

We're now locking the tail while looking for some room in the ring. In
fact it's still while writing to it, but the goal definitely is to get
rid of the lock ASAP. For this we reserve the topmost bit of the tail
as a lock, which may have as a possible visible effect that buffers will
be limited to 2GB instead of 4GB on 32-bit machines (though in practise,
good luck for allocating more than 2GB contiguous on 32-bit), but in
practice since the size is read with atol() and some operating systems
limit it to LONG_MAX unless passing negative numbers, the limit is
already there.

For now the impact on x86_64 is significant (drop from 2.35 to 1.4M/s
on 48 threads on EPYC 24 cores) but this situation is only temporary
so that changes can be reviewable and bisectable.

Other approaches were attempted, such as using XCHG instead, which is
slightly faster on x86 with low thread counts (but causes more write
contention), and forces readers to stall under heavy traffic because
they can't access a valid value for the queue anymore. A CAS requires
preloading the value and is les good on ARMv8.1. XADD could also be
considered with 12-13 upper bits of the offset dedicated to locking,
but that looks overkill.

15 months agoMEDIUM: ring: protect the reader's positions against writers
Willy Tarreau [Wed, 28 Feb 2024 16:18:34 +0000 (17:18 +0100)] 
MEDIUM: ring: protect the reader's positions against writers

The reader now needs to protect the positions it's reading. This is
already done via the readers counter at the beginning of messages,
but as long as the lock is present, this counter is decremented
before starting to parse messages, and incremented at the end.

We must now do that in reverse, first protect the end of the messages,
and only then remove ourselves from the already processed messages, so
that at no point could a writer pass over and possibly overwrite data
we're currently watching.

15 months agoMEDIUM: ring: lock the tail's readers counters before proceeding with the changes
Willy Tarreau [Wed, 28 Feb 2024 08:20:54 +0000 (09:20 +0100)] 
MEDIUM: ring: lock the tail's readers counters before proceeding with the changes

The goal here is to start to protect the writing area inside the area
itself so that we'll later be able to release the ring's lock. We're not
there yet, but at least the tail is marked as protected for as long as the
message is not fully written.

15 months agoMINOR: ring: make the reader check the readers count before inc/dec
Willy Tarreau [Wed, 28 Feb 2024 08:03:46 +0000 (09:03 +0100)] 
MINOR: ring: make the reader check the readers count before inc/dec

We'll want to reserve some special values for the readers count to
temporary lock the following message, but for this it will be mandatory
that readers check for them before incrementing/decrementing the counter.
Let'sdo that using a CAS. The readers performance is not as critical as
the writer's anyway so the slight overhead is not a problem.

15 months agoMEDIUM: ring: align the head and tail fields in the ring_storage structure
Willy Tarreau [Wed, 28 Feb 2024 11:04:22 +0000 (12:04 +0100)] 
MEDIUM: ring: align the head and tail fields in the ring_storage structure

We really want to let the readers and writers act on different areas, so
we want to have the tail and the head on separate cache lines, themselves
separate from the rest of the ring. Doing so improves the performance from
2.15 to 2.35M msg/s at 48 threads on a 24-core EPYC.

This increases the header space from 32 to 192 bytes when threads are
enabled. But since we already have the header size available in the file,
haring remains able to detect the aligned vs unaligned formats and call
dump_v2a() when aligned is detected.

15 months agoMEDIUM: ring: remove the struct buffer from the ring
Willy Tarreau [Tue, 27 Feb 2024 08:17:45 +0000 (09:17 +0100)] 
MEDIUM: ring: remove the struct buffer from the ring

The purpose is to store a head and a tail that are independent so that
we can further improve the API to update them independently from each
other.

The struct was arranged like the original one so that as long as a ring
has its head set to zero (i.e. no recycling) it will continue to work.
The new format is already detectable thanks to the "rsvd" field which
indicates the number of reserved bytes at the beginning. It's located
where the buffer's area pointer previously was, so that older versions
of haring can continue to open the ring in repair mode, and newer ones
can use the fact that the upper bits of that variable are zero to guess
that it's working with the new format instead of the old one. Also let's
keep in mind that the layout will further change to place some alignment
constraints.

The haring tool will thus updated based on this and it detects that the
rsvd field is smaller than a page and that the sum of it with the size
equals the mapped size, in which case it uses the new dump_v2() function
instead of dump_v1(). The new function also creates a buffer from the
ring's area, size, head and tail and calls the generic one so that no
other code had to be adapted.

15 months agoMEDIUM: ring: change the ring reader to use the new vector-based API now
Willy Tarreau [Tue, 27 Feb 2024 06:58:26 +0000 (07:58 +0100)] 
MEDIUM: ring: change the ring reader to use the new vector-based API now

The code now looks cleaner and more easily shows what still needs to be
addressed. There are not that many changes in practice, these are mostly
mechanical, essentially hiding the buffer from the callers.

15 months agoMEDIUM: ring: replace the buffer API in ring_write() with the vec<->ring API
Willy Tarreau [Mon, 26 Feb 2024 10:03:03 +0000 (11:03 +0100)] 
MEDIUM: ring: replace the buffer API in ring_write() with the vec<->ring API

This is the start of the replacement of the buffer API calls. Only the
ring_write() function was touched. Instead of manipulating a buffer all
along, we now extract the ring buffer's head and tail upon entry, store
them locally and use them using the vec<->ring API until the last moment
where we can update the buffer with the new values. One subtle point is
that we must never fill the buffer past the last byte otherwise the
vec-to-ring conversion gets lost and there's no more possibility to know
where's the beginning nor the end (just like when dealing with head+tail
in fact), because it then becomes impossible to distinguish between an
empty and a full buffer.

15 months agoMINOR: ring: allow to reduce a ring size
Willy Tarreau [Thu, 14 Mar 2024 05:48:41 +0000 (06:48 +0100)] 
MINOR: ring: allow to reduce a ring size

In ring_resize() we used to check if the new ring was at least as large
as the previous one before resizing it, but what counts is that it's as
large as the previous one's contents. Initially it was thought this
would not really matter, but given that rings are initially created as
BUFSIZE, it's currently not possible to shrink them for debugging
purposes. Now with this change it is.

15 months agoMINOR: ring: resize only under thread isolation
Willy Tarreau [Wed, 28 Feb 2024 08:45:54 +0000 (09:45 +0100)] 
MINOR: ring: resize only under thread isolation

The ring resizing was already quite tricky, but when facing atomic
writes it will no longer be possible and we definitely do not want to
have to deal with a lock there. Since it's only done at boot time, and
possibly later from the CLI, let's just do it under thread isolation.

15 months agoMAJOR: ring: insert an intermediary ring_storage level
Willy Tarreau [Sun, 3 Mar 2024 16:20:10 +0000 (17:20 +0100)] 
MAJOR: ring: insert an intermediary ring_storage level

We'll need to add more complex structures in the ring, such as wait
queues. That's far too much to be stored into the area in case of
file-backed contents, so let's split the ring definition and its
storage once for all.

This patch introduces a struct ring_storage which is assigned to
ring->storage, which contains minimal information to represent the
storage layout, i.e. for now only the buffer, and all the rest
remains in the ring itself. The storage is appended immediately after
it and the buffer's pointer always points to that area. It has the
benefit of remaining 100% compatible with the existing file-backed
layout. In memory, the allocation loses the size of a struct buffer.

It's not even certain it's worth placing the size there, given that it's
constant and that a dump of a ring wouldn't really need it (the file size
is sufficient). But for now everything comes with the struct buffer, and
later this will change once split into head and tail. Also this area may
be completed with more information in the future (e.g. storage version,
format, endianness, word size etc).

15 months agoMINOR: ring: add a flag to indicate a mapped file
Willy Tarreau [Sun, 3 Mar 2024 16:50:11 +0000 (17:50 +0100)] 
MINOR: ring: add a flag to indicate a mapped file

Till now we used to rely on a heuristic pointer comparison to check if
a ring was mapped or allocated. Better assign a flag to clarify this
because it's going to become difficult otherwise.

15 months agoMINOR: ring: use ring_size(), ring_area(), ring_head() and ring_tail()
Willy Tarreau [Wed, 6 Mar 2024 15:50:40 +0000 (16:50 +0100)] 
MINOR: ring: use ring_size(), ring_area(), ring_head() and ring_tail()

Some open-coded constructs were updated to make use of the ring accessors
instead. This allows to remove some direct dependencies on the buffers
API a bit more.

15 months agoMINOR: errors: use ring_dup() to duplicate the startup_logs
Willy Tarreau [Tue, 27 Feb 2024 18:52:50 +0000 (19:52 +0100)] 
MINOR: errors: use ring_dup() to duplicate the startup_logs

In startup_logs_dup() we currently need to reference the ring's buffer,
better not do this as it will complicate operations when switching to
other types.

15 months agoMINOR: ring: make callers use ring_data() and ring_size(), not ring->buf
Willy Tarreau [Tue, 27 Feb 2024 17:54:19 +0000 (18:54 +0100)] 
MINOR: ring: make callers use ring_data() and ring_size(), not ring->buf

As we're going to remove the ring's buffer, we don't want callers to access
it directly, so let's use ring_data() and ring_size() instead for this.

15 months agoMINOR: ring: also add ring_area(), ring_head(), ring_tail()
Willy Tarreau [Wed, 6 Mar 2024 15:40:39 +0000 (16:40 +0100)] 
MINOR: ring: also add ring_area(), ring_head(), ring_tail()

These will essentially be used to simplify the conversion to a new API.

15 months agoMINOR: ring: add ring_dup() to copy a ring into another one
Willy Tarreau [Tue, 27 Feb 2024 18:52:00 +0000 (19:52 +0100)] 
MINOR: ring: add ring_dup() to copy a ring into another one

This will mostly be used during reallocation and boot-time duplicates,
the purpose is simply to save the caller from having to know the details
of the internal representation.

15 months agoMINOR: ring: add ring_size() to return the ring's size
Willy Tarreau [Tue, 27 Feb 2024 17:53:40 +0000 (18:53 +0100)] 
MINOR: ring: add ring_size() to return the ring's size

This is just to ease conversion so that callers stop accessing the ring's
buffer.

15 months agoMINOR: ring: add ring_data() to report the amount of data in a ring
Willy Tarreau [Tue, 27 Feb 2024 08:12:19 +0000 (09:12 +0100)] 
MINOR: ring: add ring_data() to report the amount of data in a ring

This will be used as an accessor for the few functions that need this
outside of ring.c.

15 months agoMINOR: ring: rename totlen vs msglen in ring_write()
Willy Tarreau [Mon, 26 Feb 2024 19:03:20 +0000 (20:03 +0100)] 
MINOR: ring: rename totlen vs msglen in ring_write()

The ring_write() function uses confusing variable names: totlen is in
fact the length of the message, not the total length that is going to
be written. Let's rename it msglen and have a real "needed" that
corresponds to the total size we're going to write. We also add a
BUG_ON_HOT() to catch mistakes causing discrepancies.

15 months agoMINOR: vecpair: add necessary functions to use vecpairss from/to ring APIs
Willy Tarreau [Mon, 26 Feb 2024 18:53:34 +0000 (19:53 +0100)] 
MINOR: vecpair: add necessary functions to use vecpairss from/to ring APIs

Many ring-based APIs need a tail and a head, with some extra assumption
that the user takes care of not filling the ring so that tail==head is
unambiguous. Vectors are particularly suited to this usage so here we
create 4 functions to create vectors representing free room or data
from a ring, as well as updating rings based on a pair of vectors that
represents either free space or data.

15 months agoMINOR: vecpair: add new vector pair based data manipulation mechanisms
Willy Tarreau [Thu, 22 Feb 2024 07:02:35 +0000 (08:02 +0100)] 
MINOR: vecpair: add new vector pair based data manipulation mechanisms

The buffers API defines both a storage layout and how to handle the
data. The storage is shared with the chunks API which only deals with
non-wrapping messages while buffers support wrapping both of the data
and of the free space. As such, most of the buffers code already makes
special cases of two parts in a buffer, the first one before wrapping
and the optional second one after the wrapping occurred.

The thing is, there are plenty of other places (e.g. rings) where the
code dealing with wrapping is desirable but with a different storage
layout. Let's export the existing buffer handling code related to
reading/writing wrapping data and make it work with arbitrary vector
pairs instead. This will handle wrapping and holes in messages if
desired, and it will be up to the caller to decide how its messages
are arranged and to pass the relevant ptr,len elements.

The code is limited to two vectors because this is sufficient to deal
with wrapping without making the code needlessly complex. I.e. this will
not reassemble an iovec. For vectors, since we already had the ist type,
there's no point inventing a new type, and it's even possible that over
time some callers will find benefits in using this unified API (i.e. no
NOP translation layer). It also allows to pass inputs as direct arguments
and outputs as pointers. Not only this is more efficient code-wise, but
it also avoids the accidental use of a wrong function. It was indeed
found that naming functions is even harder than with the buffer as the
notion of from/to is even fuzzier here.

The API will likely continue to evolve and some functions might get
renamed to more explicit ones over time to limit confusion. For now
the code provides anything needed to reset/create/fill/erase/read/peek
or measure vector pairs and to manipulate chars/blocks/varints to/from
there.

15 months agoMINOR: ring: reserve one special value for the readers count
Willy Tarreau [Mon, 20 Feb 2023 18:21:52 +0000 (19:21 +0100)] 
MINOR: ring: reserve one special value for the readers count

In order to support concurrent writers we'll need to lock areas in the
buffer. For this we'll use one special value of the single-byte readers
count. Let's reserve it now and use the macro instead of the hardcoded
255.

15 months agoMINOR: ring: make the ring reader use only absolute offsets
Willy Tarreau [Thu, 23 Feb 2023 08:53:38 +0000 (09:53 +0100)] 
MINOR: ring: make the ring reader use only absolute offsets

The goal is to remove references to the buffer's head and tail in the
fast path so that we can release the lock during some reads. This means
no more comparisons with b_data() nor operations relative to b_head()
will be possible anymore. As a first step we need to have an absolute
offset in the buffer, and to use b_getblk_ofs() in the applet callbacks
to retrieve the data based on this.

15 months agoMINOR: buf: add b_getblk_ofs() that works relative to area and not head
Willy Tarreau [Wed, 22 Feb 2023 14:53:52 +0000 (15:53 +0100)] 
MINOR: buf: add b_getblk_ofs() that works relative to area and not head

For some concurrently accessed buffers we can't rely on head/data etc,
but sometimes the access patterns guarantees that the buffer contents
are there. Let's implement a function to read contents from a fixed
offset, which never checks head nor data, only the area and its size.
It's the caller's job to get this offset.

15 months agoMINOR: buf: add b_putblk_ofs() to copy a block at a specific position
Willy Tarreau [Thu, 23 Feb 2023 11:02:05 +0000 (12:02 +0100)] 
MINOR: buf: add b_putblk_ofs() to copy a block at a specific position

This new function b_putblk_ofs() puts one full block of data of length
<len> from <blk> into the buffer, starting from absolute offset <offset>
after the buffer's area.  As a convenience to avoid complex checks in
callers, the offset is allowed to exceed a valid one by no more than one
buffer size, and will automatically be wrapped. The caller is responsible
for ensuring that <len> doesn't exceed the known length of the available
room at this position, otherwise data may be overwritten. The buffer's
length is *not* updated, so generally the caller will have updated it
before calling this function. This is meant to be used on concurrently
accessed buffers, so that a writer can append data while a reader is
blocked by other means from reaching the current area The function
guarantees never to use ->head nor ->data.

15 months agoMINOR: buf: add b_rel_ofs() to turn an absolute offset into a relative one
Willy Tarreau [Thu, 23 Feb 2023 08:52:58 +0000 (09:52 +0100)] 
MINOR: buf: add b_rel_ofs() to turn an absolute offset into a relative one

It basically does the opposite of b_peek_ofs(). If x=b_peek_ofs(y), then
y=b_rel_ofs(x).

15 months agoMINOR: buf: add b_add_ofs() to add a count to an absolute position
Willy Tarreau [Thu, 23 Feb 2023 08:52:42 +0000 (09:52 +0100)] 
MINOR: buf: add b_add_ofs() to add a count to an absolute position

This function is used to compute a new absolute buffer offset by adding a
length to an existing valid offset. It will check for wrapping.

15 months agoMEDIUM: log/sink: make the log forwarder code use ring_dispatch_messages()
Willy Tarreau [Tue, 27 Feb 2024 16:05:11 +0000 (17:05 +0100)] 
MEDIUM: log/sink: make the log forwarder code use ring_dispatch_messages()

This code becomes even simpler and almost does not need any knowledge
of the structure of the ring anymore. It even highlighted that an old
race had not been fixed due to code duplication, but that's now done.

15 months agoMEDIUM: sink: move the generic ring forwarder code use ring_dispatch_messages()
Willy Tarreau [Tue, 27 Feb 2024 16:01:45 +0000 (17:01 +0100)] 
MEDIUM: sink: move the generic ring forwarder code use ring_dispatch_messages()

Now the code is much simpler than the ring forwarding function almost does
not need any knowledge of the structure of the ring anymore.

15 months agoMEDIUM: ring: move the ring reader code to ring_dispatch_messages()
Willy Tarreau [Tue, 27 Feb 2024 15:54:18 +0000 (16:54 +0100)] 
MEDIUM: ring: move the ring reader code to ring_dispatch_messages()

This new function is made around the loop that scans a ring for new
messages and dispatches them to a message handler. It also takes
ring flags (WAIT, NEW, etc) and offset pointers that the caller will
use to initialize/reuse/update the current processing offset. The
caller is still responsible for presetting it to ~0 before the
first call if it wants the function to automatically adjust it (or set
it to the correct value). The function may also return the last_ofs
that was known before releasing the lock so that the caller knows
what to compare against and if it needs to restart processing or not.
The context remains a void* so that should not necessarily depend on
an appctx.

The current "show ring" code was ported to this and it continues to
work as expected.

15 months agoREORG: dns/ring: split the ring between the generic one and the DNS one
Willy Tarreau [Mon, 19 Feb 2024 13:38:59 +0000 (14:38 +0100)] 
REORG: dns/ring: split the ring between the generic one and the DNS one

A ring is used for the DNS code but slightly differently from the generic
one, which prevents some important changes from being made to the generic
code without breaking DNS. As the use cases differ, it's better to just
split them apart for now and have the DNS code use its own ring that we
rename dns_ring and let the generic code continue to live on its own.

The unused parts such as CLI registration were dropped, resizing and
allocation from a mapped area were dropped. dns_ring_detach_appctx() was
kept despite not being used, so as to stay consistent with the comments
that say it must be called, despite the DNS code explicitly mentioning
that it skips it for now (i.e. this may change in the future).

Hopefully after the generic rings are converted the DNS code can migrate
back to them, though this is really not necessary.

15 months agoMEDIUM: ring/sink: use applet_append_line()/syslog_applet_append_event() for readers
Willy Tarreau [Tue, 27 Feb 2024 14:55:26 +0000 (15:55 +0100)] 
MEDIUM: ring/sink: use applet_append_line()/syslog_applet_append_event() for readers

The rink reader code was duplicated as-is in 2.2 for the ring forwarding
code in commits 494c505703 ("MEDIUM: ring: add server statement to forward
messages from a ring") and 975564784f ("MEDIUM: ring: add new srv statement
to support octet counting forward") (which only differs by using a prefix
instead of a suffix to delimit messages).

Unfortunately, that makes it almost impossible to rework the core ring
code because all these parts rely on it. This first commit aims at
restoring a common structure for the core loop by just calling a distinct
function based on the use case. The functions are either
applet_append_line() when a whole line is to be emitted followed by an LF
character, or syslog_applet_appent_event() when trying to send a TCP
syslog line prepended with its size in decimal.

There is no functional change beyond this.

15 months agoMINOR: log/applet: add new function syslog_applet_append_event()
Willy Tarreau [Tue, 27 Feb 2024 15:17:42 +0000 (16:17 +0100)] 
MINOR: log/applet: add new function syslog_applet_append_event()

This function takes a buffer on input, and offset and a length, and
consumes the block from that buffer to send it to the appctx's output
buffer. Contrary to its sibling applet_append_line(), instead of just
appending an LF at the end of the line, it prepends the message size
in decimal and a space before the message, as expected by syslog TCP
implementaions. This will be used to simplify the ring reader code.

15 months agoMINOR: applet: add new function applet_append_line()
Willy Tarreau [Tue, 27 Feb 2024 14:50:55 +0000 (15:50 +0100)] 
MINOR: applet: add new function applet_append_line()

This function takes a buffer on input, and offset and a length, and
consumes the block from that buffer to send it to the appctx's output
buffer. This will be used to simplify the ring reader code.

15 months agoMINOR: atomic: add a read-specific variant of __ha_cpu_relax()
Willy Tarreau [Fri, 15 Mar 2024 16:31:35 +0000 (17:31 +0100)] 
MINOR: atomic: add a read-specific variant of __ha_cpu_relax()

Tests on various systems show that x86 prefers not to wait at all inside
read loops while aarch64 prefers to wait a little bit. Instead of having
to stuff ifdefs around __ha_cpu_relax() inside plenty of such loops
waiting for a condition to appear, better implement a new variant that
we call __ha_cpu_relax_for_read() which honors each architecture's
preferences and is the same as __ha_cpu_relax() for other ones.

15 months agoMINOR: debug: add "debug dev trace" to flood with traces
Willy Tarreau [Fri, 15 Mar 2024 06:16:08 +0000 (07:16 +0100)] 
MINOR: debug: add "debug dev trace" to flood with traces

This new command, enabled only with "DEBUG_DEV", sends 2 or 20 traces
per task wakeup (depending on the verbosity level), and stops after 1M
wakeups per thread in order not to have to stop/start the process each
time it's fired.

We have two small messages and 18 larger ones from 20 to 270 bytes
each, so that the average size is approx 213 bytes counting headers
(the header adds approx 82 bytes), which matches what's generally
observed on average when traces are enabled in all muxes.

Typical figures show varations between 5.7M and 6.2M msg/s on an EPYC
in a 3C6T setup (single CCX), and 2.12M - 2.22M in a 24C48T setup
(across 8 CCX, with 8 thread groups).

15 months agoOPTIM: http_ext: avoid useless copy in http_7239_extract_{ipv4,ipv6}
Aurelien DARRAGON [Mon, 18 Mar 2024 14:18:08 +0000 (15:18 +0100)] 
OPTIM: http_ext: avoid useless copy in http_7239_extract_{ipv4,ipv6}

In http_7239_extract_{ipv4,ipv6}, we declare a local buffer in order to
use inet_pton() since it requires a valid destination argument (cannot be
NULL). Then, if the caller provided <ip> argument, we copy inet_pton()
result (from local buffer to <ip>).

In fact when the caller provides <ip>, we may directly use <ip> as
inet_pton() dst argument to avoid an useless copy. Thus the local buffer
is only relevant when the user doesn't provide <ip>.

While at it, let's add a missing testcase for the rfc7239_n2nn converter
(to check that http_7239_extract_ipv4() with <ip> provided works properly)

This could be backported in 2.8 with b2bb925 ("MINOR: proxy/http_ext:
introduce proxy forwarded option")

15 months agoBUILD: server: fix build regression on old compilers (<= gcc-4.4)
Aurelien DARRAGON [Mon, 25 Mar 2024 12:38:36 +0000 (13:38 +0100)] 
BUILD: server: fix build regression on old compilers (<= gcc-4.4)

Willy reported that since 3ac79b504 ("MEDIUM: server:
make server_set_inetaddr() updater serializable"), haproxy fails to
compile on some older compilers such as gcc-4.4 with this kind of error:

  src/server.c: In function 'snr_resolution_cb':
  src/server.c:4471: error: unknown field 'dns_resolver' specified in initializer
  compilation terminated due to -Wfatal-errors.
  make: *** [Makefile:1006: src/server.o] Error 1

This is due to referencing a member inside anonymous union from a compound
literal assignment. Apparently such use of anonymous union wasn't properly
supported back then on older compilers. To fix the issue, we give "u" name
to the parent union use this name to explicitly refer to the union where
relevant in the code (only a few changes fortunately).

The fix itself was verified to restore build compatibility with gcc 4.4
(and even 4.2).

As 3ac79b504 is used as a prerequisite for 64c9c8ef3 ("BUG/MINOR:
server/dns: use server_set_inetaddr() to unset srv addr from DNS"), please
consider backporting this patch too if 64c9c8ef3 happens to be backported
in 2.9.

15 months agoBUG/MEDIUM: mux-fcgi: Properly handle EOM flag on end-of-trailers HTX block
Christopher Faulet [Mon, 25 Mar 2024 07:02:06 +0000 (08:02 +0100)] 
BUG/MEDIUM: mux-fcgi: Properly handle EOM flag on end-of-trailers HTX block

Trailers are skipped by the FCGI multiplexer. However empty chunked messages
are not properly handled. It may be a chunked H1 request with no payload or
a H2/H3 POST request with no payload. In that caes, the EOT HTX block is
just ignored. The issue is that the EOM flag is thus ignored too. It means
no empty STDIN record is sent to mark the end of the request to the server.

To fix the issue, when a EOT htx block is found and it is the last HTX block
of the message (and it should be), the EOM flag is tested. If it is found,
an empty STDIN record is emitted.

This patch should fix the issue #2499. It must be backported as far as 2.4.

15 months agoBUG/MINOR: mux-quic: close all QCS before freeing QCC tasklet
Amaury Denoyelle [Fri, 22 Mar 2024 14:13:42 +0000 (15:13 +0100)] 
BUG/MINOR: mux-quic: close all QCS before freeing QCC tasklet

QUIC MUX is freed via qcc_release(). This in turn liberate all the
remaining QCS instances. For each one of them, their corresponding
stream-desc is released via qc_stream_desc_release().

This last function may itself notifies QUIC MUX when new buffers are
available. This is useful when QCS are closed individually without the
whole connection. However, when the connection is closed through
qcc_release(), this may cause issue as some elements of QUIC MUX are
already freed.

In 2.9.6, a bug was detected directly linked to this. Indeed, QCC
instance may be woken up on stream-desc release. If called through
qcc_release(), this is an issue because QCC tasklet is freed before QCS
instances. However, this bug is not systematic and relies on prior
conditions : in particular, QUIC MUX must be under Tx buffers exhaustion
prior to the qcc_release() invocation.

The current dev tree is not impacted by this bug, thanks to QUIC MUX
refactoring. Indeed, notifying accross layers have changed and now
stream-desc release notifies individual QCS instances instead of the QCC
element, which is a safer mechanism. However, to simplify backport
process, bugfix is introduced in the current dev tree as it does not
have any impact.

Note that a proper fix would be to set quic-conn MUX state to
QC_MUX_RELEASED. However, it is not possible to call quic_close()
without having releasing all stream-desc elements first. The simpler
solution was chosen to prevent other breaking issues during backports.

This should fix github issue #2494.

It should be backported up to 2.6. Note that prior to 2.7 qcc_release()
was named qc_release().

15 months agoMEDIUM: server: close private idle connection before server deletion
Amaury Denoyelle [Thu, 14 Mar 2024 10:24:19 +0000 (11:24 +0100)] 
MEDIUM: server: close private idle connection before server deletion

This commit similar to the following one :
  65ae241dcfe710e1cdd3ec4e7a9bde38d2e4c116
  MEDIUM: server: close idle conn before server deletion

This patch implements a similar logic, this time to close private idle
connections stored in sessions. The principle is identical to the above
commit : conn_release() is used on idle connections after a takeover to
ensure thread safety.

An extra change was required to be able to execute takeover on such
connections. Their original thread ID was unknown, contrary to non
private connections which are stored in sharded lists. As such, a new
tid member has been added under sess_priv_conns chaining element.

15 months agoMEDIUM: mux: prepare for takeover on private connections
Amaury Denoyelle [Wed, 20 Mar 2024 14:51:09 +0000 (15:51 +0100)] 
MEDIUM: mux: prepare for takeover on private connections

When a backend connection is marked as idle, a special flag TASK_F_USR1
is set on MUX tasklet. When MUX tasklet is reactivated, extra checks are
executed under this flag to ensure no takeover occurred in the meantime.

Previously, only non private connections could be targetted by a
takeover. However, this will change when implementing private idle
connections closure on "delete server" CLI handler. As such, TASK_F_USR1
is now also set for private connections in MUX detach callbacks.

15 months agoMEDIUM: server: close idle conn on server deletion
Amaury Denoyelle [Wed, 13 Mar 2024 10:33:50 +0000 (11:33 +0100)] 
MEDIUM: server: close idle conn on server deletion

To be able to delete a server, a number of preconditions must be
validated to ensure it is not in used anymore. Previously, if idle
connections were stored in the server, the deletion was cancelled. No
action was implemented to force idle connection closure, the only
solution was to wait for the periodic purging to be achieved.

This is an extra burden to be able to delete a server. Indeed, idle
connections are by definition inactive and can be closed prior to delete
a server. This is the exact purpose of this patch.

Idle connections removal is implemented inside "delete server" handler,
once it has been determined that the server can be freely removed. A
simple loop is run to call conn_release() over each idle connections.
Takeover is also executed before conn_release() to ensure tasks/tasklets
or any other sensible elements are not deleted from a foreign thread.

This patch should reduce the occurence of rejected "delete server"
execution, especially when connection reuse is high.

15 months agoMINOR: connection: extend takeover with release option
Amaury Denoyelle [Fri, 15 Mar 2024 14:36:33 +0000 (15:36 +0100)] 
MINOR: connection: extend takeover with release option

Extend takeover API both for MUX and XPRT with a new boolean argument
<release>. Its purpose is to signal if the connection will be freed
immediately after the takeover, rendering new resources allocation
unnecessary.

For the moment, release argument is always false. However, it will be
set to true on delete server CLI handler to proactively close server
idle connections.

15 months agoMINOR: connection: implement conn_release()
Amaury Denoyelle [Wed, 20 Mar 2024 14:37:09 +0000 (15:37 +0100)] 
MINOR: connection: implement conn_release()

Several places reuse the same code to ensure a connection is properly
freed, either via its MUX or by calling the proper set of functions.
Factorize all of this in a new function conn_release().

This new function is now called via session_free() and
session_accept_fd(). It will also be reused on delete server to
proactively close idle connections.

15 months agoREGTESTS: ssl: Add checks on ocsp-update log format
Remi Tricot-Le Breton [Wed, 20 Mar 2024 13:13:39 +0000 (14:13 +0100)] 
REGTESTS: ssl: Add checks on ocsp-update log format

Add checks on the ocsp-update's dedicated log format.

15 months agoCLEANUP: ssl: Remove undocumented ocsp fetches
Remi Tricot-Le Breton [Wed, 20 Mar 2024 13:13:38 +0000 (14:13 +0100)] 
CLEANUP: ssl: Remove undocumented ocsp fetches

Those fetchess were undocumented and were just here so that the
ocsp-update log could be made through a regular log format. But since
the logging is now "handmade" (since BUG/MEDIUM: ssl: Fix crash in
ocsp-update log function), we don't need those anymore.

15 months agoMINOR: ssl: Change level of ocsp-update logs
Remi Tricot-Le Breton [Wed, 20 Mar 2024 13:13:37 +0000 (14:13 +0100)] 
MINOR: ssl: Change level of ocsp-update logs

The pure ocsp-update log used to be in log level "info" and it would be
mixed with actual traffic logs. This patch changes it to level "notice".

15 months agoMEDIUM: ssl: Change output of ocsp-update log
Remi Tricot-Le Breton [Wed, 20 Mar 2024 13:13:36 +0000 (14:13 +0100)] 
MEDIUM: ssl: Change output of ocsp-update log

Since commit "BUG/MEDIUM: ssl: Fix crash in ocsp-update log function",
some information from the log line are "faked" because they can be
actually retrieved anymore (or never could). We should then remove them
from the logline all along instead of providing some useless fields.

We then only keep pure OCSP-update information in the log line:
"<certname> <status> <status str> <fail count> <success count>"

15 months agoBUG/MEDIUM: ssl: Fix crash in ocsp-update log function
Remi Tricot-Le Breton [Wed, 20 Mar 2024 13:13:35 +0000 (14:13 +0100)] 
BUG/MEDIUM: ssl: Fix crash in ocsp-update log function

The ocsp-update logging mechanism was built around the 'sess_log'
function which required to keep a pointer to the said session until the
logging function could be called. This was made by keeping a pointer to
the appctx returned by the 'httpclient_start' function. But this appctx
lives its life on its own and might be destroyed before
'ssl_ocsp_send_log' is called, which could result in a crash (UAF).
Fixing this crash requires to stop using the 'sess_log' function to emit
the ocsp-update logs. The log line will then need to be built by hand
out of the information actually available when 'ssl_ocsp_send_log' is
called. Since we don't use the "regular" logging functions anymore, we
don't need to use the error_logformat anymore. In order to keep a
consistent behavior than before, we will keep the same format for the
logs but replace the fields that required a 'sess' pointer by fake
values (the %ci:%cp for instance, which was never filled anyway).

This crash was raised in GitHub issue #2442.
It should be backported up to branch 2.8.

15 months agoBUG/MEDIUM: ssl: Fix crash when calling "update ssl ocsp-response" when an update...
Remi Tricot-Le Breton [Thu, 14 Mar 2024 14:38:32 +0000 (15:38 +0100)] 
BUG/MEDIUM: ssl: Fix crash when calling "update ssl ocsp-response" when an update is ongoing

The CLI command "update ssl ocsp-response" was forcefully removing an
OCSP response from the update tree regardless of whether it used to be
in it beforehand or not. But since the main OCSP upate task works by
removing the entry being currently updated from the update tree and then
reinserting it when the update process is over, it meant that in the CLI
command code we were modifying a structure that was already being used.

These concurrent accesses were not properly locked on the "regular"
update case because it was assumed that once an entry was removed from
the update tree, the update task was the only one able to work on it.

Rather than locking the whole update process, an "updating" flag was
added to the certificate_ocsp in order to prevent the "update ssl
ocsp-response" command from trying to update a response already being
updated.

An easy way to reproduce this crash was to perform two "simultaneous"
calls to "update ssl ocsp-response" on the same certificate. It would
then crash on an eb64_delete call in the main ocsp update task function.

This patch can be backported up to 2.8. Wait a little bit before
backporting.

15 months agoREGTESTS: ssl: Add OCSP related tests
Remi Tricot-Le Breton [Thu, 14 Mar 2024 14:38:31 +0000 (15:38 +0100)] 
REGTESTS: ssl: Add OCSP related tests

Add tests that combine the OCSP update mechanism and the various
preexisting commands that allow to manipulate certificates and
crt-lists.

15 months agoBUG/MAJOR: ocsp: Separate refcount per instance and per store
Remi Tricot-Le Breton [Thu, 14 Mar 2024 14:38:30 +0000 (15:38 +0100)] 
BUG/MAJOR: ocsp: Separate refcount per instance and per store

With the current way OCSP responses are stored, a single OCSP response
is stored (in a certificate_ocsp structure) when it is loaded during a
certificate parsing, and each SSL_CTX that references it increments its
refcount. The reference to the certificate_ocsp is kept in the SSL_CTX
linked to each ckch_inst, in an ex_data entry that gets freed when the
context is freed.
One of the downsides of this implementation is that if every ckch_inst
referencing a certificate_ocsp gets detroyed, then the OCSP response is
removed from the system. So if we were to remove all crt-list lines
containing a given certificate (that has an OCSP response), and if all
the corresponding SSL_CTXs were destroyed (no ongoing connection using
them), the OCSP response would be destroyed even if the certificate
remains in the system (as an unused certificate).
In such a case, we would want the OCSP response not to be "usable",
since it is not used by any ckch_inst, but still remain in the OCSP
response tree so that if the certificate gets reused (via an "add ssl
crt-list" command for instance), its OCSP response is still known as
well.
But we would also like such an entry not to be updated automatically
anymore once no instance uses it. An easy way to do it could have been
to keep a reference to the certificate_ocsp structure in the ckch_store
as well, on top of all the ones in the ckch_instances, and to remove the
ocsp response from the update tree once the refcount falls to 1, but it
would not work because of the way the ocsp response tree keys are
calculated. They are decorrelated from the ckch_store and are the actual
OCSP_CERTIDs, which is a combination of the issuer's name hash and key
hash, and the certificate's serial number. So two copies of the same
certificate but with different names would still point to the same ocsp
response tree entry.

The solution that answers to all the needs expressed aboved is actually
to have two reference counters in the certificate_ocsp structure, one
actual reference counter corresponding to the number of "live" pointers
on the certificate_ocsp structure, incremented for every SSL_CTX using
it, and one for the ckch stores.
If the ckch_store reference counter falls to 0, the corresponding
certificate must have been removed via CLI calls ('set ssl cert' for
instance).
If the actual refcount falls to 0, then no live SSL_CTX uses the
response anymore. It could happen if all the corresponding crt-list
lines were removed and there are no live SSL sessions using the
certificate anymore.
If any of the two refcounts becomes 0, we will always remove the
response from the auto update tree, because there's no point in spending
time updating an OCSP response that no new SSL connection will be able
to use. But the certificate_ocsp object won't be removed from the tree
unless both refcounts are 0.

Must be backported up to 2.8. Wait a little bit before backporting.

15 months agoBUG/MAJOR: connection: fix server used_conns with H2 + reuse safe
Amaury Denoyelle [Tue, 19 Mar 2024 09:34:45 +0000 (10:34 +0100)] 
BUG/MAJOR: connection: fix server used_conns with H2 + reuse safe

By default, backend connections are accounted by the server. This allows
to determine the number of idle connections to keep. A backend
connection can also be marked as private to prevent its reuse. It is
thus removed from server lists into the session list. As such, a private
connection is not accounted into server : conn_set_private() uses
srv_release_conn() to ensure this.

When using HTTP/2 on backend side with default http-reuse safe, the
above principle are mixed. Indeed, when a connection is first used, or
switches from idle to used, it is moved into the session list but it is
not flagged as private. This is done to prevent its sharing by different
clients to prevent head-of-line blocking issue. When all streams are
closed, the connection becomes idle again and is reinserted in the
server list. This has been introduced by the following patch :

  0d21deaded1a4bbd1e1700ab8386af1f1441ea73
  MEDIUM: backend: add reused conn to sess if mux marked as HOL blocking

When freeing a backend connection, special care is taken to ensure
server used counter is decremented. This is implemented into
conn_backend_deinit(). However, this function does this only if the
connection is not present in a session list. This is valid for private
connections. However, if a connection is non-private and present only
temporarily into a session list, the decrement operation won't be
executed despite the connection being accounted by the server.

This bug has several impacts. The server used counter won't be able to
reach its initial null value, even when all its connections are closed.
This can result in a wrong estimation of necessary idle connections,
which may cause unnecessary new connection usage. Also, this will
prevent definitely the server from being removed via "delete server" CLI
command.

This should be backported up to 2.4. Note that conn_backend_deinit() was
introduced in 2.9. For lesser versions, the change should be done
directly into conn_free().

15 months agoBUG/MEDIUM: http_ana: ignore NTLM for reuse aggressive/always and no H1
Amaury Denoyelle [Wed, 20 Mar 2024 08:25:03 +0000 (09:25 +0100)] 
BUG/MEDIUM: http_ana: ignore NTLM for reuse aggressive/always and no H1

Backend connections can be marked as private to prevent their sharing by
multiple clients. Now, this has become an exception as only two reasons
for data traffic can trigger this (checks are ignored here) :
* http-reuse never
* HTTP response with NTLM header

The first case is easy to manage as the connection is flagged as private
since its inception. However, the second case is dynamic as the
connection can be flagged anytime during its lifetime. When using a
backend protocol such as HTTP/2 with reuse mode aggressive or always, we
face a design issue as the connection would be marked as private,
despite potentially being shared by several clients at the same time.

This is conceptually invalid, but worst it can trigger crashes on MUX
stream detach callback depending on the order of release of the streams,
by calling session_check_idle_conn() with a NULL session. It could also
be possible to have several NTLM responses on a single connection for
different sessions. In this case, connection owner is still being
updated without attaching the connection to its correct session, which
ultimately would cause a crash on session_check_idle_conn with an
invalid session.

Here are two backtrace examples from GDB for such cases :

Thread 1 (Thread 0x7ff73e9fc700 (LWP 648859)):
 #0  session_check_idle_conn (conn=0x7ff72f597800, sess=0x0) at include/haproxy/session.h:209
 #1  h2_detach (sd=<optimized out>) at src/mux_h2.c:4520
 #2  0x000056151742be24 in sc_detach_endp (scp=scp@entry=0x7ff73e9f0f18) at src/stconn.c:376
 #3  0x000056151742c208 in sc_destroy (sc=<optimized out>) at src/stconn.c:444
 #4  0x0000561517370871 in stream_free (s=s@entry=0x7ff72a2dbd80) at src/stream.c:728
 #5  0x000056151737541f in process_stream (t=t@entry=0x7ff72d5e2620, context=0x7ff72a2dbd80, state=<optimized out>) at src/stream.c:2645
 #6  0x0000561517456cbb in run_tasks_from_lists (budgets=budgets@entry=0x7ff73e9f10d0) at src/task.c:632
 #7  0x00005615174576b9 in process_runnable_tasks () at src/task.c:876
 #8  0x000056151742275a in run_poll_loop () at src/haproxy.c:2996
 #9  0x0000561517422db1 in run_thread_poll_loop (data=<optimized out>) at src/haproxy.c:3195
 #10 0x00007ff789e081ca in start_thread () from /lib64/libpthread.so.0
 #11 0x00007ff789a39e73 in clone () from /lib64/libc.so.6

(gdb)
Thread 1 (Thread 0x7ff52e7fc700 (LWP 681458)):
 #0  0x0000556ebd6e7e69 in session_check_idle_conn (conn=0x7ff5787ff100, sess=0x7ff51d2539a0) at include/haproxy/session.h:209
 #1  h2_detach (sd=<optimized out>) at src/mux_h2.c:4520
 #2  0x0000556ebd7f3e24 in sc_detach_endp (scp=scp@entry=0x7ff52e7f0f18) at src/stconn.c:376
 #3  0x0000556ebd7f4208 in sc_destroy (sc=<optimized out>) at src/stconn.c:444
 #4  0x0000556ebd738871 in stream_free (s=s@entry=0x7ff520e28200) at src/stream.c:728
 #5  0x0000556ebd73d41f in process_stream (t=t@entry=0x7ff565783700, context=0x7ff520e28200, state=<optimized out>) at src/stream.c:2645
 #6  0x0000556ebd81ecbb in run_tasks_from_lists (budgets=budgets@entry=0x7ff52e7f10d0) at src/task.c:632
 #7  0x0000556ebd81f6b9 in process_runnable_tasks () at src/task.c:876
 #8  0x0000556ebd7ea75a in run_poll_loop () at src/haproxy.c:2996
 #9  0x0000556ebd7eadb1 in run_thread_poll_loop (data=<optimized out>) at src/haproxy.c:3195
 #10 0x00007ff5752081ca in start_thread () from /lib64/libpthread.so.0
 #11 0x00007ff574e39e73 in clone () from /lib64/libc.so.6
(gdb)

To solve this issue, simply ignore NTLM responses when using a
multiplexer with streams support and the connection is not already
attached to the session. The connection is not marked as private and
will continue to be shared freely accross clients. This is considered
conceptually valid as NTLM usage (rfc 4559) with HTTP is broken and was
designed only with HTTP/1.1 in mind. A side-effect of the change is that
SESS_FL_PREFER_LAST is also not set anymore on NTLM detection, which
allows following requests to be load-balanced accross several server
instances.

The original behavior is kept for HTTP/1 or if the connection is already
attached to the session. This last case happens when using HTTP/2 with
default http-reuse safe mode since the following patch :

  0d21deaded1a4bbd1e1700ab8386af1f1441ea73
  MEDIUM: backend: add reused conn to sess if mux marked as HOL blocking

This should be backported up to all stable releases. Up until 2.4, it
can be taken as-is. For lesser versions, above patch is not present. In
this case the condition should be restricted only to HTTP/1 usage :

  if (srv_conn && strcmp(srv_conn->mux->name, "H1") == 0) {

15 months agoBUG/MINOR: session: ensure conn owner is set after insert into session
Amaury Denoyelle [Wed, 20 Mar 2024 10:25:31 +0000 (11:25 +0100)] 
BUG/MINOR: session: ensure conn owner is set after insert into session

A crash could occured if a session_add_conn() would temporarily failed
when called via h2_detach(). In this case, connection owner is reset to
NULL. However, if this wasn't the last connection stream, the connection
won't be destroyed. When h2_detach() is recalled for another stream and
this time session_add_conn() succeeds, a crash will occur due to
session_check_idle_conn() invocation with a NULL connection owner.

To fix this, ensure connection owner is always set after
session_add_conn() success.

This bug is considered as minor as the only failure reason for
session_add_conn() is a pool allocation issue.

This should be backported up to all stable releases.

15 months agoBUG/MEDIUM: spoe: Return an invalid frame on recv if size is too small
Christopher Faulet [Mon, 18 Mar 2024 07:02:32 +0000 (08:02 +0100)] 
BUG/MEDIUM: spoe: Return an invalid frame on recv if size is too small

Frames with a too small size must be detected on receive and an error must
be triggered. It is especially important for frames of size 0. Otherwise,
because the frame length is used as return value, the frame is ignored (0 is
the return value to state the frame must be ignored). It is an issue because
in this case, outgoing data, the 4 bytes representing the frame size, are
never consumed. If the agent also closes the connection, this leads to a
wakeup loop because outgoing data are stuck and a shutdown is pending.

In addition, all pending outgoing data are systematcially skipped when the
applet is in SPOE_APPCTX_ST_END state.

The patch should fix the issue #2490. It must be backported to all stable
versions.

15 months agoCI: temporarily adjust kernel entropy to work with ASAN/clang
Ilia Shipitsin [Sun, 17 Mar 2024 16:01:39 +0000 (17:01 +0100)] 
CI: temporarily adjust kernel entropy to work with ASAN/clang

clang runtime (shipped with clang14) is not compatible with recent
Ubuntu kernels

more details: https://github.com/actions/runner-images/issues/9491

15 months agoCLEANUP: assorted typo fixes in the code and comments
Ilia Shipitsin [Sun, 17 Mar 2024 16:01:38 +0000 (17:01 +0100)] 
CLEANUP: assorted typo fixes in the code and comments

This is 40th iteration of typo fixes

15 months agoMINOR: spoe: Add SPOE filters in the exposed deprecated directives
Christopher Faulet [Fri, 15 Mar 2024 08:08:08 +0000 (09:08 +0100)] 
MINOR: spoe: Add SPOE filters in the exposed deprecated directives

It is the first deprecated directive exposed via the
'expose-deprecated-directives' global option. This way, it is possible to
silent the warning about the SPOE uses.

15 months agoMINOR: cfgparse: Add a global option to expose deprecated directives
Christopher Faulet [Fri, 15 Mar 2024 08:01:11 +0000 (09:01 +0100)] 
MINOR: cfgparse: Add a global option to expose deprecated directives

Similarly to "expose-exprimental-directives" option, there is no a global
option to expose some deprecated directives. Idea is to have a way to silent
warnings about deprecated directives when there is no alternative solution.

Of course, deprecated directives covered by this option are not listed and
may change. It is only a best effort to let users upgrade smoothly.

15 months agoMAJOR: spoe: Deprecate the SPOE filter
Christopher Faulet [Thu, 14 Mar 2024 10:53:27 +0000 (11:53 +0100)] 
MAJOR: spoe: Deprecate the SPOE filter

As announced on the ML few weeks (months ?) ago and on several GH issues,
the SPOE is now deprecated. Sadly, this filter should be refactored to work
properly. It was implemented as a functionnal PoC for the 1.7 and since
then, no time was invest to improve it and make it truly maintainable in
time. Worst, other parts of HAProxy evolve, especially applets part, making
maintenance ever more expensive.

Instead of keeping the SPOE filter in a this state and always reply to users
encountering issues or limitations that it is far from perfect but we cannot
work on it for now, we decided to deprecate it.

We can still change our mind before the 3.0.0 release if the situation
evolves. Otherwise the filter will be removed or marked as unmaintained for
the 3.1. If the situation does not change, it means the 3.0 will be the last
version with a true SPOE support.

15 months agoBUG/MINOR: spoe: Be sure to be able to quickly close IDLE applets on soft-stop
Christopher Faulet [Thu, 14 Mar 2024 09:49:01 +0000 (10:49 +0100)] 
BUG/MINOR: spoe: Be sure to be able to quickly close IDLE applets on soft-stop

On soft-stop, we try, as far as possible, to process all pending messages
before closing SPOE applets. However, in sync mode, when an applets waiting
for a response receives the ACK frame, it is switched to IDLE state without
checking if it may be closed. In this case, we will wait the idle timeout
before closing de applet, delaying the soft-stop.

To reduce this delay, on soft-stop, IDLE applets are woken up. On the next
wakeup, the applet will try to process pending messages or will be
closed.

This patch should be backported to all stable versions.

15 months agoBUG/MEDIUM: spoe: Don't rely on stream's expiration to detect processing timeout
Christopher Faulet [Thu, 14 Mar 2024 09:21:56 +0000 (10:21 +0100)] 
BUG/MEDIUM: spoe: Don't rely on stream's expiration to detect processing timeout

On stream side, the SPOE filter relied on the stream's expiration date to be
woken up and be able to detect processing timeout. However, the stream
expiration date must not be updated this way. Mainly because it may be
overwritten at the end of process_stream(). In the worst case, it is set to
TICK_ETERNITY for any reason. In this case, it is impossible to detect the
SPOE filter must time out and abort the processing.

The right way to do is to set an analysis expiration date on the
corresponding channel, depending on the direction. This expiration date will
be used to compute the stream's expiration date at the end of
process_stream().

This patch may be related to issue #2478. It must be backported to all
stable versions.

15 months agoBUG/MAJOR: server: do not delete srv referenced by session
Amaury Denoyelle [Tue, 12 Mar 2024 13:09:55 +0000 (14:09 +0100)] 
BUG/MAJOR: server: do not delete srv referenced by session

A server can only be deleted if there is no elements which reference it.
This is taken care via srv_check_for_deletion(), most notably for active
and idle connections.

A special case occurs for connections directly managed by a session.
This is for so-called private connections, when using http-reuse never
or H2 + http-reuse safe for example. In this case. server does not
account these connections into its idle lists. This caused a bug as the
server is deleted despite the session still being able to access it.

To properly fix this, add a new referencing element into the server for
these session connections. A mt_list has been chosen for this. On
default http-reuse, private connections are typically not used so it
won't make any difference. If using H2 servers, or more generally when
dealing with private connections, insert/delete should typically occur
only once per session lifetime so impact on performance should be
minimal.

This should be backported up to 2.4. Note that srv_check_for_deletion()
was introduced in 3.0 dev tree. On backport, the extra condition in it
should be placed in cli_parse_delete_server() instead.

15 months agoMINOR: session: rename private conns elements
Amaury Denoyelle [Thu, 14 Mar 2024 10:24:10 +0000 (11:24 +0100)] 
MINOR: session: rename private conns elements

By default, backend connections are attached to a server instance. This
allows to implement connection reuse. However, in some particular cases,
connection cannot be shared accross several clients. These connections
are considered and private and are attached to the session instance
instead.

These private connections are also indexed by the target server to not
mix them. All of this is implemented via a dedicated structure
previously named struct sess_srv_list.

Rename it to better reflect its usage to struct sess_priv_conns. Also
rename its internal members and all of the associated functions.

This commit is only a renaming, thus no functional impact is expected.

15 months agoBUG/MINOR: listener: Don't schedule frontend without task in listener_release()
Christopher Faulet [Thu, 14 Mar 2024 08:29:09 +0000 (09:29 +0100)] 
BUG/MINOR: listener: Don't schedule frontend without task in listener_release()

null pointer dereference was reported by Coverity in listener_release()
function. Indeed, we must not try to schedule frontend without task when a
limit is still blocking the frontend. This issue was introduced by commit
65ae1347c7 ("BUG/MINOR: listener: Wake proxy's mngmt task up if necessary on
session release")

This patch should fix issue #2488. It must be backported to all stable
version with the commit above.

15 months agoBUG/MINOR: listener: Wake proxy's mngmt task up if necessary on session release
Christopher Faulet [Tue, 12 Mar 2024 06:31:56 +0000 (07:31 +0100)] 
BUG/MINOR: listener: Wake proxy's mngmt task up if necessary on session release

When a session is released, listener_release() function is called to notify
the listener. It is an opportunity to resume limited/full listeners. We
first try to resume the listener owning the released session, then all
limited listeners in the global queue and finally all limited listeners in
the frontend's waiting queue. This last step is only performed if there is
no limit applied on the frontend. Nothing is performed if the session rate is
still limited. And it is an issue because if this happens for the last
listener's session, there is no other event to wake the frontend's managment
task up and the listener remains in the limited state.

To fix the issue, when a limit is still applied on the frontent, we must
compute the new wake up date from the sessions rate and schedule the
frontend's managment task.

It is easy to reproduce the issue in SSL by setting a maxconn and a rate
limit on sessions.

This patch should fix the issue #2476. It must be backported to all stable
versions.

15 months agoCI: github: add -dI to haproxy arguments
William Lallemand [Wed, 13 Mar 2024 10:19:17 +0000 (11:19 +0100)] 
CI: github: add -dI to haproxy arguments

-dI is useful when running with ASAN, allow to fork addr2line

15 months agoMINOR: debug: enable insecure fork on the command line
William Lallemand [Wed, 13 Mar 2024 10:08:50 +0000 (11:08 +0100)] 
MINOR: debug: enable insecure fork on the command line

-dI allow to enable "insure-fork-wanted" directly from the command line,
which is useful when you want to run ASAN with addr2line with a lot of
configuration files without editing them.

15 months agoBUG/MEDIUM: hlua: streams don't support mixing lua-load with lua-load-per-thread...
Aurelien DARRAGON [Tue, 12 Mar 2024 16:05:54 +0000 (17:05 +0100)] 
BUG/MEDIUM: hlua: streams don't support mixing lua-load with lua-load-per-thread (2nd try)

While trying to reproduce another crash case involving lua filters
reported by @bgrooot on GH #2467, we found out that mixing filters loaded
from different contexts ('lua-load' vs 'lua-load-per-thread') for the same
stream isn't supported and may even cause the process to crash.

Historically, mixing lua-load and lua-load-per-threads for a stream wasn't
supported, but this changed thanks to 0913386 ("BUG/MEDIUM: hlua: streams
don't support mixing lua-load with lua-load-per-thread").

However, the above fix didn't consider lua filters's use-case properly:
unlike lua fetches, actions or even services, lua filters don't simply
use the stream hlua context as a "temporary" hlua running context to
process some hlua code. For fetches, actions.. hlua executions are
processed sequentially, so we simply reuse the hlua context from the
previous action/fetch to run the next one (this allows to bypass memory
allocations and initialization, thus it increases performance), unless
we need to run on a different hlua state-id, in which case we perform a
reset of the hlua context.

But this cannot work with filters: indeed, once registered, a filter will
last for the whole stream duration. It means that the filter will rely
on the stream hlua context from ->attach() to ->detach(). And here is the
catch, if for the same stream we register 2 lua filters from different
contexts ('lua-load' + 'lua-load-per-thread'), then we have an issue,
because the hlua stream will be re-created each time we switch between
runtime contexts, which means each time we switch between the filters (may
happen for each stream processing step), and since lua filters rely on the
stream hlua to carry context between filtering steps, this context will be
lost upon a switch. Given that lua filters code was not designed with that
in mind, it would confuse the code and cause unexpected behaviors ranging
from lua errors to crashing process.

So here we take another approach: instead of re-creating the stream hlua
context each time we switch between "global" and "per-thread" runtime
context, let's have both of them inside the stream directly as initially
suggested by Christopher back then when talked about the original issue.

For this we leverage hlua_stream_ctx_prepare() and hlua_stream_ctx_get()
helper functions which return the proper hlua context for a given stream
and state_id combination.

As for debugging infos reported after ha_panic(), we check for both hlua
runtime contexts to check if one of them was active when the panic occured
(only 1 runtime ctx per stream may be active at a given time).

This should be backported to all stable versions with 0913386
("BUG/MEDIUM: hlua: streams don't support mixing lua-load with lua-load-per-thread")

This commit depends on:
 - "DEBUG: lua: precisely identify if stream is stuck inside lua or not"
   [for versions < 2.9 the ha_thread_dump_one() part should be skipped]
 - "MINOR: hlua: use accessors for stream hlua ctx"

For 2.4, the filters API didn't exist. However it may be a good idea to
backport it anyway because ->set_priv()/->get_priv() from tcp/http lua
applets may also be affected by this bug, plus it will ease code
maintenance. Of course, filters-related parts should be skipped in this
case.

15 months agoMINOR: hlua: use accessors for stream hlua ctx
Aurelien DARRAGON [Tue, 12 Mar 2024 16:00:25 +0000 (17:00 +0100)] 
MINOR: hlua: use accessors for stream hlua ctx

Change hlua_stream_ctx_prepare() prototype so that it now returns the
proper hlua ctx on success instead of returning a boolean.

Add hlua_stream_ctx_get() to retrieve hlua ctx out of a given stream.

This way we may easily change the storage mechanism for hlua stream in
the future without extensive code changes.

No backport needed unless a commit depends on it.

15 months agoDEBUG: lua: precisely identify if stream is stuck inside lua or not
Aurelien DARRAGON [Mon, 11 Mar 2024 23:22:44 +0000 (00:22 +0100)] 
DEBUG: lua: precisely identify if stream is stuck inside lua or not

When ha_panic() is called by the watchdog, we try to guess from
ha_task_dump() and ha_thread_dump_one() if the thread was stuck while
executing lua from the stream context. However we consider this is the
case by simply checking if the stream hlua context was set, but this is
not very precise because if the hlua context is set, then it simply means
that at least one lua instruction was executed at the stream level, not
that the stuck was currently executing lua when the panic occured.

This is especially true with filters, one could simply register a lua
filter that does nothing but this will still end up initializing the
stream hlua context for each stream. If the thread end up being stuck
during the stream handling, then debug dumping functions will report
that the stream was stuck while handling lua, which is not necessarilly
true, and could in fact confuse us even more.

So here we take another approach, we add the BUSY flag to hlua context:
this flag is set by hlua_ctx_resume() around lua_resume() call, this way
we can precisely tell if the thread was handling lua when it was
interrupted, and we rely on this flag in debug functions to check if the
thread was effectively stuck inside lua or not while processing the stream

No backport needed unless a commit depends on it.

15 months agoBUG/MINOR: hlua: fix missing lock in hlua_filter_delete()
Aurelien DARRAGON [Mon, 11 Mar 2024 12:51:03 +0000 (13:51 +0100)] 
BUG/MINOR: hlua: fix missing lock in hlua_filter_delete()

hlua_filter_delete() calls hlua_unref() on the stream hlua stack, but
we should own the lock prior to manipulating the stack.

This should be backported up to 2.6.

15 months agoBUG/MINOR: hlua: missing lock in hlua_filter_new()
Aurelien DARRAGON [Mon, 11 Mar 2024 12:50:41 +0000 (13:50 +0100)] 
BUG/MINOR: hlua: missing lock in hlua_filter_new()

This is a complementary patch to 8670db7 ("BUG/MAJOR: hlua: improper lock
usage with hlua_ctx_resume()") for hlua_filter_new().

Indeed, the HLUA_E_ERRMSG case still relies on the lua stack but didn't
take the lock to do so.

This should be backported up to 2.6.