From f2db23ede5b3015330c2314f796dd694f35c7508 Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Thu, 21 Nov 2024 11:52:48 +0100 Subject: [PATCH 1/7] 9pfs: cleanup V9fsFidState Drop V9fsFidState's 'next' member, which is no longer used since: f5265c8f917e ('9pfs: use GHashTable for fid table') Fixes: f5265c8f917e ('9pfs: use GHashTable for fid table') Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: --- hw/9pfs/9p.h | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h index a6f59abccb..5e041e1f60 100644 --- a/hw/9pfs/9p.h +++ b/hw/9pfs/9p.h @@ -280,7 +280,6 @@ struct V9fsFidState { uid_t uid; int ref; bool clunked; - QSIMPLEQ_ENTRY(V9fsFidState) next; QSLIST_ENTRY(V9fsFidState) reclaim_next; }; From 462db8fb1d405391b83a0d3099fdb9bfb85c2d92 Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Wed, 21 Feb 2024 15:13:13 +0100 Subject: [PATCH 2/7] tests/9p: add 'use-after-unlink' test After removing a file from the file system, we should still be able to work with the file if we already had it open before removal. As a first step we verify that it is possible to write to an unlinked file, as this is what already works. This test is extended later on after having fixed other use cases after unlink that are not working yet. Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: <3d6449d4df25bcdd3e807eff169f46f1385e5257.1732465720.git.qemu_oss@crudebyte.com> --- tests/qtest/virtio-9p-test.c | 41 ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c index 3c8cd235cf..f6d7400a87 100644 --- a/tests/qtest/virtio-9p-test.c +++ b/tests/qtest/virtio-9p-test.c @@ -693,6 +693,45 @@ static void fs_unlinkat_hardlink(void *obj, void *data, g_assert(stat(real_file, &st_real) == 0); } +static void fs_use_after_unlink(void *obj, void *data, + QGuestAllocator *t_alloc) +{ + QVirtio9P *v9p = obj; + v9fs_set_allocator(t_alloc); + static const uint32_t write_count = P9_MAX_SIZE / 2; + g_autofree char *real_file = virtio_9p_test_path("09/doa_file"); + g_autofree char *buf = g_malloc0(write_count); + struct stat st_file; + uint32_t fid_file; + uint32_t count; + + tattach({ .client = v9p }); + + /* create a file "09/doa_file" and make sure it exists and is regular */ + tmkdir({ .client = v9p, .atPath = "/", .name = "09" }); + tlcreate({ .client = v9p, .atPath = "09", .name = "doa_file" }); + g_assert(stat(real_file, &st_file) == 0); + g_assert((st_file.st_mode & S_IFMT) == S_IFREG); + + /* request a FID for that regular file that we can work with next */ + fid_file = twalk({ + .client = v9p, .fid = 0, .path = "09/doa_file" + }).newfid; + g_assert(fid_file != 0); + + /* now first open the file in write mode before ... */ + tlopen({ .client = v9p, .fid = fid_file, .flags = O_WRONLY }); + /* ... removing the file from file system */ + tunlinkat({ .client = v9p, .atPath = "09", .name = "doa_file" }); + + /* file is removed, but we still have it open, so this should succeed */ + count = twrite({ + .client = v9p, .fid = fid_file, .offset = 0, .count = write_count, + .data = buf + }).count; + g_assert_cmpint(count, ==, write_count); +} + static void cleanup_9p_local_driver(void *data) { /* remove previously created test dir when test is completed */ @@ -758,6 +797,8 @@ static void register_virtio_9p_test(void) qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts); qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink, &opts); + qos_add_test("local/use_after_unlink", "virtio-9p", fs_use_after_unlink, + &opts); } libqos_init(register_virtio_9p_test); From abf0f092c1dd33b9ffa986c6924addc0a9c1d0b8 Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sun, 24 Nov 2024 14:34:31 +0100 Subject: [PATCH 3/7] tests/9p: fix Rreaddir response name All 9p response types are prefixed with an "R", therefore fix "READDIR" -> "RREADDIR" in function rmessage_name(). Fixes: 4829469fd9ff ("tests/virtio-9p: added readdir test") Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: --- tests/qtest/libqos/virtio-9p-client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c index b8adc8d4b9..c61632fcd3 100644 --- a/tests/qtest/libqos/virtio-9p-client.c +++ b/tests/qtest/libqos/virtio-9p-client.c @@ -238,7 +238,7 @@ static const char *rmessage_name(uint8_t id) id == P9_RLINK ? "RLINK" : id == P9_RUNLINKAT ? "RUNLINKAT" : id == P9_RFLUSH ? "RFLUSH" : - id == P9_RREADDIR ? "READDIR" : + id == P9_RREADDIR ? "RREADDIR" : ""; } From 4ec984965079b51a9afce339af75edea6de973a2 Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sun, 24 Nov 2024 15:49:55 +0100 Subject: [PATCH 4/7] tests/9p: add missing Rgetattr response name 'Tgetattr' 9p request and its 'Rgetattr' response types are already used by test client, however this response type is yet missing in function rmessage_name(), so add it. Fixes: a6821b828404 ("tests/9pfs: compare QIDs in fs_walk_none() test") Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: --- tests/qtest/libqos/virtio-9p-client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c index c61632fcd3..98b77db51d 100644 --- a/tests/qtest/libqos/virtio-9p-client.c +++ b/tests/qtest/libqos/virtio-9p-client.c @@ -235,6 +235,7 @@ static const char *rmessage_name(uint8_t id) id == P9_RMKDIR ? "RMKDIR" : id == P9_RLCREATE ? "RLCREATE" : id == P9_RSYMLINK ? "RSYMLINK" : + id == P9_RGETATTR ? "RGETATTR" : id == P9_RLINK ? "RLINK" : id == P9_RUNLINKAT ? "RUNLINKAT" : id == P9_RFLUSH ? "RFLUSH" : From 3bc4db44430f53387d17145bb52b330a830a03fe Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sun, 24 Nov 2024 16:06:40 +0100 Subject: [PATCH 5/7] 9pfs: remove obsolete comment in v9fs_getattr() The comment claims that we'd only support basic Tgetattr fields. This is no longer true, so remove this comment. Fixes: e06a765efbe3 ("hw/9pfs: Add st_gen support in getattr reply") Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: --- hw/9pfs/9p.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 9a291d1b51..851e36b9a1 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1596,10 +1596,6 @@ static void coroutine_fn v9fs_getattr(void *opaque) retval = -ENOENT; goto out_nofid; } - /* - * Currently we only support BASIC fields in stat, so there is no - * need to look at request_mask. - */ retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf); if (retval < 0) { goto out; From c81e7219e0736f80bfd3553676a19e2992cff41d Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sun, 24 Nov 2024 16:50:03 +0100 Subject: [PATCH 6/7] 9pfs: fix 'Tgetattr' after unlink With a valid file ID (FID) of an open file, it should be possible to send a 'Tgettattr' 9p request and successfully receive a 'Rgetattr' response, even if the file has been removed in the meantime. Currently this would fail with ENOENT. I.e. this fixes the following misbehaviour with a 9p Linux client: open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3 unlink("/home/tst/filename") = 0 fstat(3, 0x23aa1a8) = -1 ENOENT (No such file or directory) Expected results: open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3 unlink("/home/tst/filename") = 0 fstat(3, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 This is because 9p server is always using a path name based lstat() call which fails as soon as the file got removed. So to fix this, use fstat() whenever we have an open file descriptor already. Fixes: 00ede4c2529b ("virtio-9p: getattr server implementation...") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/103 Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: <4c41ad47f449a5cc8bfa9285743e029080d5f324.1732465720.git.qemu_oss@crudebyte.com> --- hw/9pfs/9p.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 851e36b9a1..578517739a 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1596,7 +1596,13 @@ static void coroutine_fn v9fs_getattr(void *opaque) retval = -ENOENT; goto out_nofid; } - retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf); + if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) || + (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream)) + { + retval = v9fs_co_fstat(pdu, fidp, &stbuf); + } else { + retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf); + } if (retval < 0) { goto out; } From eaab44ccc59b83d8dff60fca3361a9b98ec7fee6 Mon Sep 17 00:00:00 2001 From: Christian Schoenebeck Date: Sun, 24 Nov 2024 17:05:32 +0100 Subject: [PATCH 7/7] tests/9p: also check 'Tgetattr' in 'use-after-unlink' test This verifies expected behaviour of previous bug fix patch. Signed-off-by: Christian Schoenebeck Reviewed-by: Greg Kurz Message-Id: <7017658155c517b9665b75333a97c79aa2d4f3df.1732465720.git.qemu_oss@crudebyte.com> --- tests/qtest/virtio-9p-test.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c index f6d7400a87..ab3a12c816 100644 --- a/tests/qtest/virtio-9p-test.c +++ b/tests/qtest/virtio-9p-test.c @@ -702,6 +702,7 @@ static void fs_use_after_unlink(void *obj, void *data, g_autofree char *real_file = virtio_9p_test_path("09/doa_file"); g_autofree char *buf = g_malloc0(write_count); struct stat st_file; + struct v9fs_attr attr; uint32_t fid_file; uint32_t count; @@ -725,6 +726,10 @@ static void fs_use_after_unlink(void *obj, void *data, tunlinkat({ .client = v9p, .atPath = "09", .name = "doa_file" }); /* file is removed, but we still have it open, so this should succeed */ + tgetattr({ + .client = v9p, .fid = fid_file, .request_mask = P9_GETATTR_BASIC, + .rgetattr.attr = &attr + }); count = twrite({ .client = v9p, .fid = fid_file, .offset = 0, .count = write_count, .data = buf