[PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[Christian Schoenebeck]

This fixes an infamous, long standing bug:
https://gitlab.com/qemu-project/qemu/-/issues/103

* Actual fix of this bug is patch 5.

* Patches 1 and 6 add a test case to verify the expected behaviour.

* The other patches (2, 3, 4) are basically just minor cleanup patches more
  or less (un)related that I simply did not bother to send separately.

Probably there are still other 9p request types that should be fixed for this
use-after-unlink idiom, but this series fixes the mentioned bug report as
described by reporter, so fair enough to round this up here for now.

Simple test app to verify this behaviour on a Linux guest:

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>

int main() {
  struct stat st;
  int fd = open("doa-file", O_RDWR | O_CREAT | O_EXCL, 0600);
  unlink("doa-file");
  int res = fstat(fd, &st);
  printf("fstat() = %d\n", res);
  return res;
}

Christian Schoenebeck (6):
  tests/9p: add 'use-after-unlink' test
  tests/9p: fix Rreaddir response name
  tests/9p: add missing Rgetattr response name
  9pfs: remove obsolete comment in v9fs_getattr()
  9pfs: fix 'Tgetattr' after unlink
  tests/9p: also check 'Tgetattr' in 'use-after-unlink' test

 hw/9pfs/9p.c                          | 12 ++++---
 tests/qtest/libqos/virtio-9p-client.c |  3 +-
 tests/qtest/virtio-9p-test.c          | 46 +++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 6 deletions(-)

-- 
2.39.5

[PATCH 1/6] tests/9p: add 'use-after-unlink' test

[Christian Schoenebeck]

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 <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);
-- 
2.39.5

Re: [PATCH 1/6] tests/9p: add 'use-after-unlink' test

[Greg Kurz]

Hi Christian,

On Wed, 21 Feb 2024 15:13:13 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> 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 <qemu_oss@crudebyte.com>
> ---

Test looks good but make sure it is merged last to preserve bisect.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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);


Cheers,

-- 
Greg

Re: [PATCH 1/6] tests/9p: add 'use-after-unlink' test

[Christian Schoenebeck]

On Monday, November 25, 2024 9:47:17 AM CET Greg Kurz wrote:
> Hi Christian,
> 
> On Wed, 21 Feb 2024 15:13:13 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > 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 <qemu_oss@crudebyte.com>
> > ---
> 
> Test looks good but make sure it is merged last to preserve bisect.

I think there is a misapprehension: this test already passed! So no need to
move this patch.

What this test does is verifying the scenario open-unlink-write. I already
sent this patch in February and was surprised by myself that this idiom
already works:

https://lore.kernel.org/all/E1rcnYJ-0004KK-LV@lizzy.crudebyte.com/

What this entire series (i.e. patch 5) rather fixes is the idiom
open-unlink-fstat, and the test for this idiom is the last patch, not this
first one here.

So bisect is already fine.

> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!

/Christian

[PATCH 2/6] tests/9p: fix Rreaddir response name

[Christian Schoenebeck]

All 9p response types are prefixed with an "R", therefore fix
"READDIR" -> "RREADDIR" in function rmessage_name().

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 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" :
         "<unknown>";
 }
 
-- 
2.39.5

Re: [PATCH 2/6] tests/9p: fix Rreaddir response name

[Christian Schoenebeck]

On Sunday, November 24, 2024 2:34:31 PM CET Christian Schoenebeck wrote:
> All 9p response types are prefixed with an "R", therefore fix
> "READDIR" -> "RREADDIR" in function rmessage_name().
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Fixes: 4829469fd9ff ('tests/virtio-9p: added readdir test')

>  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" :
>          "<unknown>";
>  }
>  
> 

Re: [PATCH 2/6] tests/9p: fix Rreaddir response name

[Greg Kurz]

On Sun, 24 Nov 2024 14:34:31 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> All 9p response types are prefixed with an "R", therefore fix
> "READDIR" -> "RREADDIR" in function rmessage_name().
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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" :
>          "<unknown>";
>  }
>  



-- 
Greg

