BUG/MEDIUM: mworker/listener: ambiguous use of RX_F_INHERITED with shards

The RX_F_INHERITED flag was ambiguous, as it was used to mark both
listeners inherited from the parent process and listeners duplicated
from another local receiver. This could lead to incorrect behavior
concerning socket unbinding and suspension.

This commit refactors the handling of inherited listeners by splitting
the RX_F_INHERITED flag into two more specific flags:

- RX_F_INHERITED_FD: Indicates a listener inherited from the parent
  process via its file descriptor. These listeners should not be unbound
  by the master.

- RX_F_INHERITED_SOCK: Indicates a listener that shares a socket with
  another one, either by being inherited from the parent or by being
  duplicated from another local listener. These listeners should not be
  suspended or resumed individually.

Previously, the sharding code was unconditionally using RX_F_INHERITED
when duplicating a file descriptor. In HAProxy versions prior to 3.1,
this led to a file descriptor leak for duplicated unix stats sockets in
the master process. This would eventually cause the master to crash with
a BUG_ON in fd_insert() once the file descriptor limit was reached.

This must be backported as far as 3.0. Branches earlier than 3.0 are
affected but would need a different patch as the logic is different.

diff --git a/include/haproxy/receiver-t.h b/include/haproxy/receiver-t.h
index 90f52aae533f1b55181ad1988da4be9202615d6f..e4e9b292262f2c681b4f3acdd133b92c2a259c40 100644 (file)
--- a/include/haproxy/receiver-t.h
+++ b/include/haproxy/receiver-t.h
@@ -33,11 +33,12 @@
 
 /* Bit values for receiver->flags */
 #define RX_F_BOUND              0x00000001  /* receiver already bound */
-#define RX_F_INHERITED          0x00000002  /* inherited FD from the parent process (fd@) or duped from another local receiver */
+#define RX_F_INHERITED_FD       0x00000002  /* inherited FD from the parent process (fd@) */
 #define RX_F_MWORKER            0x00000004  /* keep the FD open in the master but close it in the children */
 #define RX_F_MUST_DUP           0x00000008  /* this receiver's fd must be dup() from a reference; ignore socket-level ops here */
 #define RX_F_NON_SUSPENDABLE    0x00000010  /* this socket cannot be suspended hence must always be unbound */
 #define RX_F_PASS_PKTINFO       0x00000020  /* pass pktinfo in received messages */
+#define RX_F_INHERITED_SOCK     0x00000040  /* inherited sock that could be duped from another local receiver */
 
 /* Bit values for rx_settings->options */
 #define RX_O_FOREIGN            0x00000001  /* receives on foreign addresses */

diff --git a/src/cli.c b/src/cli.c
index cbcecff43e963ba5c98935f8b82582d190e3a530..32365d70a9e0a232a2a0ede33ee4341936db3f5c 100644 (file)
--- a/src/cli.c
+++ b/src/cli.c
@@ -3894,7 +3894,7 @@ int mworker_cli_global_proxy_new_listener(struct mworker_proc *proc)
        list_for_each_entry(l, &bind_conf->listeners, by_bind) {
                HA_ATOMIC_INC(&unstoppable_jobs);
                /* it's a sockpair but we don't want to keep the fd in the master */
-               l->rx.flags &= ~RX_F_INHERITED;
+               l->rx.flags &= ~RX_F_INHERITED_FD;
                global.maxsock++; /* for the listening socket */
        }
 

diff --git a/src/listener.c b/src/listener.c
index ba1385e477ef93c03c101da451c2e21a851e291f..a86e946ce3efe5b1e32df6b92ceb452b9224b496 100644 (file)
--- a/src/listener.c
+++ b/src/listener.c
@@ -846,7 +846,7 @@ int create_listeners(struct bind_conf *bc, const struct sockaddr_storage *ss,
                proto->add(proto, l);
 
                if (fd != -1)
-                       l->rx.flags |= RX_F_INHERITED;
+                       l->rx.flags |= RX_F_INHERITED_FD|RX_F_INHERITED_SOCK;
 
                guid_init(&l->guid);
 
@@ -1844,6 +1844,12 @@ int bind_complete_thread_setup(struct bind_conf *bind_conf, int *err_code)
                                        /* it has been allocated already in the previous round */
                                        shard_info_attach(&new_li->rx, ref->rx.shard_info);
                                        new_li->rx.flags |= RX_F_MUST_DUP;
+                                       /* taking the other one's FD will result in it being marked
+                                        * extern and being dup()ed. Let's mark the receiver as
+                                        * inherited so that it properly bypasses all second-stage
+                                        * setup/unbind and avoids being passed to new processes.
+                                        */
+                                       new_li->rx.flags |= ref->rx.flags & RX_F_INHERITED_SOCK;
                                }
 
                                gmask &= gmask - 1; // drop lowest bit

diff --git a/src/proto_sockpair.c b/src/proto_sockpair.c
index e9271605f27b0fd28678d3bc5803fcbe20dfef71..3c3cd543e12aeee5163658aa90b71355e265cc86 100644 (file)
--- a/src/proto_sockpair.c
+++ b/src/proto_sockpair.c
@@ -151,12 +151,6 @@ int sockpair_bind_receiver(struct receiver *rx, char **errmsg)
                        err |= ERR_RETRYABLE;
                        goto bind_ret_err;
                }
-               /* taking the other one's FD will result in it being marked
-                * extern and being dup()ed. Let's mark the receiver as
-                * inherited so that it properly bypasses all second-stage
-                * setup and avoids being passed to new processes.
-                */
-               rx->flags |= RX_F_INHERITED;
                rx->fd = rx->shard_info->ref->fd;
        }
 

