BUG/MEDIUM: fd: Fix a deadlock when closing other tgroups fds

Commit 061754b249b9903913d6766c1ab31bb393ee5c0d attempted to make it
possible to close file descriptors belonging to other thread groups by
using thread isolation.
The problem is, closing other thread groups' fds usually happens when we
destroy a listener, in which case we hold the listener lock. If any
other thread tries to get that lock while we're waiting for the thread
isolation, then they will deadlock.
This can happen with any type of listener, but it is easier to reproduce
with a suspend/resume loop with ABNS sockets, as a suspend translates to
a close here.
To fix that, instead of using thread isolation, do something similar to
what's done when the fd belongs to our thread group. Increase the tgid
ref counter, so that we're sure nobody will close the fd while we're
dealing with it, then set the FD_MUST_CLOSE bit and set thread_mask to
0.
At this point, if no thread was running on that fd, no one will and we
can safely close it. So just call _fd_delete_orphan() if the
running_mask is 0 and if the FD_MUST_CLOSE bit is still there, otherwise
we can safely assume another thread will take care of it.

This should be backported up to 2.8.

diff --git a/src/fd.c b/src/fd.c
index 6e7276f38e1a0348e219c07d6f3328b01b7f1fb1..7df4ad0aeb8dab64dc110659f7dee877208b6c09 100644 (file)
--- a/src/fd.c
+++ b/src/fd.c
@@ -363,10 +363,10 @@ void _fd_delete_orphan(int fd)
 
 /* Deletes an FD from the fdsets. The file descriptor is also closed, possibly
  * asynchronously. It is safe to call it from another thread from the same
- * group as the FD's or from a thread from a different group. However if called
- * from a thread from another group, there is an extra cost involved because
- * the operation is performed under thread isolation, so doing so must be
- * reserved for ultra-rare cases (e.g. stopping a listener).
+ * group as the FD's or from a thread from a different group.
+ * Calling from a thread from another group is handled by grabbing the
+ * reference count, adding the FD_MUST_CLOSE and doing the close ourselves
+ * only if there is no other thread currently using the fd.
  */
 void fd_delete(int fd)
 {
@@ -387,23 +387,33 @@ void fd_delete(int fd)
         * face the situation where we try to close an fd that was reassigned.
         * However there is one corner case where this happens, it's when an
         * attempt to pause a listener fails (e.g. abns), leaving the listener
-        * in fault state and it is forcefully stopped. This needs to be done
-        * under isolation, and it's quite rare (i.e. once per such FD per
-        * process). Since we'll be isolated we can clear the thread mask and
-        * close the FD ourselves.
+        * in fault state and it is forcefully stopped.
+        * To make it work when the fd does not belong to our thread group,
+        * first increase the tgid refcount, so that we're sure nobody will
+        * close it before we figure out if we should, then set the
+        * FD_MUST_CLOSE bit, so that any running thread will take care
+        * of closing it.
+        * Once we set the thread_mask to 0, we know for sure that if
+        * nobody is running, nobody ever will, so at this point, if
+        * there is no running thread, and the FD_MUST_CLOSE bit is still
+        * set, we know it is safe to actually close the fd.
         */
        if (unlikely(fd_tgid(fd) != ti->tgid)) {
-               int must_isolate = !thread_isolated() && !(global.mode & MODE_STOPPING);
+               uint tgrp;
 
-               if (must_isolate)
-                       thread_isolate();
+               tgrp = fd_take_tgid(fd);
 
+               HA_ATOMIC_OR(&fdtab[fd].state, FD_MUST_CLOSE);
                HA_ATOMIC_STORE(&fdtab[fd].thread_mask, 0);
-               HA_ATOMIC_STORE(&fdtab[fd].running_mask, 0);
-               _fd_delete_orphan(fd);
 
-               if (must_isolate)
-                       thread_release();
+               if (!HA_ATOMIC_LOAD(&fdtab[fd].running_mask) &&
+                   HA_ATOMIC_BTR(&fdtab[fd].state, FD_MUST_CLOSE_BIT)) {
+                       if (tgrp)
+                               fd_drop_tgid(fd);
+                       _fd_delete_orphan(fd);
+               }
+               else if (tgrp)
+                       fd_drop_tgid(fd);
                return;
        }