[LibOS] Fix dentry of open file handles related to a rename (revised version)

libos/include/libos_handle.h

@@ -165,6 +165,7 @@ struct libos_handle {
     refcount_t ref_count;
 
     struct libos_fs* fs;
+    /* dentry can change due to rename, so to derefence or update requires holding `lock`. */
     struct libos_dentry* dentry;
 
     /*

libos/src/bookkeep/libos_handle.c

@@ -324,25 +324,28 @@ static struct libos_handle* __detach_fd_handle(struct libos_fd_handle* fd, int*
 }
 
 static int clear_posix_locks(struct libos_handle* handle) {
-    if (handle && handle->dentry) {
-        /* Clear file (POSIX) locks for a file. We are required to do that every time a FD is
-         * closed, even if the process holds other handles for that file, or duplicated FDs for the
-         * same handle. */
-        struct libos_file_lock file_lock = {
-            .family = FILE_LOCK_POSIX,
-            .type = F_UNLCK,
-            .start = 0,
-            .end = FS_LOCK_EOF,
-            .pid = g_process.pid,
-        };
-        int ret = file_lock_set(handle->dentry, &file_lock, /*block=*/false);
-        if (ret < 0) {
-            log_warning("error releasing locks: %s", unix_strerror(ret));
-            return ret;
+    int ret = 0;
+    if (handle) {
+        lock(&handle->lock);
+        if (handle->dentry) {
+            /* Clear file (POSIX) locks for a file. We are required to do that every time a FD is
+             * closed, even if the process holds other handles for that file, or duplicated FDs for
+             * the same handle. */
+            struct libos_file_lock file_lock = {
+                .family = FILE_LOCK_POSIX,
+                .type   = F_UNLCK,
+                .start  = 0,
+                .end    = FS_LOCK_EOF,
+                .pid    = g_process.pid,
+            };
+            ret = file_lock_set(handle->dentry, &file_lock, /*block=*/false);
+            if (ret < 0) {
+                log_warning("error releasing locks: %s", unix_strerror(ret));
+            }
         }
+        unlock(&handle->lock);
     }
-
-    return 0;
+    return ret;
 }
 
 struct libos_handle* detach_fd_handle(uint32_t fd, int* flags,
@@ -512,20 +515,24 @@ 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. */
-    if (hdl && hdl->dentry && hdl->created_by_process) {
-        assert(hdl->ref_count == 0);
-        struct libos_file_lock file_lock = {
-            .family = FILE_LOCK_FLOCK,
-            .type = F_UNLCK,
-            .handle_id = hdl->id,
-        };
-        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;
+    int ret = 0;
+    if (hdl) {
+        lock(&hdl->lock);
+        if (hdl->dentry && hdl->created_by_process) {
+            assert(hdl->ref_count == 0);
+            struct libos_file_lock file_lock = {
+                .family    = FILE_LOCK_FLOCK,
+                .type      = F_UNLCK,
+                .handle_id = hdl->id,
+            };
+            int ret = file_lock_set(hdl->dentry, &file_lock, /*block=*/false);
+            if (ret < 0) {
+                log_warning("error releasing locks: %s", unix_strerror(ret));
+            }
         }
+        unlock(&hdl->lock);
     }
-    return 0;
+    return ret;
 }
 
 void put_handle(struct libos_handle* hdl) {
@@ -549,7 +556,7 @@ void put_handle(struct libos_handle* hdl) {
             hdl->pal_handle = NULL;
         }
 
-        if (hdl->dentry) {
+        if (hdl->dentry) { /* no locking needed as no other reference exists */
             (void)clear_flock_locks(hdl);
             put_dentry(hdl->dentry);
         }

libos/src/fs/libos_fs_pseudo.c

@@ -266,6 +266,9 @@ static int pseudo_stat(struct libos_dentry* dent, struct stat* buf) {
 }
 
 static int pseudo_hstat(struct libos_handle* handle, struct stat* buf) {
+    /* Note: derefence handle->dentry in general has to be protected by handle->lock as it could
+     * change due to rename.  However, as pseudo-fs does not support rename we can safely omit it
+     * here (or push it on the numerous callers of fs_op->hstat). */
     return pseudo_istat(handle->dentry, handle->inode, buf);
 }
 

libos/src/fs/libos_namei.c

@@ -367,7 +367,7 @@ static void assoc_handle_with_dentry(struct libos_handle* hdl, struct libos_dent
     assert(locked(&g_dcache_lock));
     assert(dent->inode);
 
-    hdl->dentry = dent;
+    hdl->dentry = dent; /* not-yet-shared handle, so no look needed. */
     get_dentry(dent);
 
     hdl->inode = dent->inode;
@@ -381,7 +381,7 @@ static void assoc_handle_with_dentry(struct libos_handle* hdl, struct libos_dent
 int dentry_open(struct libos_handle* hdl, struct libos_dentry* dent, int flags) {
     assert(locked(&g_dcache_lock));
     assert(dent->inode);
-    assert(!hdl->dentry);
+    assert(!hdl->dentry); /* not-yet-shared handle, so no look needed. */
 
     int ret;
     struct libos_fs* fs = dent->inode->fs;
@@ -431,7 +431,7 @@ int open_namei(struct libos_handle* hdl, struct libos_dentry* start, const char*
         assert(hdl);
 
     if (hdl)
-        assert(!hdl->dentry);
+        assert(!hdl->dentry); /* not-yet-shared handle, so no look needed. */
 
     lock(&g_dcache_lock);
 
@@ -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/ipc/libos_ipc_process_info.c

@@ -101,8 +101,11 @@ int ipc_pid_getmeta_callback(IDTYPE src, void* msg_data, uint64_t seq) {
             struct libos_dentry* dent = NULL;
             switch (msgin->code) {
                case PID_META_EXEC:
-                   if (g_process.exec)
+                   if (g_process.exec) {
+                       lock(&g_process.exec->lock);
                        dent = g_process.exec->dentry;
+                       unlock(&g_process.exec->lock);
+                   }
                    break;
                case PID_META_CWD:
                    dent = g_process.cwd;

libos/src/libos_rtld.c

@@ -597,10 +597,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

@@ -83,7 +83,9 @@ long libos_syscall_fchdir(int fd) {
         return -EBADF;
 
     int ret;
-    lock(&g_dcache_lock);
+    lock(&g_dcache_lock); /* for dent->inode */
+    lock(&g_process.fs_lock); /* for g_process.cwd */
+    lock(&hdl->lock); /* for hdl->dentry */
 
     struct libos_dentry* dent = hdl->dentry;
 
@@ -101,14 +103,15 @@ long libos_syscall_fchdir(int fd) {
         goto out;
     }
 
-    lock(&g_process.fs_lock);
     get_dentry(dent);
     put_dentry(g_process.cwd);
     g_process.cwd = dent;
-    unlock(&g_process.fs_lock);
     ret = 0;
+
 out:
     put_handle(hdl);
+    unlock(&hdl->lock);
+    unlock(&g_process.fs_lock);
     unlock(&g_dcache_lock);
     return ret;
 }

libos/src/sys/libos_ioctl.c

@@ -41,9 +41,11 @@ long libos_syscall_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg) {
     if (!hdl)
         return -EBADF;
 
-    lock(&g_dcache_lock);
+    lock(&g_dcache_lock); /* for dentry->inode */
+    lock(&hdl->lock);     /* for hdl->dentry */
     bool is_host_dev = hdl->type == TYPE_CHROOT && hdl->dentry->inode &&
         hdl->dentry->inode->type == S_IFCHR;
+    unlock(&hdl->lock);
     unlock(&g_dcache_lock);
 
     if (is_host_dev) {