[v2] 9pfs: Fix ftruncate-after-unlink

[PATCH v2 2/6] 9pfs: Don't use file descriptors in core code

Posted by Greg Kurz 3 weeks, 1 day ago

v9fs_getattr() currently peeks into V9fsFidOpenState to know if a fid
has a valid file descriptor or directory stream. Even though the fields
are accessible, this is an implementation detail of the local backend
that should not be manipulated directly by the server code.

Abstract that with a new has_valid_file_handle() backend operation.

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

v2: - rename to has_valid_file_handle()
    - don't reuse local_fid_fd()
---
 fsdev/file-op-9p.h | 1 +
 hw/9pfs/9p-local.c | 8 ++++++++
 hw/9pfs/9p-synth.c | 6 ++++++
 hw/9pfs/9p.c       | 9 ++++++---
 4 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 4997677460e8..b815cea44e85 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -164,6 +164,7 @@ struct FileOperations {
     int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char *old_name,
                     V9fsPath *newdir, const char *new_name);
     int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int flags);
+    bool (*has_valid_file_handle)(int fid_type, V9fsFidOpenState *fs);
 };
 
 #endif
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 99b9560a528b..b16132299f2c 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1572,6 +1572,13 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
     return 0;
 }
 
+static bool local_has_valid_file_handle(int fid_type, V9fsFidOpenState *fs)
+{
+    return
+        (fid_type == P9_FID_FILE && fs->fd != -1) ||
+        (fid_type == P9_FID_DIR && fs->dir.stream != NULL);
+}
+
 FileOperations local_ops = {
     .parse_opts = local_parse_opts,
     .init  = local_init,
@@ -1609,4 +1616,5 @@ FileOperations local_ops = {
     .name_to_path = local_name_to_path,
     .renameat  = local_renameat,
     .unlinkat = local_unlinkat,
+    .has_valid_file_handle = local_has_valid_file_handle,
 };
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 2abaf3a2918a..be0492b400e1 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -615,6 +615,11 @@ static int synth_init(FsContext *ctx, Error **errp)
     return 0;
 }
 
+static bool synth_has_valid_file_handle(int fid_type, V9fsFidOpenState *fs)
+{
+    return false;
+}
+
 FileOperations synth_ops = {
     .init         = synth_init,
     .lstat        = synth_lstat,
@@ -650,4 +655,5 @@ FileOperations synth_ops = {
     .name_to_path = synth_name_to_path,
     .renameat     = synth_renameat,
     .unlinkat     = synth_unlinkat,
+    .has_valid_file_handle = synth_has_valid_file_handle,
 };
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7cad2bce6209..10363f1a1df8 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1574,6 +1574,11 @@ out_nofid:
     pdu_complete(pdu, err);
 }
 
+static bool fid_has_valid_handle(V9fsState *s, V9fsFidState *fidp)
+{
+    return s->ops->has_valid_file_handle(fidp->fid_type, &fidp->fs);
+}
+
 static void coroutine_fn v9fs_getattr(void *opaque)
 {
     int32_t fid;
@@ -1596,9 +1601,7 @@ static void coroutine_fn v9fs_getattr(void *opaque)
         retval = -ENOENT;
         goto out_nofid;
     }
-    if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
-        (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream))
-    {
+    if (fid_has_valid_handle(pdu->s, fidp)) {
         retval = v9fs_co_fstat(pdu, fidp, &stbuf);
     } else {
         retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
-- 
2.48.1

Re: [PATCH v2 2/6] 9pfs: Don't use file descriptors in core code

Posted by Christian Schoenebeck 3 weeks ago

On Tuesday, March 11, 2025 6:28:05 PM CET Greg Kurz wrote:
> v9fs_getattr() currently peeks into V9fsFidOpenState to know if a fid
> has a valid file descriptor or directory stream. Even though the fields
> are accessible, this is an implementation detail of the local backend
> that should not be manipulated directly by the server code.
> 
> Abstract that with a new has_valid_file_handle() backend operation.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> v2: - rename to has_valid_file_handle()
>     - don't reuse local_fid_fd()
> ---
>  fsdev/file-op-9p.h | 1 +
>  hw/9pfs/9p-local.c | 8 ++++++++
>  hw/9pfs/9p-synth.c | 6 ++++++
>  hw/9pfs/9p.c       | 9 ++++++---
>  4 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 4997677460e8..b815cea44e85 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -164,6 +164,7 @@ struct FileOperations {
>      int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char *old_name,
>                      V9fsPath *newdir, const char *new_name);
>      int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int flags);
> +    bool (*has_valid_file_handle)(int fid_type, V9fsFidOpenState *fs);
>  };
>  
>  #endif
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 99b9560a528b..b16132299f2c 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1572,6 +1572,13 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
>      return 0;
>  }
>  
> +static bool local_has_valid_file_handle(int fid_type, V9fsFidOpenState *fs)
> +{
> +    return
> +        (fid_type == P9_FID_FILE && fs->fd != -1) ||
> +        (fid_type == P9_FID_DIR && fs->dir.stream != NULL);
> +}
> +
>  FileOperations local_ops = {
>      .parse_opts = local_parse_opts,
>      .init  = local_init,
> @@ -1609,4 +1616,5 @@ FileOperations local_ops = {
>      .name_to_path = local_name_to_path,
>      .renameat  = local_renameat,
>      .unlinkat = local_unlinkat,
> +    .has_valid_file_handle = local_has_valid_file_handle,
>  };
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 2abaf3a2918a..be0492b400e1 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -615,6 +615,11 @@ static int synth_init(FsContext *ctx, Error **errp)
>      return 0;
>  }
>  
> +static bool synth_has_valid_file_handle(int fid_type, V9fsFidOpenState *fs)
> +{
> +    return false;
> +}
> +
>  FileOperations synth_ops = {
>      .init         = synth_init,
>      .lstat        = synth_lstat,
> @@ -650,4 +655,5 @@ FileOperations synth_ops = {
>      .name_to_path = synth_name_to_path,
>      .renameat     = synth_renameat,
>      .unlinkat     = synth_unlinkat,
> +    .has_valid_file_handle = synth_has_valid_file_handle,
>  };
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 7cad2bce6209..10363f1a1df8 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1574,6 +1574,11 @@ out_nofid:
>      pdu_complete(pdu, err);
>  }
>  
> +static bool fid_has_valid_handle(V9fsState *s, V9fsFidState *fidp)
> +{
> +    return s->ops->has_valid_file_handle(fidp->fid_type, &fidp->fs);
> +}
> +

I would also rename that to fid_has_valid_file_handle(), but I can also do
this on my end.

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

>  static void coroutine_fn v9fs_getattr(void *opaque)
>  {
>      int32_t fid;
> @@ -1596,9 +1601,7 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>          retval = -ENOENT;
>          goto out_nofid;
>      }
> -    if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
> -        (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream))
> -    {
> +    if (fid_has_valid_handle(pdu->s, fidp)) {
>          retval = v9fs_co_fstat(pdu, fidp, &stbuf);
>      } else {
>          retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
>

[PATCH v2 1/6] 9pfs: local : Introduce local_fid_fd() helper
[PATCH v2 2/6] 9pfs: Don't use file descriptors in core code
[PATCH v2 3/6] 9pfs: Introduce ftruncate file op
[PATCH v2 4/6] 9pfs: Introduce futimens file op
[PATCH v2 5/6] tests/9p: add 'Tsetattr' request to test client
[PATCH v2 6/6] tests/9p: Test `Tsetattr` can truncate unlinked file