From 32a251160f2db690886bb9474d9af29381c275a3 Mon Sep 17 00:00:00 2001 From: g2flyer Date: Mon, 6 May 2024 15:23:34 -0700 Subject: [PATCH 1/2] [LibOS] Protect handle->dentry with handle->lock Signed-off-by: g2flyer --- libos/include/libos_handle.h | 1 + libos/src/bookkeep/libos_handle.c | 67 ++++++++++++++------------ libos/src/fs/libos_fs_pseudo.c | 3 ++ libos/src/fs/libos_namei.c | 8 +-- libos/src/fs/proc/thread.c | 8 +++ libos/src/ipc/libos_ipc_process_info.c | 5 +- libos/src/libos_rtld.c | 2 + libos/src/sys/libos_fcntl.c | 35 +++++++++----- libos/src/sys/libos_getcwd.c | 9 ++-- libos/src/sys/libos_ioctl.c | 4 +- libos/src/sys/libos_open.c | 10 ++-- libos/src/sys/libos_pipe.c | 4 +- libos/src/sys/libos_stat.c | 14 +++--- libos/src/utils/log.c | 2 + 14 files changed, 109 insertions(+), 63 deletions(-) diff --git a/libos/include/libos_handle.h b/libos/include/libos_handle.h index d0920cff06..cdabc3cf41 100644 --- a/libos/include/libos_handle.h +++ b/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; /* diff --git a/libos/src/bookkeep/libos_handle.c b/libos/src/bookkeep/libos_handle.c index bbb39acf8e..bfd3995729 100644 --- a/libos/src/bookkeep/libos_handle.c +++ b/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); } diff --git a/libos/src/fs/libos_fs_pseudo.c b/libos/src/fs/libos_fs_pseudo.c index eab8129e0f..7a0c19f807 100644 --- a/libos/src/fs/libos_fs_pseudo.c +++ b/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); } diff --git a/libos/src/fs/libos_namei.c b/libos/src/fs/libos_namei.c index f23f2de8da..315735d10c 100644 --- a/libos/src/fs/libos_namei.c +++ b/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; } diff --git a/libos/src/fs/proc/thread.c b/libos/src/fs/proc/thread.c index c3da147c48..b22828fbb9 100644 --- a/libos/src/fs/proc/thread.c +++ b/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(); diff --git a/libos/src/ipc/libos_ipc_process_info.c b/libos/src/ipc/libos_ipc_process_info.c index 77767903f9..c3dadb48fc 100644 --- a/libos/src/ipc/libos_ipc_process_info.c +++ b/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; diff --git a/libos/src/libos_rtld.c b/libos/src/libos_rtld.c index baa08f9e30..5a86dd7899 100644 --- a/libos/src/libos_rtld.c +++ b/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; diff --git a/libos/src/sys/libos_fcntl.c b/libos/src/sys/libos_fcntl.c index 24578155e2..0fa6b7b297 100644 --- a/libos/src/sys/libos_fcntl.c +++ b/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; diff --git a/libos/src/sys/libos_getcwd.c b/libos/src/sys/libos_getcwd.c index 1368bfc85a..b469ccf2d9 100644 --- a/libos/src/sys/libos_getcwd.c +++ b/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; } diff --git a/libos/src/sys/libos_ioctl.c b/libos/src/sys/libos_ioctl.c index 89d5424da9..26c7e08510 100644 --- a/libos/src/sys/libos_ioctl.c +++ b/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) { diff --git a/libos/src/sys/libos_open.c b/libos/src/sys/libos_open.c index ce3afad908..069b19b2f4 100644 --- a/libos/src/sys/libos_open.c +++ b/libos/src/sys/libos_open.c @@ -373,15 +373,14 @@ 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); + lock(&g_dcache_lock); /* for dentry->inode */ + maybe_lock_pos_handle(hdl); + lock(&hdl->lock); /* for hdl->dentry */ if (!hdl->dentry->inode) { ret = -ENOENT; - goto out_unlock_only_dcache_lock; + goto out; } - maybe_lock_pos_handle(hdl); - lock(&hdl->lock); - struct libos_dir_handle* dirhdl = &hdl->dir_info; if ((ret = populate_directory_handle(hdl)) < 0) goto out; @@ -467,7 +466,6 @@ 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); diff --git a/libos/src/sys/libos_pipe.c b/libos/src/sys/libos_pipe.c index 53af9d5e92..bb677b6f1b 100644 --- a/libos/src/sys/libos_pipe.c +++ b/libos/src/sys/libos_pipe.c @@ -246,14 +246,14 @@ long libos_syscall_mknodat(int dirfd, const char* pathname, mode_t mode, dev_t d hdl1->flags = O_RDONLY; hdl1->acc_mode = MAY_READ; get_dentry(dent); - hdl1->dentry = dent; + hdl1->dentry = dent; /* new not-yet-exported handle, so skip unnecessary handle locking */ hdl2->type = TYPE_PIPE; hdl2->fs = &fifo_builtin_fs; hdl2->flags = O_WRONLY; hdl2->acc_mode = MAY_WRITE; get_dentry(dent); - hdl2->dentry = dent; + hdl2->dentry = dent; /* new not-yet-exported handle, so skip unnecessary handle locking */ /* FIFO must be open'ed to start read/write operations, mark as not ready */ hdl1->info.pipe.ready_for_ops = false; diff --git a/libos/src/sys/libos_stat.c b/libos/src/sys/libos_stat.c index 5e54c0ad5c..13f8200628 100644 --- a/libos/src/sys/libos_stat.c +++ b/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); } diff --git a/libos/src/utils/log.c b/libos/src/utils/log.c index 457ea8897d..777aedaa30 100644 --- a/libos/src/utils/log.c +++ b/libos/src/utils/log.c @@ -34,12 +34,14 @@ void log_setprefix(libos_tcb_t* tcb) { const char* exec_name; if (g_process.exec) { + lock(&g_process.exec->lock); if (g_process.exec->dentry) { exec_name = g_process.exec->dentry->name; } else { /* Unknown executable name */ exec_name = "?"; } + unlock(&g_process.exec->lock); } else { /* `g_process.exec` not available yet, happens on process init */ exec_name = ""; From b4b13dd85c4d7b091bbe934d9a57e802a8e9eda2 Mon Sep 17 00:00:00 2001 From: g2flyer Date: Tue, 2 Jul 2024 22:41:55 -0700 Subject: [PATCH 2/2] [LibOS] Fix dentry of open files after rename Signed-off-by: g2flyer --- libos/src/sys/libos_file.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/libos/src/sys/libos_file.c b/libos/src/sys/libos_file.c index f4050813d3..70c6a8f0a6 100644 --- a/libos/src/sys/libos_file.c +++ b/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; }