mirrors/haproxy
1
0
Fork 0
mirror of https://git.haproxy.org/git/haproxy.git/ synced 2025-09-28 17:21:32 +02:00
Code Issues Projects Releases Wiki Activity

BUG/MINOR: server/del: fix srv->next pointer consistency

Browse Source 
We recently discovered a bug which affects dynamic server deletion:

When a server is deleted, it is removed from the "visible" server list.
But as we've seen in previous commit
("MINOR: server: add SRV_F_DELETED flag"), it can still be accessed by
someone who keeps a reference on it (waiting for the final srv_drop()).
Throughout this transient state, server ptr is still valid (may be
dereferenced) and the flag SRV_F_DELETED is set.

However, as the server is not part of server list anymore, we have
an issue: srv->next pointer won't be updated anymore as the only place
where we perform such update is in cli_parse_delete_server() by
iterating over the "visible" server list.

Because of this, we cannot guarantee that a server with the
SRV_F_DELETED flag has a valid 'next' ptr: 'next' could be pointing
to a fully removed (already freed) server.

This problem can be easily demonstrated with server dumping in
the stats:

server list dumping is performed in stats_dump_proxy_to_buffer()
The function can be interrupted and resumed later by design.
ie: output buffer is full: partial dump and finish the dump after
the flush

This is implemented by calling srv_take() on the server being dumped,
and only releasing it when we're done with it using srv_drop().
(drop can be delayed after function resume if buffer is full)

While the function design seems OK, it works with the assumption that
srv->next will still be valid after the function resumes, which is
not true. (especially if multiple servers are being removed in between
the 2 dumping attempts)

In practice, this did not cause any crash yet (at least this was not
reported so far), because server dumping is so fast that it is very
unlikely that multiple server deletions make their way between 2
dumping attempts in most setups. But still, this is a problem that we
need to address because some upcoming work might depend on this
assumption as well and for the moment it is not safe at all.

========================================================================

Here is a quick reproducer:

With this patch, we're creating a large deletion window of 3s as soon
as we reach a server named "t2" while iterating over the list.

