]> git.ipfire.org Git - thirdparty/haproxy.git/commit
BUG/MAJOR: server/addr: fix a race during server addr:svc_port updates
authorAurelien DARRAGON <adarragon@haproxy.com>
Mon, 13 Nov 2023 17:14:24 +0000 (18:14 +0100)
committerChristopher Faulet <cfaulet@haproxy.com>
Wed, 29 Nov 2023 07:59:27 +0000 (08:59 +0100)
commitcd994407a9545a8d84e410dc0cc18c30966b70d8
treefb21b37d3b7105fccfec15d7f356acce1dfbb77c
parentcb3ec978fd182f770ba9e9688143737f26801a36
BUG/MAJOR: server/addr: fix a race during server addr:svc_port updates

For inet families (IP4/IP6), it is expected that server's addr/port might
be updated at runtime from DNS, cli or lua for instance.

Such updates were performed under the server's lock.

Unfortunately, most readers such as backend.c or sink.c perform the read
without taking server's lock because they can't afford slowing down their
processing for a type of event which is normally rare. But this could
result in bad values being read for the server addr:svc_port tuple (ie:
during connection etablishment) as a result of concurrent updates from
external components, which can obviously cause some undesirable effects.

Instead of slowing the readers down, as we consider server's addr changes
are relatively rare, we take another approach and try to update the
addr:port atomically by performing changes under full thread isolation
when a new change is requested. The changes are performed by a dedicated
task which takes care of isolating the current thread and doesn't depend
on other threads (independent code path) to protect against dead locks.

As such, server's addr:port changes will now be performed atomically, but
they will not be processed instantly, they will be translated to events
that the dedicated task will pick up from time to time to apply the
pending changes.

This bug existed for a very long time and has never been reported so
far. It was discovered by reading the code during the implementation
of log backend ("mode log" in backends). As it involves changes in
sensitive areas as well as thread isolation, it is probably not
worth considering backporting it for now, unless it is proven that it
will help to solve bugs that are actually encountered in the field.

This patch depends on:
 - 24da4d3 ("MINOR: tools: use const for read only pointers in ip{cmp,cpy}")
 - c886fb5 ("MINOR: server/ip: centralize server ip updates")
 - event_hdl API (which was first seen on 2.8) +
   683b2ae ("MINOR: server/event_hdl: add SERVER_INETADDR event") +
   BUG/MEDIUM: server/event_hdl: memory overrun in _srv_event_hdl_prepare_inetaddr() +
   "MINOR: event_hdl: add global tunables"

   Note that the patch may be reworked so that it doesn't depend on
   event_hdl API for older versions, the approach would remain the same:
   this would result in a larger patch due to the need to manually
   implement a global queue of pending updates with its dedicated task
   responsible for picking updates and comitting them. An alternative
   approach could consist in per-server, lock-protected, temporary
   addr:svc_port storage dedicated to "updaters" were only the most
   recent values would be kept. The sync task would then use them as
   source values to atomically update the addr:svc_port members that the
   runtime readers are actually using.
src/server.c