From e36a532774bef0ebf4bc2e0b2b9cfc9ddf1eac7c Mon Sep 17 00:00:00 2001 From: g2flyer Date: Tue, 7 May 2024 12:01:03 -0700 Subject: [PATCH] amend! Fix dentry of open files after rename [LibOS] Fix dentry of open files after rename Signed-off-by: g2flyer --- libos/src/sys/libos_file.c | 20 ++++++++++++-------- libos/test/regression/rename_unlink.c | 26 +++++++++++++------------- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/libos/src/sys/libos_file.c b/libos/src/sys/libos_file.c index 8542f050e8..c3affcf03d 100644 --- a/libos/src/sys/libos_file.c +++ b/libos/src/sys/libos_file.c @@ -358,24 +358,28 @@ static int do_rename(struct libos_dentry* old_dent, struct libos_dentry* new_den new_dent->inode = old_dent->inode; old_dent->inode = NULL; - // also update dentry of any potentially open fd pointing to old_dent + + /* also update dentry of any potentially open fd pointing to old_dent */ struct libos_handle_map* handle_map = get_thread_handle_map(NULL); - rwlock_write_lock(&handle_map->lock); + assert(handle_libos_call != 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; - if ((handle->dentry != old_dent) || (handle->inode != new_dent->inode)) - continue; + /* TODO (MST): systematically go through all for other uses of handle->dentry as they also + * would require locking */ lock(&handle->lock); - handle->dentry = new_dent; - put_dentry(old_dent); - get_dentry(new_dent); + 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_write_unlock(&handle_map->lock); + rwlock_read_unlock(&handle_map->lock); return 0; } diff --git a/libos/test/regression/rename_unlink.c b/libos/test/regression/rename_unlink.c index ad8c0592c9..9ad70cfdea 100644 --- a/libos/test/regression/rename_unlink.c +++ b/libos/test/regression/rename_unlink.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-3.0-or-later */ -/* Copyright (C) 2021 Intel Corporation +/* Copyright (C) 2024 Intel Corporation * Paweł Marczewski + * Michael Steiner */ /* @@ -156,7 +157,6 @@ static void test_rename_replace(const char* path1, const char* path2) { err(1, "rename"); should_not_exist(path1); - should_exist(path2, message1_len); /* We expect `fd` to still point to old data, even though we replaced the file under its path */ @@ -182,9 +182,8 @@ static void test_rename_follow(const char* path1, const char* path2) { printf("%s...\n", __func__); int fd = create_file(path1, message1, message1_len); - if (fd < 0) - err(1, "open %s", path1); + err(1, "create %s", path1); if (rename(path1, path2) != 0) err(1, "rename"); @@ -220,17 +219,20 @@ static void test_rename_follow(const char* path1, const char* path2) { err(1, "unlink %s", path2); } -// NOTE: below will _not_ run correctly when directly executed unless you run as root. -// But it should run properly in gramine when executed as normal user. +/* NOTE: below will _not_ run correctly when directly executed unless you run as root. + * But it should run properly in Gramine when executed as normal user. */ static void test_rename_fchown_fchmod(const char* path1, const char* path2) { printf("%s...\n", __func__); int fd = create_file(path1, message1, message1_len); + if (fd < 0) + err(1, "create %s", path1); - if (fchown(fd, 1, 1)) + if (fchown(fd, /*owner=*/1, /*group=*/1) != 0) /* dummy owner/group just for testing */ err(1, "fchown before rename"); - if (fchmod(fd, S_IRWXU | S_IRWXG) != 0) // Note: no other! + if (fchmod(fd, S_IRWXU | S_IRWXG) != 0) /* note: no "other users" mode bits */ err(1, "fchmod before rename"); + struct stat st; if (stat(path1, &st) != 0) err(1, "Failed to stat file %s", path1); @@ -239,19 +241,17 @@ static void test_rename_fchown_fchmod(const char* path1, const char* path2) { if (st.st_mode & S_IRWXO) err(1, "wrong permissions of file %s", path1); - if (fd < 0) - err(1, "open %s", path1); - if (rename(path1, path2) != 0) err(1, "rename"); should_not_exist(path1); should_exist(path2, message1_len); - if (fchown(fd, 2, 2)) + if (fchown(fd, /*owner=*/2, /*group=*/2) != 0) /* different dummy owner/group */ err(1, "fchown after rename"); - if (fchmod(fd, S_IRWXU | S_IRWXG | S_IRWXO) != 0) // Note: with other now! + if (fchmod(fd, S_IRWXU | S_IRWXG | S_IRWXO) != 0) /* note: now with "other users" mode bits */ err(1, "fchmod after rename"); + if (stat(path2, &st) != 0) err(1, "Failed to stat (renamed) file %s", path2); if (st.st_uid != 2 || st.st_gid != 2)