[PATCH 3/6] tests/9p: add missing Rgetattr response name

[Christian Schoenebeck]

'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.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 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" :
-- 
2.39.5

Re: [PATCH 3/6] tests/9p: add missing Rgetattr response name

[Christian Schoenebeck]

On Sunday, November 24, 2024 3:49:55 PM CET Christian Schoenebeck wrote:
> '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.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Fixes: a6821b828404 ('tests/9pfs: compare QIDs in fs_walk_none() test')

>  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" :
> 

Re: [PATCH 3/6] tests/9p: add missing Rgetattr response name

[Greg Kurz]

On Sun, 24 Nov 2024 15:49:55 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> '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.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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" :



-- 
Greg

[PATCH 4/6] 9pfs: remove obsolete comment in v9fs_getattr()

[Christian Schoenebeck]

The comment claims that we'd only support basic Tgetattr fields. This is
no longer true, so remove this comment.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 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;
-- 
2.39.5

Re: [PATCH 4/6] 9pfs: remove obsolete comment in v9fs_getattr()

[Christian Schoenebeck]

On Sunday, November 24, 2024 4:06:40 PM CET Christian Schoenebeck wrote:
> The comment claims that we'd only support basic Tgetattr fields. This is
> no longer true, so remove this comment.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Fixes: e06a765efbe3 ('hw/9pfs: Add st_gen support in getattr reply')

>  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;
> 

Re: [PATCH 4/6] 9pfs: remove obsolete comment in v9fs_getattr()

[Greg Kurz]

On Sun, 24 Nov 2024 16:06:40 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> The comment claims that we'd only support basic Tgetattr fields. This is
> no longer true, so remove this comment.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Heh another very long standing nit :-)

Good catch !

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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;



-- 
Greg

[PATCH 5/6] 9pfs: fix 'Tgetattr' after unlink

[Christian Schoenebeck]

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 stat() call
which fails as soon as the file got removed. So to fix this, use fstat()
whenever we have an open file descriptor already.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/103
Signed-off-by: Christian Schoenebeck <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;
     }
-- 
2.39.5

Re: [PATCH 5/6] 9pfs: fix 'Tgetattr' after unlink

[Christian Schoenebeck]

On Sunday, November 24, 2024 4:50:03 PM CET Christian Schoenebeck wrote:
> 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 stat() call
> which fails as soon as the file got removed. So to fix this, use fstat()
> whenever we have an open file descriptor already.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/103
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Fixes: 00ede4c2529b ('virtio-9p: getattr server implementation...')

>  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;
>      }
> 

Re: [PATCH 5/6] 9pfs: fix 'Tgetattr' after unlink

[Christian Schoenebeck]

On Sunday, November 24, 2024 4:50:03 PM CET Christian Schoenebeck wrote:
> 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 stat() call
> which fails as soon as the file got removed. So to fix this, use fstat()
> whenever we have an open file descriptor already.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/103
> Signed-off-by: Christian Schoenebeck <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);
> +    }

As for performance fstat() vs. lstat(): with glibc >= 2.39 and/or Linux
kernel >= 6.6, fstat() is Theta(1) whereas lstat() is O(log n). So fstat() is
faster than lstat() and hence prioritizing fstat() over lstat() does make
sense here IMO.

That's because on Linux kernel side fstat() is implemented by a simple
constant time linear array access via file descriptor number, whereas lstat()
needs to lookup the path and hence walk a tree.

There is a caveat though: Both on glibc and Linux kernel side there was a
performance bug each, which were both fixed in September 2023 by glibc 2.39
and Linux kernel 6.6 respectively:

kernel fix: https://github.com/torvalds/linux/commit/9013c51

glibc fix: https://github.com/bminor/glibc/commit/551101e

So on glibc side, due to a misconception, they inappropriately translated
fstat(fd, buf) -> fstatat(fd, "", buf, AT_EMPTY_PATH) for a long time, instead
of just calling fstat() directly as ought to be and done now.

And on kernel side, the negative performance impact of case AT_EMPTY_PATH +
empty string wasn't considered in fstatat() implementation. This case is now
short-circuited right at the beginning of the function.

