[LibOS] Fix dentry of open file handles related to a rename

libos/include/libos_handle.h

@@ -165,6 +165,9 @@ struct libos_handle {
     refcount_t ref_count;
 
     struct libos_fs* fs;
+    /* dentry can change due to rename, so to secure read-access, acquire g_dcache_lock and/or
+     * handle->lock; to protect updates (unless during creation and deletion where unique use is
+     * guaranteed), acquire first g_dcache_lock and then handle->lock */
     struct libos_dentry* dentry;
 
     /*

libos/include/libos_process.h

@@ -62,7 +62,7 @@ struct libos_process {
     LISTP_TYPE(libos_child_process) zombies;
 
     struct libos_lock children_lock;
-    struct libos_lock fs_lock;
+    struct libos_lock fs_lock; /* Note: has lower priority than g_dcache_lock. */
 
     /* Complete command line for the process, as reported by /proc/[pid]/cmdline; currently filled
      * once during initialization and not able to be modified.

libos/src/bookkeep/libos_handle.c

@@ -312,7 +312,9 @@ static int clear_posix_locks(struct libos_handle* handle) {
             .end = FS_LOCK_EOF,
             .pid = g_process.pid,
         };
+        lock(&handle->lock);
         int ret = file_lock_set(handle->dentry, &file_lock, /*block=*/false);
