Fix epoll-before-listen bug (XCC)
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 1 Sep 2016 19:09:04 +0000 (15:09 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 6 Sep 2016 13:26:23 +0000 (09:26 -0400)
Previously, if you added a socket FD (a.k.a. a rock) to an epoll set before
you called listen(), then we'd tap the data FD instead of the listen FD.

Rebuild glibc and any apps using epoll/select (e.g. dropbear).

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/listen.c
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/plan9_sockets.c
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sys/plan9_helpers.h
user/iplib/epoll.c

index fa04109..40df2b9 100644 (file)
@@ -66,9 +66,6 @@ int __listen(int fd, int backlog)
                                return -1;
                        }
                        close(cfd);
-                       r->is_listener = TRUE;
-                       if (_rock_open_listen_fd(r) < 0)
-                               return -1;
                        return 0;
                case PF_UNIX:
                        if (r->other < 0) {
index 97781fe..5951663 100644 (file)
@@ -182,7 +182,7 @@ Rock *_sock_newrock(int fd)
        memset(&r->raddr, 0, sizeof(r->raddr_stor));
        r->ctl[0] = '\0';
        r->other = -1;
-       r->is_listener = FALSE;
+       r->has_listen_fd = FALSE;
        r->listen_fd = -1;
        return r;
 }
@@ -193,8 +193,11 @@ void _sock_fd_closed(int fd)
 
        if (!r)
                return;
-       if (r->is_listener)
+       if (r->has_listen_fd) {
                close(r->listen_fd);
+               /* This shouldn't matter - the rock is being closed anyways. */
+               r->has_listen_fd = FALSE;
+       }
 }
 
 /* For a ctlfd and a few other settings, it opens and returns the corresponding
@@ -297,15 +300,15 @@ int _sock_get_opts(int type)
 
 /* Opens the FD for "listen", and attaches it to the Rock.  When the dfd (and
  * thus the Rock) closes, we'll close the listen file too.  Returns the FD on
- * success, -1 on error. */
-int _rock_open_listen_fd(Rock *r)
+ * success, -1 on error.  This is racy, like a lot of other Rock stuff. */
+static int _rock_get_listen_fd(Rock *r)
 {
        char listen_file[Ctlsize + 3];
        char *x, *last_ctl;
        int ret;
 
-       if (!r->is_listener)
-               return -1;
+       if (r->has_listen_fd)
+               return r->listen_fd;
        strncpy(listen_file, r->ctl, sizeof(listen_file));
        /* We want the conversation directory.  We can find the last "ctl"
         * in the CTL name (they could have mounted at /ctlfoo/net/) */
@@ -324,20 +327,19 @@ int _rock_open_listen_fd(Rock *r)
         * our listen. */
        assert(ret >= 0);
        r->listen_fd = ret;
+       r->has_listen_fd = TRUE;
        return ret;
 }
 
-/* Used by user/iplib (e.g. epoll).  Looks up the FD listen file for this
- * conversation.  Returns -1 if the FD is not a listener. */
+/* Used by user/iplib (e.g. epoll).  Returns an FD for the listen file, opened
+ * O_PATH, for this conversation.  Returns -1 on error. */
 int _sock_lookup_listen_fd(int sock_fd)
 {
        Rock *r = _sock_findrock(sock_fd, 0);
 
        if (!r)
                return -1;
-       if (!r->is_listener)
-               return -1;
-       return r->listen_fd;
+       return _rock_get_listen_fd(r);
 }
 
 /* Given an FD, opens the FD with the name 'sibling' in the same directory.
index 71214af..aa60625 100644 (file)
@@ -59,7 +59,7 @@ struct Rock {
        };
        char ctl[Ctlsize];                      /* name of control file (if any) */
        int other;                                      /* fd of the remote end for Unix domain */
-       bool is_listener;                       /* has called listen() and will accept() */
+       bool has_listen_fd;                     /* has set up a listen file, O_PATH */
        int listen_fd;                          /* fd of the listen file, if any */
 };
 
@@ -74,7 +74,6 @@ extern void _sock_ingetaddr(Rock *, struct sockaddr_in *, socklen_t *,
                                                        const char *);
 extern int _sock_strip_opts(int type);
 extern int _sock_get_opts(int type);
