[PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Andrey Erokhin]

Directories attached using virtfs with security-model=mapped
may contain native symlinks

This can happen e.g. when booting from a rootfs directory tree
(usually with a writable overlay set up on the host side)

Currently, when security-model=mapped[-xattr|-file],
QEMU assumes that host-side "symlinks" are in the mapped format,
i.e. are regular files storing the linked path,
so it tries to open with O_NOFOLLOW
and fails with ELOOP on native symlinks

This patch introduces a fallback for such cases:
reuse security-model=[none|passthrough] else if branch logic
where readlink will be called for the path basename

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/173

Signed-off-by: Andrey Erokhin <language.lawyer@gmail.com>
---
 hw/9pfs/9p-local.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 6230466de1..ddf111b674 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -474,12 +474,16 @@ static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
 
         fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
         if (fd == -1) {
+            if (errno == ELOOP) {
+                goto native_symlink;
+            }
             return -1;
         }
         tsize = RETRY_ON_EINTR(read(fd, (void *)buf, bufsz));
         close_preserve_errno(fd);
     } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
                (fs_ctx->export_flags & V9FS_SM_NONE)) {
+    native_symlink:;
         char *dirpath = g_path_get_dirname(fs_path->data);
         char *name = g_path_get_basename(fs_path->data);
         int dirfd;
-- 
2.34.1

Re: [PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Greg Kurz]

On Sun, 23 Nov 2025 01:19:09 +0500
Andrey Erokhin <language.lawyer@gmail.com> wrote:

> Directories attached using virtfs with security-model=mapped
> may contain native symlinks
> 
> This can happen e.g. when booting from a rootfs directory tree
> (usually with a writable overlay set up on the host side)
> 
> Currently, when security-model=mapped[-xattr|-file],
> QEMU assumes that host-side "symlinks" are in the mapped format,
> i.e. are regular files storing the linked path,

This looks like an unfortunate design choice to start with. The mapped-xattr
mode was introduced to cache client-side uids/gids/mode in extended attributes.
Since there was no support in linux to set extended attributes on symlinks at
the time, 879c28133dfa ("virtio-9p: Security model for symlink and readlink")
opted to convert client originated symlinks into regular files.

A better choice would probably have been to leave symlinks as is and to cache
the metadata in a separate file, a bit like what the mapped-file does.

> so it tries to open with O_NOFOLLOW
> and fails with ELOOP on native symlinks
> 
> This patch introduces a fallback for such cases:
> reuse security-model=[none|passthrough] else if branch logic
> where readlink will be called for the path basename
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/173
> 
> Signed-off-by: Andrey Erokhin <language.lawyer@gmail.com>
> ---
>  hw/9pfs/9p-local.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 6230466de1..ddf111b674 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -474,12 +474,16 @@ static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
>  
>          fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
>          if (fd == -1) {
> +            if (errno == ELOOP) {
> +                goto native_symlink;
> +            }
>              return -1;
>          }
>          tsize = RETRY_ON_EINTR(read(fd, (void *)buf, bufsz));
>          close_preserve_errno(fd);
>      } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
>                 (fs_ctx->export_flags & V9FS_SM_NONE)) {
> +    native_symlink:;

Still has the terminating but unneeded semicolon. With that fixed,

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

>          char *dirpath = g_path_get_dirname(fs_path->data);
>          char *name = g_path_get_basename(fs_path->data);
>          int dirfd;



-- 
Greg

Re: [PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Andrey Erokhin]

>> +    native_symlink:;
> 
> Still has the terminating but unneeded semicolon

I think I've addressed this in the v1 thread, with links to the C11 draft grammar.
Can repeat in plain English: a label shall be followed by a statement. (No, declaration is not a statement)

Re: [PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Greg Kurz]

On Tue, 25 Nov 2025 19:21:00 +0500
Andrey Erokhin <language.lawyer@gmail.com> wrote:

> >> +    native_symlink:;
> > 
> > Still has the terminating but unneeded semicolon
> 
> I think I've addressed this in the v1 thread, with links to the C11 draft grammar.
> Can repeat in plain English: a label shall be followed by a statement. (No, declaration is not a statement)

My bad, I didn't see your answer.

It is funny that I had to pass -pedantic to gcc to get a complaint (in plain
English as well) if I drop the semicolon :

warning: a label can only be part of a statement and a declaration is not a statement [-Wpedantic]

Cheers,

-- 
Greg

Re: [PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Christian Schoenebeck]

On Tuesday, 25 November 2025 19:58:19 CET Greg Kurz wrote:
> On Tue, 25 Nov 2025 19:21:00 +0500
> 
> Andrey Erokhin <language.lawyer@gmail.com> wrote:
> > >> +    native_symlink:;
> > > 
> > > Still has the terminating but unneeded semicolon
> > 
> > I think I've addressed this in the v1 thread, with links to the C11 draft
> > grammar. Can repeat in plain English: a label shall be followed by a
> > statement. (No, declaration is not a statement)
> My bad, I didn't see your answer.
> 
> It is funny that I had to pass -pedantic to gcc to get a complaint (in plain
> English as well) if I drop the semicolon :
> 
> warning: a label can only be part of a statement and a declaration is not a
> statement [-Wpedantic]
> 
> Cheers,

Yes, I noticed that as well. GCC compiles fine without the semicolon, clang 
OTOH errors:

../hw/9pfs/9p-local.c:481:9: error: expected expression
        char *dirpath = g_path_get_dirname(fs_path->data);
        ^

Anyway, Andrey is right of course. The C standard defines a "labeled-
statement" as

  identifier : statement
  ...

and the subsequent line in the patch is a declaration, not a statement.

But I also understand if GCC developers relaxed this rule. Because it "feels" 
like both, a declaration and a statement. Interesting, because usually it's 
clang to be more relaxed than GCC.

/Christian

Re: [PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Andrey Erokhin]

On 26/11/2025 00:31, Christian Schoenebeck wrote:
> On Tuesday, 25 November 2025 19:58:19 CET Greg Kurz wrote:
>> On Tue, 25 Nov 2025 19:21:00 +0500
>>
>> Andrey Erokhin <language.lawyer@gmail.com> wrote:
>>>>> +    native_symlink:;
>>>>
>>>> Still has the terminating but unneeded semicolon
>>>
>>> I think I've addressed this in the v1 thread, with links to the C11 draft
>>> grammar. Can repeat in plain English: a label shall be followed by a
>>> statement. (No, declaration is not a statement)
>> My bad, I didn't see your answer.
>>
>> It is funny that I had to pass -pedantic to gcc to get a complaint (in plain
>> English as well) if I drop the semicolon :
>>
>> warning: a label can only be part of a statement and a declaration is not a
>> statement [-Wpedantic]
>>
>> Cheers,
> 
> Yes, I noticed that as well. GCC compiles fine without the semicolon, clang 
> OTOH errors:
> 
> ../hw/9pfs/9p-local.c:481:9: error: expected expression
>         char *dirpath = g_path_get_dirname(fs_path->data);
>         ^

I use clangd language server, so I don't even need to compile, I see the error immediately in the editor ;)

> Anyway, Andrey is right of course. The C standard defines a "labeled-
> statement" as
> 
>   identifier : statement
>   ...
> 
> and the subsequent line in the patch is a declaration, not a statement.
> 
> But I also understand if GCC developers relaxed this rule. Because it "feels" 
> like both, a declaration and a statement. Interesting, because usually it's 
> clang to be more relaxed than GCC.

Probably GCC allowed mixing declarations and statements in a block way before C99, in C++-like way, which has statement → declaration-statement → block-declaration production
(C99+ uses block-item → declaration|statement)

Re: [PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Christian Schoenebeck]

On Tuesday, 25 November 2025 21:40:57 CET Andrey Erokhin wrote:
> On 26/11/2025 00:31, Christian Schoenebeck wrote:
> > On Tuesday, 25 November 2025 19:58:19 CET Greg Kurz wrote:
[...]
> > Anyway, Andrey is right of course. The C standard defines a "labeled-
> > statement" as
> > 
> >   identifier : statement
> >   ...
> > 
> > and the subsequent line in the patch is a declaration, not a statement.
> > 
> > But I also understand if GCC developers relaxed this rule. Because it
> > "feels" like both, a declaration and a statement. Interesting, because
> > usually it's clang to be more relaxed than GCC.
> 
> Probably GCC allowed mixing declarations and statements in a block way
> before C99, in C++-like way, which has statement → declaration-statement →
> block-declaration production (C99+ uses block-item → declaration|statement)

It's not that old. It was introduced by the following commit in GCC, simply 
because the C23 standard now allows labels to "appear before declarations and 
at the end of compound statements.":

commit 8b7a9a249a63e066cff6e95db05a3158b4cc56cc
Author: Martin Uecker <muecker@gwdg.de>
Date:   Sat Nov 7 00:48:33 2020 +0100

    C Parser: Implement mixing of labels and code.
    
    Implement mixing of labels and code as adopted for C2X
    and process some std-attributes on labels.
    [...]


And in fact, clang -std=c23 accepts it without error or warning as well.

/Christian

Re: [PATCH v2] 9pfs: local: read native symlinks when security-model=mapped

[Christian Schoenebeck]

On Saturday, 22 November 2025 21:19:09 CET Andrey Erokhin wrote:
> Directories attached using virtfs with security-model=mapped
> may contain native symlinks
> 
> This can happen e.g. when booting from a rootfs directory tree
> (usually with a writable overlay set up on the host side)
> 
> Currently, when security-model=mapped[-xattr|-file],
> QEMU assumes that host-side "symlinks" are in the mapped format,
> i.e. are regular files storing the linked path,
> so it tries to open with O_NOFOLLOW
> and fails with ELOOP on native symlinks
> 
> This patch introduces a fallback for such cases:
> reuse security-model=[none|passthrough] else if branch logic
> where readlink will be called for the path basename
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/173
> 
> Signed-off-by: Andrey Erokhin <language.lawyer@gmail.com>
> ---
>  hw/9pfs/9p-local.c | 4 ++++
>  1 file changed, 4 insertions(+)

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

Thanks!

/Christian