+        unlock(&handle->lock);
         if (ret < 0) {
             log_warning("error releasing locks: %s", unix_strerror(ret));
             return ret;
@@ -477,13 +479,17 @@ int set_new_fd_handle_above_fd(uint32_t fd, struct libos_handle* hdl, int fd_fla
 }
 
 static inline __attribute__((unused)) const char* __handle_name(struct libos_handle* hdl) {
+    /* This function seems unused, so probably could be dropped? */
+    const char* ret = "(unknown)";
+    lock(&hdl->lock);
     if (hdl->uri)
-        return hdl->uri;
+        ret = hdl->uri;
     if (hdl->dentry && hdl->dentry->name[0] != '\0')
-        return hdl->dentry->name;
+        ret = hdl->dentry->name;
     if (hdl->fs)
-        return hdl->fs->name;
-    return "(unknown)";
+        ret = hdl->fs->name;
+    unlock(&hdl->lock);
+    return ret;
 }
 
 void get_handle(struct libos_handle* hdl) {
@@ -499,6 +505,8 @@ static void destroy_handle(struct libos_handle* hdl) {
 
 static int clear_flock_locks(struct libos_handle* hdl) {
     /* Clear flock (BSD) locks for a file. We are required to do that when the handle is closed. */
+    int ret = 0;
+    lock(&hdl->lock);
     if (hdl && hdl->dentry && hdl->created_by_process) {
         assert(hdl->ref_count == 0);
         struct libos_file_lock file_lock = {
@@ -509,10 +517,10 @@ static int clear_flock_locks(struct libos_handle* hdl) {
         int ret = file_lock_set(hdl->dentry, &file_lock, /*block=*/false);
         if (ret < 0) {
             log_warning("error releasing locks: %s", unix_strerror(ret));
-            return ret;
         }
     }
-    return 0;
+    unlock(&hdl->lock);
+    return ret;
 }
 
 void put_handle(struct libos_handle* hdl) {

libos/src/fs/libos_fs_pseudo.c

@@ -266,7 +266,16 @@ static int pseudo_stat(struct libos_dentry* dent, struct stat* buf) {
 }
 
 static int pseudo_hstat(struct libos_handle* handle, struct stat* buf) {
-    return pseudo_istat(handle->dentry, handle->inode, buf);
+    /* some callers, e.g., do_hstat in libos_stat.c, already hold lock but others do not */
+    bool hold_lock = locked(&handle->lock);
+    if (!hold_lock)
+        lock(&handle->lock);
+    int ret = -ENOENT;
+    if (handle->dentry)
+        ret = pseudo_istat(handle->dentry, handle->inode, buf);
+    if (!hold_lock)
+        unlock(&handle->lock);
+    return ret;
 }
 
 static int pseudo_unlink(struct libos_dentry* dent) {

libos/src/fs/libos_namei.c

@@ -741,8 +741,10 @@ int get_dirfd_dentry(int dirfd, struct libos_dentry** dir) {
         return -ENOTDIR;
     }
 
+    lock(&hdl->lock); /* while hdl->is_dir is immutable, hdl->dentry can change due to rename */
     get_dentry(hdl->dentry);
     *dir = hdl->dentry;
+    unlock(&hdl->lock);
     put_handle(hdl);
     return 0;
 }

libos/src/fs/proc/thread.c

@@ -29,9 +29,11 @@ int proc_thread_follow_link(struct libos_dentry* dent, char** out_target) {
         dent = g_process.cwd;
         get_dentry(dent);
     } else if (strcmp(name, "exe") == 0) {
+        lock(&g_process.exec->lock);
         dent = g_process.exec->dentry;
         if (dent)
             get_dentry(dent);
+        unlock(&g_process.exec->lock);
     }
 
     unlock(&g_process.fs_lock);
@@ -91,11 +93,13 @@ int proc_thread_maps_load(struct libos_dentry* dent, char** out_data, size_t* ou
     retry_emit_vma:
         if (vma->file) {
             int dev_major = 0, dev_minor = 0;
+            lock(&vma->file->lock);
             unsigned long ino = vma->file->dentry ? dentry_ino(vma->file->dentry) : 0;
             char* path = NULL;
 
             if (vma->file->dentry)
                 dentry_abs_path(vma->file->dentry, &path, /*size=*/NULL);
+            unlock(&vma->file->lock);
 
             EMIT(ADDR_FMT(start), start);
             EMIT("-");
@@ -310,6 +314,7 @@ int proc_thread_fd_follow_link(struct libos_dentry* dent, char** out_target) {
     int ret;
     struct libos_handle* hdl = handle_map->map[fd]->handle;
 
+    lock(&hdl->lock);
     if (hdl->dentry) {
         ret = dentry_abs_path(hdl->dentry, out_target, /*size=*/NULL);
     } else {
@@ -318,6 +323,7 @@ int proc_thread_fd_follow_link(struct libos_dentry* dent, char** out_target) {
         *out_target = describe_handle(hdl);
         ret = *out_target ? 0 : -ENOMEM;
     }
+    unlock(&hdl->lock);
 
     rwlock_read_unlock(&handle_map->lock);
 
@@ -457,9 +463,11 @@ int proc_thread_stat_load(struct libos_dentry* dent, char** out_data, size_t* ou
 
     char comm[16] = {0};
     lock(&g_process.fs_lock);
+    lock(&g_process.exec->lock);
     size_t name_length = g_process.exec->dentry->name_len;
     memcpy(comm, g_process.exec->dentry->name,
            name_length > sizeof(comm) - 1 ? sizeof(comm) - 1 : name_length);
+    unlock(&g_process.exec->lock);
     unlock(&g_process.fs_lock);
     size_t virtual_mem_size = get_total_memory_usage();
 

libos/src/libos_rtld.c

@@ -589,10 +589,12 @@ static int load_and_check_shebang(struct libos_handle* file, const char* pathnam
     ret = read_partial_fragment(file, shebang, sizeof(shebang), /*offset=*/0, &shebang_len);
     if (ret < 0 || shebang_len < 2 || shebang[0] != '#' || shebang[1] != '!') {
         char* path = NULL;
+        lock(&file->lock);
         if (file->dentry) {
             /* this may fail, but we are already inside a more serious error handler */
             dentry_abs_path(file->dentry, &path, /*size=*/NULL);
         }
+        unlock(&file->lock);
         log_debug("Failed to read shebang line from %s", path ? path : "(unknown)");
         free(path);
         return -ENOEXEC;

libos/src/sys/libos_fcntl.c

@@ -199,29 +199,34 @@ long libos_syscall_fcntl(int fd, int cmd, unsigned long arg) {
                 break;
             }
 
-            if (!hdl->dentry) {
+            lock(&hdl->lock);
+            struct libos_dentry* dent = hdl->dentry;
+
+            if (!dent) {
                 /* TODO: Linux allows locks on pipes etc. Our locks work only for "normal" files
                  * that have a dentry. */
                 ret = -EINVAL;
-                break;
+                goto out_setlkw_unlock;
             }
 
             if (fl->l_type == F_RDLCK && !(hdl->acc_mode & MAY_READ)) {
                 ret = -EINVAL;
-                break;
+                goto out_setlkw_unlock;
             }
 
             if (fl->l_type == F_WRLCK && !(hdl->acc_mode & MAY_WRITE)) {
                 ret = -EINVAL;
-                break;
+                goto out_setlkw_unlock;
             }
 
             struct libos_file_lock file_lock;
             ret = flock_to_file_lock(fl, hdl, &file_lock);
             if (ret < 0)
-                break;
+                goto out_setlkw_unlock;
 
-            ret = file_lock_set(hdl->dentry, &file_lock, /*wait=*/cmd == F_SETLKW);
+            ret = file_lock_set(dent, &file_lock, /*wait=*/cmd == F_SETLKW);
+        out_setlkw_unlock:
+            unlock(&hdl->lock);
             break;
         }
 
@@ -234,9 +239,12 @@ long libos_syscall_fcntl(int fd, int cmd, unsigned long arg) {
                 break;
             }
 
-            if (!hdl->dentry) {
+            lock(&hdl->lock);
+            struct libos_dentry* dent = hdl->dentry;
+
+            if (!dent) {
                 ret = -EINVAL;
-                break;
+                goto out_getlkw_unlock;
             }
 
             struct libos_file_lock file_lock;
@@ -246,13 +254,13 @@ long libos_syscall_fcntl(int fd, int cmd, unsigned long arg) {
 
             if (file_lock.type == F_UNLCK) {
                 ret = -EINVAL;
-                break;
+                goto out_getlkw_unlock;
             }
 
             struct libos_file_lock file_lock2;
-            ret = file_lock_get(hdl->dentry, &file_lock, &file_lock2);
+            ret = file_lock_get(dent, &file_lock, &file_lock2);
             if (ret < 0)
-                break;
+                goto out_getlkw_unlock;
 
             fl->l_type = file_lock2.type;
             if (file_lock2.type != F_UNLCK) {
@@ -267,6 +275,8 @@ long libos_syscall_fcntl(int fd, int cmd, unsigned long arg) {
                 fl->l_pid = file_lock2.pid;
             }
             ret = 0;
+        out_getlkw_unlock:
+            unlock(&hdl->lock);
             break;
         }
 
@@ -342,7 +352,10 @@ long libos_syscall_flock(unsigned int fd, unsigned int cmd) {
         .type = lock_type,
         .handle_id = hdl->id,
     };
+
+    lock(&hdl->lock);
     ret = file_lock_set(hdl->dentry, &file_lock, !(cmd & LOCK_NB));
+    unlock(&hdl->lock);
 out:
     put_handle(hdl);
     return ret;

libos/src/sys/libos_file.c

@@ -347,8 +347,31 @@ static int do_rename(struct libos_dentry* old_dent, struct libos_dentry* new_den
 
     if (new_dent->inode)
         put_inode(new_dent->inode);
+
     new_dent->inode = old_dent->inode;
     old_dent->inode = NULL;
+
+    /* also update dentry of any potentially open fd pointing to old_dent */
+    struct libos_handle_map* handle_map = get_thread_handle_map(NULL);
+    assert(handle_map != NULL);
+    rwlock_read_lock(&handle_map->lock);
+
+    for (uint32_t i = 0; handle_map->fd_top != FD_NULL && i <= handle_map->fd_top; i++) {
+        struct libos_fd_handle* fd_handle = handle_map->map[i];
+        if (!HANDLE_ALLOCATED(fd_handle))
+            continue;
+        struct libos_handle* handle = fd_handle->handle;
+        /* see comment in libos_handle.h on loocking strategy protecting handle->lock */
+        assert(locked(&g_dcache_lock));
+        lock(&handle->lock);
+        if ((handle->dentry == old_dent) && (handle->inode == new_dent->inode)) {
+            handle->dentry = new_dent;
+            put_dentry(old_dent);
+            get_dentry(new_dent);
+        }
+        unlock(&handle->lock);
+    }
+    rwlock_read_unlock(&handle_map->lock);
     return 0;
 }
 

libos/src/sys/libos_getcwd.c

@@ -82,19 +82,26 @@ long libos_syscall_fchdir(int fd) {
     if (!hdl)
         return -EBADF;
 
+    int ret = 0;
+    lock(&g_dcache_lock);
+    /* Note we have to protect hdl->dentry and hdl->dentry->inode, so hdl->lock is not enough and we
+     * have to lock g_dcache_lock (which also protectes hdl->dentry). Also note that
+     * g_process.fs_lock has lower priority than g_dcache_lock, see e.g., do_path_lookupat. */
     struct libos_dentry* dent = hdl->dentry;
 
     if (!dent) {
         log_debug("FD=%d has no path in the filesystem", fd);
-        return -ENOTDIR;
+        ret = -ENOTDIR;
+        goto out_unlock_dcache;
     }
     if (!dent->inode || dent->inode->type != S_IFDIR) {
         char* path = NULL;
         dentry_abs_path(dent, &path, /*size=*/NULL);
         log_debug("%s is not a directory", path);
         free(path);
         put_handle(hdl);
-        return -ENOTDIR;
+        ret = -ENOTDIR;
+        goto out_unlock_dcache;
     }
 
     lock(&g_process.fs_lock);
@@ -103,5 +110,7 @@ long libos_syscall_fchdir(int fd) {
     g_process.cwd = dent;
     unlock(&g_process.fs_lock);
     put_handle(hdl);
-    return 0;
+out_unlock_dcache:
+    unlock(&g_dcache_lock);
+    return ret;
 }

libos/src/sys/libos_open.c

@@ -373,12 +373,12 @@ static ssize_t do_getdents(int fd, uint8_t* buf, size_t buf_size, bool is_getden
         goto out_no_unlock;
     }
 
+    lock(&g_dcache_lock);
     if (!hdl->dentry->inode) {
         ret = -ENOENT;
-        goto out_no_unlock;
+        goto out_unlock_only_dcache_lock;
     }
 
-    lock(&g_dcache_lock);
     maybe_lock_pos_handle(hdl);
     lock(&hdl->lock);
 
@@ -467,6 +467,7 @@ static ssize_t do_getdents(int fd, uint8_t* buf, size_t buf_size, bool is_getden
 out:
     unlock(&hdl->lock);
     maybe_unlock_pos_handle(hdl);
+out_unlock_only_dcache_lock:
     unlock(&g_dcache_lock);
 out_no_unlock:
     put_handle(hdl);

libos/src/sys/libos_stat.c

@@ -39,15 +39,15 @@ static int do_hstat(struct libos_handle* hdl, struct stat* stat) {
     if (!fs || !fs->fs_ops || !fs->fs_ops->hstat)
         return -EACCES;
 
+    lock(&hdl->lock);
     int ret = fs->fs_ops->hstat(hdl, stat);
-    if (ret < 0)
-        return ret;
-
-    /* Update `st_ino` from dentry */
-    if (hdl->dentry)
+    if (ret >= 0 && hdl->dentry) {
+        /* Update `st_ino` from dentry */
         stat->st_ino = dentry_ino(hdl->dentry);
+    }
+    unlock(&hdl->lock);
 
-    return 0;
+    return ret;
 }
 
 long libos_syscall_stat(const char* file, struct stat* stat) {
@@ -210,7 +210,9 @@ long libos_syscall_fstatfs(int fd, struct statfs* buf) {
     if (!hdl)
         return -EBADF;
 
+    lock(&hdl->lock);
     struct libos_mount* mount = hdl->dentry ? hdl->dentry->mount : NULL;
+    unlock(&hdl->lock);
     put_handle(hdl);
     return __do_statfs(mount, buf);
 }