/Christian

Re: [PATCH 5/6] 9pfs: fix 'Tgetattr' after unlink

[Greg Kurz]

On Tue, 26 Nov 2024 17:03:45 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Sunday, November 24, 2024 4:50:03 PM CET Christian Schoenebeck wrote:
> > 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 stat() call
> > which fails as soon as the file got removed. So to fix this, use fstat()
> > whenever we have an open file descriptor already.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/103
> > Signed-off-by: Christian Schoenebeck <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);
> > +    }
> 
> As for performance fstat() vs. lstat(): with glibc >= 2.39 and/or Linux
> kernel >= 6.6, fstat() is Theta(1) whereas lstat() is O(log n). So fstat() is
> faster than lstat() and hence prioritizing fstat() over lstat() does make
> sense here IMO.
> 
> That's because on Linux kernel side fstat() is implemented by a simple
> constant time linear array access via file descriptor number, whereas lstat()
> needs to lookup the path and hence walk a tree.
> 
> There is a caveat though: Both on glibc and Linux kernel side there was a
> performance bug each, which were both fixed in September 2023 by glibc 2.39
> and Linux kernel 6.6 respectively:
> 
> kernel fix: https://github.com/torvalds/linux/commit/9013c51
> 
> glibc fix: https://github.com/bminor/glibc/commit/551101e
> 
> So on glibc side, due to a misconception, they inappropriately translated
> fstat(fd, buf) -> fstatat(fd, "", buf, AT_EMPTY_PATH) for a long time, instead
> of just calling fstat() directly as ought to be and done now.
> 
> And on kernel side, the negative performance impact of case AT_EMPTY_PATH +
> empty string wasn't considered in fstatat() implementation. This case is now
> short-circuited right at the beginning of the function.
> 
> /Christian
> 
> 

Great explanation Christian !

Reviewed-by: Greg Kurz <groug@kaod.org>

Cheers,

-- 
Greg

[PATCH 6/6] tests/9p: also check 'Tgetattr' in 'use-after-unlink' test

[Christian Schoenebeck]

This verifies expected behaviour of previous bug fix patch.

Signed-off-by: Christian Schoenebeck <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
-- 
2.39.5

Re: [PATCH 6/6] tests/9p: also check 'Tgetattr' in 'use-after-unlink' test

[Greg Kurz]

On Sun, 24 Nov 2024 17:05:32 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This verifies expected behaviour of previous bug fix patch.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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



-- 
Greg