-extern int _rock_open_listen_fd(Rock *r);
 extern int _sock_lookup_listen_fd(int sock_fd);
 
 int get_sibling_fd(int fd, const char *sibling);
index b370981..1a1d03d 100644 (file)
@@ -21,8 +21,8 @@
  *     with that.
  *     - epoll_pwait is probably racy.
  *     - You can't dup an epoll fd (same as other user FDs).
- *     - If you add a BSD socket FD to an epoll set before calling listen(), you'll
- *     only epoll on the data (which is inactive) instead of on the accept().
+ *     - If you add a BSD socket FD to an epoll set, you'll get taps on both the
+ *     data FD and the listen FD.
  *     - If you add the same BSD socket listener to multiple epoll sets, you will
  *     likely fail.  This is in addition to being able to tap only one FD at a
  *     time.
@@ -286,6 +286,7 @@ static int __epoll_ctl_add(struct epoll_ctlr *ep, int fd,
        struct ep_fd_data *ep_fd;
        struct fd_tap_req tap_req = {0};
        int ret, filter, sock_listen_fd;
+       struct epoll_event listen_event;
 
        /* Only support ET.  Also, we just ignore EPOLLONESHOT.  That might work,
         * logically, just with spurious events firing. */
@@ -295,15 +296,27 @@ static int __epoll_ctl_add(struct epoll_ctlr *ep, int fd,
                return -1;
        }
        /* The sockets-to-plan9 networking shims are a bit inconvenient.  The user
-        * asked us to epoll on an FD, but that FD is actually a Qdata FD.  We need
-        * to actually epoll on the listen_fd.
+        * asked us to epoll on an FD, but that FD is actually a Qdata FD.  We might
+        * need to actually epoll on the listen_fd.  Further, we don't know yet
+        * whether or not they want the listen FD.  They could epoll on the socket,
+        * then listen later and want to wake up on the listen.
+        *
+        * So in the case we have a socket FD, we'll actually open the listen FD
+        * regardless (glibc handles this), and we'll epoll on both FDs.
+        * Technically, either FD could fire and they'd get an epoll event for it,
+        * but I think socket users will use only listen or data.
         *
         * As far as tracking the FD goes for epoll_wait() reporting, if the app
         * wants to track the FD they think we are using, then they already passed
         * that in event->data. */
        sock_listen_fd = _sock_lookup_listen_fd(fd);
-       if (sock_listen_fd >= 0)
-               fd = sock_listen_fd;
+       if (sock_listen_fd >= 0) {
+               listen_event.events = EPOLLET | EPOLLIN | EPOLLHUP;
+               listen_event.data = event->data;
+               ret = __epoll_ctl_add(ep, sock_listen_fd, &listen_event);
+               if (ret < 0)
+                       return ret;
+       }
        ceq_ev = ep_get_ceq_ev(ep, fd);
        if (!ceq_ev) {
                errno = ENOMEM;
@@ -342,11 +355,17 @@ static int __epoll_ctl_del(struct epoll_ctlr *ep, int fd,
        struct fd_tap_req tap_req = {0};
        int ret, sock_listen_fd;
 
-       /* They could be asking to clear an epoll for a listener.  We need to remove
-        * the tap for the real FD we tapped */
+       /* If we were dealing with a socket shim FD, we tapped both the listen and
+        * the data file and need to untap both of them. */
        sock_listen_fd = _sock_lookup_listen_fd(fd);
-       if (sock_listen_fd >= 0)
-               fd = sock_listen_fd;
+       if (sock_listen_fd >= 0) {
+               /* It's possible to fail here.  Even though we tapped it already, if the
+                * deletion was triggered from close callbacks, it's possible for the
+                * sock_listen_fd to be closed first, which would have triggered an
+                * epoll_ctl_del.  When we get around to closing the Rock FD, the listen
+                * FD was already closed. */
+               __epoll_ctl_del(ep, sock_listen_fd, event);
+       }
        ceq_ev = ep_get_ceq_ev(ep, fd);
        if (!ceq_ev) {
                errno = ENOENT;