diff --git a/src/proto_tcp.c b/src/proto_tcp.c
index 93115f42f99a9482a558b9b327f436d904670d97..1f8c8f293d1367932496d80fc2ecfd4bc6fb6b30 100644 (file)
--- a/src/proto_tcp.c
+++ b/src/proto_tcp.c
@@ -909,7 +909,7 @@ static int tcp_suspend_receiver(struct receiver *rx)
         * parent process and any possible subsequent worker inheriting it.
         * Thus we just stop receiving from it.
         */
-       if (rx->flags & RX_F_INHERITED)
+       if (rx->flags & RX_F_INHERITED_SOCK)
                goto done;
 
        if (connect(rx->fd, &sa, sizeof(sa)) < 0)
@@ -945,7 +945,7 @@ static int tcp_resume_receiver(struct receiver *rx)
        if (rx->fd < 0)
                return 0;
 
-       if ((rx->flags & RX_F_INHERITED) || listen(rx->fd, listener_backlog(l)) == 0) {
+       if ((rx->flags & RX_F_INHERITED_SOCK) || listen(rx->fd, listener_backlog(l)) == 0) {
                fd_want_recv(l->rx.fd);
                return 1;
        }

diff --git a/src/proto_udp.c b/src/proto_udp.c
index bedc6f6cba01960ebcfbbacc8b7570928fa6d397..764124116dcb79512a80de8c6fcf914779be32b5 100644 (file)
--- a/src/proto_udp.c
+++ b/src/proto_udp.c
@@ -220,7 +220,7 @@ int udp_suspend_receiver(struct receiver *rx)
        /* we never do that with a shared FD otherwise we'd break it in the
         * parent process and any possible subsequent worker inheriting it.
         */
-       if (rx->flags & RX_F_INHERITED)
+       if (rx->flags & RX_F_INHERITED_SOCK)
                goto done;
 
        if (getsockname(rx->fd, (struct sockaddr *)&ss, &len) < 0)
@@ -248,7 +248,7 @@ int udp_resume_receiver(struct receiver *rx)
        if (rx->fd < 0)
                return 0;
 
-       if (!(rx->flags & RX_F_INHERITED) && connect(rx->fd, &sa, sizeof(sa)) < 0)
+       if (!(rx->flags & RX_F_INHERITED_SOCK) && connect(rx->fd, &sa, sizeof(sa)) < 0)
                return -1;
 
        fd_want_recv(rx->fd);

diff --git a/src/sock.c b/src/sock.c
index 3644a1a9156d5b15deec0ee3727d627a20dad1c7..bda07024eb1f1e740ab77e007c6916e94a7caa9e 100644 (file)
--- a/src/sock.c
+++ b/src/sock.c
@@ -381,7 +381,7 @@ void sock_unbind(struct receiver *rx)
                return;
 
        if (!stopping && master &&
-           rx->flags & RX_F_INHERITED)
+           rx->flags & RX_F_INHERITED_FD)
                return;
 
        rx->flags &= ~RX_F_BOUND;

diff --git a/src/sock_inet.c b/src/sock_inet.c
index 4ef77f001e9763a99f8d115e2e1eca59644fbea4..342f3ac0f3d802dd73b9be9745b0f2decc43db71 100644 (file)
--- a/src/sock_inet.c
+++ b/src/sock_inet.c
@@ -327,12 +327,6 @@ int sock_inet_bind_receiver(struct receiver *rx, char **errmsg)
                        err |= ERR_RETRYABLE;
                        goto bind_ret_err;
                }
-               /* taking the other one's FD will result in it being marked
-                * extern and being dup()ed. Let's mark the receiver as
-                * inherited so that it properly bypasses all second-stage
-                * setup and avoids being passed to new processes.
-                */
-               rx->flags |= RX_F_INHERITED;
                rx->fd = rx->shard_info->ref->fd;
        }
 
@@ -467,7 +461,7 @@ int sock_inet_bind_receiver(struct receiver *rx, char **errmsg)
        fd_insert(fd, rx->owner, rx->iocb, rx->bind_tgroup, rx->bind_thread);
 
        /* for now, all regularly bound TCP listeners are exportable */
-       if (!(rx->flags & RX_F_INHERITED))
+       if (!(rx->flags & (RX_F_INHERITED_FD|RX_F_INHERITED_SOCK)))
                HA_ATOMIC_OR(&fdtab[fd].state, FD_EXPORTED);
 
  bind_return:

diff --git a/src/sock_unix.c b/src/sock_unix.c
index 0d00a15a4df35bf995193217cca22459804cef27..36d60fab39897ec102ee438564a9105708404d6f 100644 (file)
--- a/src/sock_unix.c
+++ b/src/sock_unix.c
@@ -237,12 +237,6 @@ int sock_unix_bind_receiver(struct receiver *rx, char **errmsg)
                        err |= ERR_RETRYABLE;
                        goto bind_ret_err;
                }
-               /* taking the other one's FD will result in it being marked
-                * extern and being dup()ed. Let's mark the receiver as
-                * inherited so that it properly bypasses all second-stage
-                * setup and avoids being passed to new processes.
-                */
-               rx->flags |= RX_F_INHERITED;
                rx->fd = rx->shard_info->ref->fd;
        }
 
@@ -418,7 +412,7 @@ int sock_unix_bind_receiver(struct receiver *rx, char **errmsg)
        fd_insert(fd, rx->owner, rx->iocb, rx->bind_tgroup, rx->bind_thread);
 
        /* for now, all regularly bound TCP listeners are exportable */
-       if (!(rx->flags & RX_F_INHERITED))
+       if (!(rx->flags & (RX_F_INHERITED_FD|RX_F_INHERITED_SOCK)))
                HA_ATOMIC_OR(&fdtab[fd].state, FD_EXPORTED);
 
        return err;