Re: [PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[Greg Kurz]

On Sun, 24 Nov 2024 17:28:40 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This fixes an infamous, long standing bug:
> https://gitlab.com/qemu-project/qemu/-/issues/103
> 

\o/

It is great if you manage to fix that once and far all !

> * Actual fix of this bug is patch 5.
> 
> * Patches 1 and 6 add a test case to verify the expected behaviour.
> 
> * The other patches (2, 3, 4) are basically just minor cleanup patches more
>   or less (un)related that I simply did not bother to send separately.
> 
> Probably there are still other 9p request types that should be fixed for this
> use-after-unlink idiom, but this series fixes the mentioned bug report as
> described by reporter, so fair enough to round this up here for now.
> 

When I last worked on that issue I had spotted some other places to fix.

Maybe you can find some ideas for future work at :

https://github.com/gkurz/qemu/tree/9p-attr-fixes

> Simple test app to verify this behaviour on a Linux guest:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <unistd.h>
> #include <fcntl.h>
> 
> int main() {
>   struct stat st;
>   int fd = open("doa-file", O_RDWR | O_CREAT | O_EXCL, 0600);
>   unlink("doa-file");
>   int res = fstat(fd, &st);
>   printf("fstat() = %d\n", res);
>   return res;
> }
> 
> Christian Schoenebeck (6):
>   tests/9p: add 'use-after-unlink' test
>   tests/9p: fix Rreaddir response name
>   tests/9p: add missing Rgetattr response name
>   9pfs: remove obsolete comment in v9fs_getattr()
>   9pfs: fix 'Tgetattr' after unlink
>   tests/9p: also check 'Tgetattr' in 'use-after-unlink' test
> 
>  hw/9pfs/9p.c                          | 12 ++++---
>  tests/qtest/libqos/virtio-9p-client.c |  3 +-
>  tests/qtest/virtio-9p-test.c          | 46 +++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 6 deletions(-)
> 

Cheers,

-- 
Greg

Re: [PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[Greg Kurz]

On Mon, 25 Nov 2024 09:45:54 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Sun, 24 Nov 2024 17:28:40 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > This fixes an infamous, long standing bug:
> > https://gitlab.com/qemu-project/qemu/-/issues/103
> > 
> 
> \o/
> 
> It is great if you manage to fix that once and far all !
> 

For the records. Original report was :

https://bugs.launchpad.net/qemu/+bug/1336794

> > * Actual fix of this bug is patch 5.
> > 
> > * Patches 1 and 6 add a test case to verify the expected behaviour.
> > 
> > * The other patches (2, 3, 4) are basically just minor cleanup patches more
> >   or less (un)related that I simply did not bother to send separately.
> > 
> > Probably there are still other 9p request types that should be fixed for this
> > use-after-unlink idiom, but this series fixes the mentioned bug report as
> > described by reporter, so fair enough to round this up here for now.
> > 
> 
> When I last worked on that issue I had spotted some other places to fix.
> 
> Maybe you can find some ideas for future work at :
> 
> https://github.com/gkurz/qemu/tree/9p-attr-fixes
> 
> > Simple test app to verify this behaviour on a Linux guest:
> > 
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <unistd.h>
> > #include <fcntl.h>
> > 
> > int main() {
> >   struct stat st;
> >   int fd = open("doa-file", O_RDWR | O_CREAT | O_EXCL, 0600);
> >   unlink("doa-file");
> >   int res = fstat(fd, &st);
> >   printf("fstat() = %d\n", res);
> >   return res;
> > }
> > 
> > Christian Schoenebeck (6):
> >   tests/9p: add 'use-after-unlink' test
> >   tests/9p: fix Rreaddir response name
> >   tests/9p: add missing Rgetattr response name
> >   9pfs: remove obsolete comment in v9fs_getattr()
> >   9pfs: fix 'Tgetattr' after unlink
> >   tests/9p: also check 'Tgetattr' in 'use-after-unlink' test
> > 
> >  hw/9pfs/9p.c                          | 12 ++++---
> >  tests/qtest/libqos/virtio-9p-client.c |  3 +-
> >  tests/qtest/virtio-9p-test.c          | 46 +++++++++++++++++++++++++++
> >  3 files changed, 55 insertions(+), 6 deletions(-)
> > 
> 
> Cheers,
> 



-- 
Greg

Re: [PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[Christian Schoenebeck]

On Monday, November 25, 2024 9:45:54 AM CET Greg Kurz wrote:
> On Sun, 24 Nov 2024 17:28:40 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > This fixes an infamous, long standing bug:
> > https://gitlab.com/qemu-project/qemu/-/issues/103
> > 
> 
> \o/
> 
> It is great if you manage to fix that once and far all !
> 
> > * Actual fix of this bug is patch 5.
> > 
> > * Patches 1 and 6 add a test case to verify the expected behaviour.
> > 
> > * The other patches (2, 3, 4) are basically just minor cleanup patches more
> >   or less (un)related that I simply did not bother to send separately.
> > 
> > Probably there are still other 9p request types that should be fixed for this
> > use-after-unlink idiom, but this series fixes the mentioned bug report as
> > described by reporter, so fair enough to round this up here for now.
> > 
> 
> When I last worked on that issue I had spotted some other places to fix.
> 
> Maybe you can find some ideas for future work at :
> 
> https://github.com/gkurz/qemu/tree/9p-attr-fixes

Was there a reason why you left those patches on the attic?

What I am seeing is that it was not fixing Tgetattr (i.e. fstat() on guest),
so it wouldn't have fixed the original reporter's scenario, but they would
have brought things forward. So just wondering ...

/Christian

Re: [PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[Greg Kurz]

On Mon, 25 Nov 2024 11:23:39 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Monday, November 25, 2024 9:45:54 AM CET Greg Kurz wrote:
> > On Sun, 24 Nov 2024 17:28:40 +0100
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > 
> > > This fixes an infamous, long standing bug:
> > > https://gitlab.com/qemu-project/qemu/-/issues/103
> > > 
> > 
> > \o/
> > 
> > It is great if you manage to fix that once and far all !
> > 
> > > * Actual fix of this bug is patch 5.
> > > 
> > > * Patches 1 and 6 add a test case to verify the expected behaviour.
> > > 
> > > * The other patches (2, 3, 4) are basically just minor cleanup patches more
> > >   or less (un)related that I simply did not bother to send separately.
> > > 
> > > Probably there are still other 9p request types that should be fixed for this
> > > use-after-unlink idiom, but this series fixes the mentioned bug report as
> > > described by reporter, so fair enough to round this up here for now.
> > > 
> > 
> > When I last worked on that issue I had spotted some other places to fix.
> > 
> > Maybe you can find some ideas for future work at :
> > 
> > https://github.com/gkurz/qemu/tree/9p-attr-fixes
> 
> Was there a reason why you left those patches on the attic?
> 

Lack of cycles

> What I am seeing is that it was not fixing Tgetattr (i.e. fstat() on guest),
> so it wouldn't have fixed the original reporter's scenario, but they would
> have brought things forward. So just wondering ...
> 

Yeah the fix for Tgetattr was in some other series I had sent at the time but
I did not get much feeback then...

> /Christian
> 
> 

Cheers,

-- 
Greg

Re: [PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[Christian Schoenebeck]

On Monday, November 25, 2024 12:35:05 PM CET Greg Kurz wrote:
> On Mon, 25 Nov 2024 11:23:39 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > On Monday, November 25, 2024 9:45:54 AM CET Greg Kurz wrote:
> > > On Sun, 24 Nov 2024 17:28:40 +0100
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
[...]
> > > > Probably there are still other 9p request types that should be fixed for this
> > > > use-after-unlink idiom, but this series fixes the mentioned bug report as
> > > > described by reporter, so fair enough to round this up here for now.
> > > > 
> > > 
> > > When I last worked on that issue I had spotted some other places to fix.
> > > 
> > > Maybe you can find some ideas for future work at :
> > > 
> > > https://github.com/gkurz/qemu/tree/9p-attr-fixes
> > 
> > Was there a reason why you left those patches on the attic?
> > 
> 
> Lack of cycles

Yeah, that's clear, I more meant in sense of known issues, as I haven't
spotted something obvious (above nit level) that would have spoken against
pushing those patches.

But OK, I also understand the lack of reviewers at that time, etc.

/Christian

> > What I am seeing is that it was not fixing Tgetattr (i.e. fstat() on guest),
> > so it wouldn't have fixed the original reporter's scenario, but they would
> > have brought things forward. So just wondering ...
> > 
> 
> Yeah the fix for Tgetattr was in some other series I had sent at the time but
> I did not get much feeback then...

Re: [PATCH 0/6] 9pfs: fix fstat() after unlink() (with a Linux guest)

[Christian Schoenebeck]

On Sunday, November 24, 2024 5:28:40 PM CET Christian Schoenebeck wrote:
> This fixes an infamous, long standing bug:
> https://gitlab.com/qemu-project/qemu/-/issues/103
> 
> * Actual fix of this bug is patch 5.
> 
> * Patches 1 and 6 add a test case to verify the expected behaviour.
> 
> * The other patches (2, 3, 4) are basically just minor cleanup patches more
>   or less (un)related that I simply did not bother to send separately.
> 
> Probably there are still other 9p request types that should be fixed for this
> use-after-unlink idiom, but this series fixes the mentioned bug report as
> described by reporter, so fair enough to round this up here for now.

Queued on 9p.next:
https://github.com/cschoenebeck/qemu/commits/9p.next

Thanks!

/Christian