This will give us plenty of time to perform multiple deletions before
the function is resumed.

    |  diff --git a/src/stats.c b/src/stats.c
    |  index 84a4f9b6e..15e49b4cd 100644
    |  --- a/src/stats.c
    |  +++ b/src/stats.c
    |  @@ -3189,11 +3189,24 @@ int stats_dump_proxy_to_buffer(struct stconn *sc, struct htx *htx,
    |                    * Temporarily increment its refcount to prevent its
    |                    * anticipated cleaning. Call free_server to release it.
    |                    */
    |  +                struct server *orig = ctx->obj2;
    |                   for (; ctx->obj2 != NULL;
    |                          ctx->obj2 = srv_drop(sv)) {
    |
    |                           sv = ctx->obj2;
    |  +                        printf("sv = %s\n", sv->id);
    |                           srv_take(sv);
    |  +                        if (!strcmp("t2", sv->id) && orig == px->srv) {
    |  +                                printf("deletion window: 3s\n");
    |  +                                thread_idle_now();
    |  +                                thread_harmless_now();
    |  +                                sleep(3);
    |  +                                thread_harmless_end();
    |  +
    |  +                                thread_idle_end();
    |  +
    |  +                                goto full; /* simulate full buffer */
    |  +                        }
    |
    |                           if (htx) {
    |                                   if (htx_almost_full(htx))
    |  @@ -4353,6 +4366,7 @@ static void http_stats_io_handler(struct appctx *appctx)
    |           struct channel *res = sc_ic(sc);
    |           struct htx *req_htx, *res_htx;
    |
    |  +        printf("http dump\n");
    |           /* only proxy stats are available via http */
    |           ctx->domain = STATS_DOMAIN_PROXY;
    |

Ok, we're ready, now we start haproxy with the following conf:

       global
          stats socket /tmp/ha.sock  mode 660  level admin  expose-fd listeners thread 1-1
          nbthread 2

       frontend stats
           mode http
           bind *:8081 thread 2-2
           stats enable
           stats uri /

       backend farm
               server t1 127.0.0.1:1899  disabled
               server t2 127.0.0.1:18999 disabled
               server t3 127.0.0.1:18998 disabled
               server t4 127.0.0.1:18997 disabled

And finally, we execute the following script:

       curl localhost:8081/stats&
       sleep .2
       echo "del server farm/t2" | nc -U /tmp/ha.sock
       echo "del server farm/t3" | nc -U /tmp/ha.sock

This should be enough to reveal the issue, I easily manage to
consistently crash haproxy with the following reproducer:

    http dump
    sv = t1
    http dump
    sv = t1
    sv = t2
    deletion window = 3s
    [NOTICE]   (2940566) : Server deleted.
    [NOTICE]   (2940566) : Server deleted.
    http dump
    sv = t2
    sv = �����U
    [1]    2940566 segmentation fault (core dumped)  ./haproxy -f ttt.conf

========================================================================

To fix this, we add prev_deleted mt_list in server struct.
For a given "visible" server, this list will contain the pending
"deleted" servers references that point to it using their 'next' ptr.

This way, whenever this "visible" server is going to be deleted via
cli_parse_delete_server() it will check for servers in its
'prev_deleted' list and update their 'next' pointer so that they no
longer point to it, and then it will push them in its
'next->prev_deleted' list to transfer the update responsibility to the
next 'visible' server (if next != NULL).

Then, following the same logic, the server about to be removed in
cli_parse_delete_server() will push itself as well into its
'next->prev_deleted' list (if next != NULL) so that it may still use its
'next' ptr for the time it is in transient removal state.

In srv_drop(), right before the server is finally freed, we make sure
to remove it from the 'next->prev_deleted' list so that 'next' won't
try to perform the pointers update for this server anymore.
This has to be done atomically to prevent 'next' srv from accessing a
purged server.

As a result:
  for a valid server, either deleted or not, 'next' ptr will always
  point to a non deleted (ie: visible) server.

With the proposed fix, and several removal combinations (including
unordered cli_parse_delete_server() and srv_drop() calls), I cannot
reproduce the crash anymore.

Example tricky removal sequence that is now properly handled:

sv list: t1,t2,t3,t4,t5,t6

ops:
   take(t2)
   del(t4)
   del(t3)
   del(t5)
   drop(t3)
   drop(t4)
   drop(t5)
   drop(t2)
This commit is contained in:
Aurelien DARRAGON 2023-02-01 17:22:32 +01:00 committed by Christopher Faulet
parent 75b9d1c041
commit f175b08bfb
2 changed files with 29 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
include/haproxy/server-t.h
View File

@ -237,6 +237,7 @@ struct server {
unsigned int pp_opts; /* proxy protocol options (SRV_PP_*) */
struct list global_list; /* attach point in the global servers_list */
struct server *next;
struct mt_list prev_deleted; /* deleted servers with 'next' ptr pointing to us */
int cklen; /* the len of the cookie, to speed up checks */
int rdr_len; /* the length of the redirection prefix */
char *cookie; /* the id set in the cookie */

28
src/server.c
View File

@ -2368,6 +2368,7 @@ struct server *new_server(struct proxy *proxy)
LIST_APPEND(&servers_list, &srv->global_list);
LIST_INIT(&srv->srv_rec_item);
LIST_INIT(&srv->ip_rec_item);
MT_LIST_INIT(&srv->prev_deleted);
event_hdl_sub_list_init(&srv->e_subs);
srv->next_state = SRV_ST_RUNNING; /* early server setup */
@ -2429,6 +2430,13 @@ struct server *srv_drop(struct server *srv)
goto end;
}
/* make sure we are removed from our 'next->prev_deleted' list
* This doesn't require full thread isolation as we're using mt lists
* However this could easily be turned into regular list if required
* (with the proper use of thread isolation)
*/
MT_LIST_DELETE(&srv->prev_deleted);
task_destroy(srv->warmup);
task_destroy(srv->srvrq_check);
@ -4998,6 +5006,7 @@ static int cli_parse_delete_server(char **args, char *payload, struct appctx *ap
struct proxy *be;
struct server *srv;
char *be_name, *sv_name;
struct server *prev_del;
if (!cli_has_level(appctx, ACCESS_LVL_ADMIN))
return 1;
@ -5098,6 +5107,25 @@ static int cli_parse_delete_server(char **args, char *payload, struct appctx *ap
next->next = srv->next;
}
/* Some deleted servers could still point to us using their 'next',
* update them as needed
* Please note the small race between the POP and APPEND, although in
* this situation this is not an issue as we are under full thread
* isolation
*/
while ((prev_del = MT_LIST_POP(&srv->prev_deleted, struct server *, prev_deleted))) {
/* update its 'next' ptr */
prev_del->next = srv->next;
if (srv->next) {
/* now it is our 'next' responsibility */
MT_LIST_APPEND(&srv->next->prev_deleted, &prev_del->prev_deleted);
}
}
/* we ourselves need to inform our 'next' that we will still point it */
if (srv->next)
MT_LIST_APPEND(&srv->next->prev_deleted, &srv->prev_deleted);
/* remove srv from addr_node tree */
eb32_delete(&srv->conf.id);
ebpt_delete(&srv->conf.name);