[LibOS] Add mask to setflags callback in struct shim_fs_ops

This allows to change only a subset of flags, e.g. just `O_NONBLOCK`. Additionally this commit enables setting `O_NONBLOCK` on UNIX sockets that were not yet bound or connected. Signed-off-by: Borys Popławski <[email protected]>
gramineproject · Jun 23, 2022 · 2cb071b · 2cb071b
1 parent 8b112b4
commit 2cb071b
Show file tree

Hide file tree

Showing 7 changed files with 98 additions and 61 deletions.
diff --git a/libos/include/shim_fs.h b/libos/include/shim_fs.h
@@ -157,8 +157,15 @@ struct shim_fs_ops {
     /* hstat: get status of the file; `st_ino` will be taken from dentry, if there's one */
     int (*hstat)(struct shim_handle* hdl, struct stat* buf);
 
-    /* setflags: set flags of the file */
-    int (*setflags)(struct shim_handle* hdl, int flags);
+    /*!
+     * \brief Set flags on the handle.
+     *
+     * \param handle  The handle to set flags on.
+     * \param flags   Flags to set.
+     * \param mask    Indicates which flags to change. Only bits set in \p mask will be changed
+     *                (to values taken from \p flags). Must be non zero.
+     */
+    int (*setflags)(struct shim_handle* handle, unsigned int flags, unsigned int mask);
 
     /* lock and unlock the file */
     int (*lock)(const char* trim_name);

diff --git a/libos/include/shim_handle.h b/libos/include/shim_handle.h
@@ -81,13 +81,15 @@ enum shim_sock_state {
  * `ops`, `domain`, `type` and `protocol` are read-only and do not need any locking.
  * Access to `peek` struct is protected by `recv_lock`. This lock also ensures proper ordering of
  * stream reads (see the comment in `do_recvmsg` in "libos/src/sys/shim_socket.c").
- * `pal_handle` should be accessed using atomic operations. It can be NULL. Once it's set, it cannot
- * change anymore.
+ * `pal_handle` should be accessed using atomic operations.
  * If you need to take both `recv_lock` and `lock`, take the former first.
  */
 struct shim_sock_handle {
     struct shim_lock lock;
     struct shim_sock_ops* ops;
+    /* `pal_handle` can be NULL. Once it's set, it cannot change anymore. All implementations must
+     * take into account all necessary settings when instantiating this field, e.g. `handle->flags`,
+     * of handle wrapping this struct. */
     PAL_HANDLE pal_handle;
     int domain;
     int type;

diff --git a/libos/src/fs/pipe/fs.c b/libos/src/fs/pipe/fs.c
@@ -158,35 +158,43 @@ static int pipe_hstat(struct shim_handle* hdl, struct stat* stat) {
     return 0;
 }
 
-static int pipe_setflags(struct shim_handle* hdl, int flags) {
-    if (!hdl->pal_handle)
-        return 0;
+static int pipe_setflags(struct shim_handle* handle, unsigned int flags, unsigned int mask) {
+    assert(mask != 0);
+    assert((flags & ~mask) == 0);
 
-    PAL_STREAM_ATTR attr;
+    /* TODO: what is this check? Why it has no locking? */
+    if (!handle->pal_handle)
+        return 0;
 
-    int ret = DkStreamAttributesQueryByHandle(hdl->pal_handle, &attr);
-    if (ret < 0) {
-        return pal_to_unix_errno(ret);
+    if (!WITHIN_MASK(flags, O_NONBLOCK)) {
+        return -EINVAL;
     }
 
-    if (attr.nonblocking) {
-        if (flags & O_NONBLOCK)
-            return 0;
+    lock(&handle->lock);
 
-        attr.nonblocking = false;
-    } else {
-        if (!(flags & O_NONBLOCK))
-            return 0;
-
-        attr.nonblocking = true;
+    PAL_STREAM_ATTR attr;
+    int ret = DkStreamAttributesQueryByHandle(handle->pal_handle, &attr);
+    if (ret < 0) {
+        ret = pal_to_unix_errno(ret);
+        goto out;
     }
 
-    ret = DkStreamAttributesSetByHandle(hdl->pal_handle, &attr);
-    if (ret < 0) {
-        return pal_to_unix_errno(ret);
+    bool nonblocking = flags & O_NONBLOCK;
+    if (attr.nonblocking != nonblocking) {
+        attr.nonblocking = nonblocking;
+        ret = DkStreamAttributesSetByHandle(handle->pal_handle, &attr);
+        if (ret < 0) {
+            ret = pal_to_unix_errno(ret);
+            goto out;
+        }
     }
 
-    return 0;
+    handle->flags = (handle->flags & ~mask) | flags;
+    ret = 0;
+
+out:
+    unlock(&handle->lock);
+    return ret;
 }
 
 static int fifo_open(struct shim_handle* hdl, struct shim_dentry* dent, int flags) {
@@ -199,13 +207,13 @@ static int fifo_open(struct shim_handle* hdl, struct shim_dentry* dent, int flag
      *        unless the other end has already been opened". We cannot enforce this failure since
      *        Gramine doesn't know whether the other process already opened this FIFO. */
 
-    if (flags & O_RDWR) {
+    if ((flags & O_ACCMODE) == O_RDWR) {
         /* POSIX disallows FIFOs opened for read-write, but Linux allows it. We must choose only
          * one end (read or write) in our emulation, so we treat such FIFOs as read-only. This
          * covers most apps seen in the wild (in particular, LTP apps). */
         log_warning("FIFO (named pipe) '%s' cannot be opened in read-write mode in Gramine. "
                     "Treating it as read-only.", dent->mount->path);
-        flags = O_RDONLY;
+        flags = (flags & ~O_ACCMODE) | O_RDONLY;
     }
 
     int fd;
@@ -234,7 +242,7 @@ static int fifo_open(struct shim_handle* hdl, struct shim_dentry* dent, int flag
 
     if (flags & O_NONBLOCK) {
         /* FIFOs were created in blocking mode (see shim_do_mknodat), change their attributes */
-        int ret = pipe_setflags(fifo_hdl, flags);
+        int ret = pipe_setflags(fifo_hdl, flags & O_NONBLOCK, O_NONBLOCK);
         if (ret < 0) {
             put_handle(fifo_hdl);
             return ret;

diff --git a/libos/src/fs/socket/fs.c b/libos/src/fs/socket/fs.c
@@ -78,34 +78,51 @@ static int hstat(struct shim_handle* handle, struct stat* stat) {
     return 0;
 }
 
-static int setflags(struct shim_handle* handle, int flags) {
+static int setflags(struct shim_handle* handle, unsigned int flags, unsigned int mask) {
+    assert(mask != 0);
+    assert((flags & ~mask) == 0);
+
     if (!WITHIN_MASK(flags, O_NONBLOCK)) {
         return -EINVAL;
     }
 
+    int ret;
     bool nonblocking = flags & O_NONBLOCK;
+    struct shim_sock_handle* sock = &handle->info.sock;
+
+    lock(&sock->lock);
+    lock(&handle->lock);
 
-    PAL_HANDLE pal_handle = __atomic_load_n(&handle->info.sock.pal_handle, __ATOMIC_ACQUIRE);
+    PAL_HANDLE pal_handle = __atomic_load_n(&sock->pal_handle, __ATOMIC_ACQUIRE);
     if (!pal_handle) {
-        log_warning("Trying to set flags on not bound / not connected UNIX socket. This is not "
-                    "supported in Gramine.");
-        return -EINVAL;
+        /* Just save the flags for later. */
+        goto out_set_flags;
     }
 
     PAL_STREAM_ATTR attr;
-    int ret = DkStreamAttributesQueryByHandle(pal_handle, &attr);
+    ret = DkStreamAttributesQueryByHandle(pal_handle, &attr);
     if (ret < 0) {
-        return pal_to_unix_errno(ret);
+        ret = pal_to_unix_errno(ret);
+        goto out;
     }
 
-    assert(ret == 0);
-
     if (attr.nonblocking != nonblocking) {
         attr.nonblocking = nonblocking;
         ret = DkStreamAttributesSetByHandle(pal_handle, &attr);
+        if (ret < 0) {
+            ret =  pal_to_unix_errno(ret);
+            goto out;
+        }
     }
 
-    return pal_to_unix_errno(ret);
+out_set_flags:
+    handle->flags = (handle->flags & ~mask) | flags;
+    ret = 0;
+
+out:
+    unlock(&handle->lock);
+    unlock(&sock->lock);
+    return ret;
 }
 
 static int checkout(struct shim_handle* handle) {

diff --git a/libos/src/net/unix.c b/libos/src/net/unix.c
@@ -132,6 +132,8 @@ static int bind(struct shim_handle* handle, void* addr, size_t addrlen) {
     }
 
     lock(&handle->lock);
+    /* `setflags` in "fs/socket/fs.c" is the only way to change this flag and it takes `sock->lock`,
+     * so using `options` after releasing the lock below is race-free. */
     pal_stream_options_t options = handle->flags & O_NONBLOCK ? PAL_OPTION_NONBLOCK : 0;
     unlock(&handle->lock);
 
@@ -239,9 +241,8 @@ static int connect(struct shim_handle* handle, void* addr, size_t addrlen) {
     }
 
     lock(&handle->lock);
-    /* TODO: this is racy, because we create the socket using `options` after releasing the lock.
-     * For now `setflags` in "fs/socket/fs.c" explicitly fails, so this will always be `0`, but we
-     * need to fix this at some point. */
+    /* `setflags` in "fs/socket/fs.c" is the only way to change this flag and it takes `sock->lock`,
+     * so using `options` after releasing the lock below is race-free. */
     pal_stream_options_t options = handle->flags & O_NONBLOCK ? PAL_OPTION_NONBLOCK : 0;
     unlock(&handle->lock);
 

diff --git a/libos/src/sys/shim_fcntl.c b/libos/src/sys/shim_fcntl.c
@@ -26,24 +26,29 @@
 #include "shim_table.h"
 #include "shim_thread.h"
 
-#define FCNTL_SETFL_MASK (O_APPEND | O_NONBLOCK)
+#define FCNTL_SETFL_MASK (O_APPEND | O_DIRECT | O_NOATIME | O_NONBLOCK)
+
+static int generic_set_flags(struct shim_handle* handle, unsigned int flags, unsigned int mask) {
+    /* TODO: DOES THIS WORK LOL
+     * The old version of this code did this, but this seem to be incorrect. If a handle type allows
+     * for setting some flags without actually doing anything with them immediately, it should have
+     * a `setflags` callback implementation. */
+    lock(&handle->lock);
+    handle->flags = (handle->flags & ~mask) | flags;
+    unlock(&handle->lock);
+    return 0;
+}
 
-static int _set_handle_flags(struct shim_handle* hdl, unsigned long arg) {
-    if (hdl->fs && hdl->fs->fs_ops && hdl->fs->fs_ops->setflags) {
-        int ret = hdl->fs->fs_ops->setflags(hdl, arg & FCNTL_SETFL_MASK);
-        if (ret < 0) {
-            return ret;
-        }
+static int set_handle_flags(struct shim_handle* handle, unsigned int flags, unsigned int mask) {
+    flags &= mask;
+    if (handle->fs && handle->fs->fs_ops && handle->fs->fs_ops->setflags) {
+        return handle->fs->fs_ops->setflags(handle, flags, mask);
     }
-    hdl->flags = (hdl->flags & ~FCNTL_SETFL_MASK) | (arg & FCNTL_SETFL_MASK);
-    return 0;
+    return generic_set_flags(handle, flags, mask);
 }
 
-int set_handle_nonblocking(struct shim_handle* hdl, bool on) {
-    lock(&hdl->lock);
-    int ret = _set_handle_flags(hdl, on ? hdl->flags | O_NONBLOCK : hdl->flags & ~O_NONBLOCK);
-    unlock(&hdl->lock);
-    return ret;
+int set_handle_nonblocking(struct shim_handle* handle, bool on) {
+    return set_handle_flags(handle, on ? O_NONBLOCK : 0, O_NONBLOCK);
 }
 
 /*
@@ -177,9 +182,7 @@ long shim_do_fcntl(int fd, int cmd, unsigned long arg) {
 
         /* F_SETFL (int) */
         case F_SETFL:
-            lock(&hdl->lock);
-            ret = _set_handle_flags(hdl, arg);
-            unlock(&hdl->lock);
+            ret = set_handle_flags(hdl, arg, FCNTL_SETFL_MASK);
             break;
 
         /* F_SETLK, F_SETLKW (struct flock*): see `shim_fs_lock.h` for caveats */

diff --git a/libos/src/sys/shim_pipe.c b/libos/src/sys/shim_pipe.c
@@ -52,8 +52,10 @@ static int create_pipes(struct shim_handle* srv, struct shim_handle* cli, int fl
         goto out;
     }
 
-    PAL_HANDLE tmp = srv->pal_handle;
+    assert(!srv->pal_handle);
+    assert(!cli->pal_handle);
     srv->pal_handle = hdl1;
+    cli->pal_handle = hdl2;
 
     assert(!srv->uri);
     assert(!cli->uri);
@@ -71,13 +73,10 @@ static int create_pipes(struct shim_handle* srv, struct shim_handle* cli, int fl
 
         ret = set_handle_nonblocking(srv, /*on=*/true);
         if (ret < 0) {
-            /* Restore original handle, if any. */
-            srv->pal_handle = tmp;
             goto out;
         }
     }
 
-    cli->pal_handle = hdl2;
     ret = 0;
 
 out:;