Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9pfs, vfscore: Fix symlink support for 9pfs #830

Closed
wants to merge 3 commits into from

Conversation

andraprs
Copy link
Contributor

@andraprs andraprs commented Apr 10, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.pl on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): x86_64
  • Platform(s): kvm
  • Application(s): app-helloworld

Checked out the fix using:

  • app-helloworld built together with Unikraft.
unikraft/support/scripts/qemu-guest -k build/app-helloworld_kvm-x86_64 -e rootfs/
  • dynamically linked helloworld example and the app-elfloader.
unikraft/support/scripts/qemu-guest -k build/app-elfloader_kvm-x86_64 -i /lib64/ld-linux-x86-64.so.2 -e rootfs/ -a "--library-path / /helloworld"

// Open a symlink file

open("<symlink-file-path>", O_RDONLY);

<symlink-file> exists in the rootfs filesystem (9pfs type).

// Create a symlink file

symlink("symlink-target-file-path", "<symlink-file-path>");

<symlink-target-file> exists in the rootfs filesystem (9pfs type).

Additional configuration

Description of changes

@andraprs andraprs requested a review from a team as a code owner April 10, 2023 09:43
@razvand razvand self-assigned this Apr 10, 2023
@razvand razvand added this to the v0.13.0 (Atlas) milestone Apr 10, 2023
@razvand razvand added plat/kvm Unikraft for KVM lib/vfscore VFS Core Interface lib/9pfs 9p client lang/c Issues or PRs to do with C/C++ arch/x86_64 bug/fix This PR fixes a bug labels Apr 10, 2023
@razvand razvand requested review from John-Ted and flpostolache and removed request for a team April 10, 2023 09:52
@razvand
Copy link
Contributor

razvand commented Apr 10, 2023

Hi, @andraprs. Thanks for the fix. This works. I added two tests in the dynamic-apps repository to validate use as regressions in the future.

@maniatro111, @John-Ted, I'll wait for you to do your own review and testing, and then I'll approve the PR.

@John-Ted
Copy link
Contributor

Hi, I tested this and have encountered a problem. The symlink call and open on a symlink work separately, however if I try to run both in a single program, it sometimes fails, based on the elfloader config. I compiled the example program below with gcc -g -static-pie -fPIC -o symlink_test symlink.c.

config_notworking.txt
config_working.txt

Test program:

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>

#define FILENAME "symlink_inside_2"
#define ORIG_FILENAME "abc.txt"

int main()
{
        char buf[10];
        strcpy(buf, "abcdefg");
        int ret = symlink(ORIG_FILENAME, FILENAME);
        printf("symlink call returned %d\n", ret);
        if (ret < 0) {
                printf("failed to create symlink errno=%d\n", errno);
                return 1;
        }

        int fd = open(FILENAME, O_RDWR);
        printf("fd=%d\n", fd);
        if (fd < 0) {
                printf("open failed errno=%d\n", errno);
                return 1;
        }
        ret = write(fd, "abcd", 4);  //read(fd, buf, 9);
        buf[10]='\0';
        if (ret < 0) {
                printf("write failed errno=%d\n", errno);
                return 1;
        }
        //printf("read buffer: %s\n", buf);
        ret = close(fd);
        if (ret < 0) {
                printf("close failed errno=%d\n", errno);
                return 1;
        }
        return 0;
}

With the first config, I get the output:

[...]
[    3.574611] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.597988] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.617457] dbg:  [libuk9p] <9p.c @  787> RREADLINK
[    3.631817] dbg:  [libuk9p] <9p.c @  427> TCLUNK fid 2
[    3.647331] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.667536] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.686380] dbg:  [libuk9p] <9p.c @  431> RCLUNK
[    3.695698] dbg:  [libuk9p] <9p.c @  222> TWALK fid 0 newfid 2 nwname 1 name abc.txt��
[    3.716689] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.738683] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.755510] dbg:  [libuk9p] <9preq.c @  226> RERROR No such file or directory 2
[    3.774880] dbg:  [libvfscore] <main.c @  732> (ssize_t) uk_syscall_r_write((int) 0x1, (const void *) 0x400201270, (size_t) 0x32)
[    3.807149] dbg:  [libvfscore] <main.c @  686> (ssize_t) uk_syscall_r_writev((int) 0x1, (const struct iovec *) 0x5fec0f750, (int) 0x1)
symlink call returned 0
fd=-1
open failed errno=2
[    3.850627] dbg:  [libposix_process] <process.c @  564> (int) uk_syscall_r_exit_group((int) 0x1)
[    3.873975] dbg:  [libposix_process] <process.c @  367> Terminating PID 0: Self-killing TID 0...
[    3.896157] dbg:  [libuksched] <sched.c @  284> 0x400042018: thread 0x5fec8b018 (elfapp) terminated
[    3.920107] dbg:  [libuksched] <thread.c @  205> uk_thread 0x5fec8b018 (elfapp) term: Call termination 0x11e270()...
[    3.943385] dbg:  [libposix_process] <process.c @  448> thread 0x5fec8b018 (elfapp): Releasing thread with TID: 0 (PID: 0)
[    3.971122] dbg:  [libposix_process] <process.c @  322> Process PID 0 released
[...]

With the second config, I get:

SeaBIOS (version 1.15.0-1)
Booting from ROM...
Powered by
o.   .o       _ _               __ _
Oo   Oo  ___ (_) | __ __  __ _ ' _) :_
oO   oO ' _ `| | |/ /  _)' _` | |_|  _)
oOo oOO| | | | |   (| | | (_) |  _) :_
 OoOoO ._, ._:_:_,\_._,  .__,_:_, \___)
             Epimetheus 0.12.0~734f2489
symlink call returned 0
fd=3

On the first config, debug messages are enabled, on the second one, debug messages are disabled. There are no other differences as far as I can tell. In both cases, abc.txt exists in rootfs and the symlink does not exist.

Also, if a symlink inside rootfs points to a file outside rootfs, operations performed on the file (tested with read and write on an fd obtained with open(symlink)), the file is not modified in any way, but no error is thrown. It's not necessarily an issue, but I thought I should mention it just in case.

@andraprs
Copy link
Contributor Author

Hi, @andraprs. Thanks for the fix. This works. I added two tests in the dynamic-apps repository to validate use as regressions in the future.

@maniatro111, @John-Ted, I'll wait for you to do your own review and testing, and then I'll approve the PR.

Thank you for trying it out and adding the tests.

@andraprs
Copy link
Contributor Author

andraprs commented Apr 11, 2023

Hi, I tested this and have encountered a problem. The symlink call and open on a symlink work separately, however if I try to run both in a single program, it sometimes fails, based on the elfloader config. I compiled the example program below with gcc -g -static-pie -fPIC -o symlink_test symlink.c.

config_notworking.txt config_working.txt

Test program:

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>

#define FILENAME "symlink_inside_2"
#define ORIG_FILENAME "abc.txt"

int main()
{
        char buf[10];
        strcpy(buf, "abcdefg");
        int ret = symlink(ORIG_FILENAME, FILENAME);
        printf("symlink call returned %d\n", ret);
        if (ret < 0) {
                printf("failed to create symlink errno=%d\n", errno);
                return 1;
        }

        int fd = open(FILENAME, O_RDWR);
        printf("fd=%d\n", fd);
        if (fd < 0) {
                printf("open failed errno=%d\n", errno);
                return 1;
        }
        ret = write(fd, "abcd", 4);  //read(fd, buf, 9);
        buf[10]='\0';
        if (ret < 0) {
                printf("write failed errno=%d\n", errno);
                return 1;
        }
        //printf("read buffer: %s\n", buf);
        ret = close(fd);
        if (ret < 0) {
                printf("close failed errno=%d\n", errno);
                return 1;
        }
        return 0;
}

With the first config, I get the output:

[...]
[    3.574611] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.597988] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.617457] dbg:  [libuk9p] <9p.c @  787> RREADLINK
[    3.631817] dbg:  [libuk9p] <9p.c @  427> TCLUNK fid 2
[    3.647331] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.667536] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.686380] dbg:  [libuk9p] <9p.c @  431> RCLUNK
[    3.695698] dbg:  [libuk9p] <9p.c @  222> TWALK fid 0 newfid 2 nwname 1 name abc.txt��
[    3.716689] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.738683] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.755510] dbg:  [libuk9p] <9preq.c @  226> RERROR No such file or directory 2
[    3.774880] dbg:  [libvfscore] <main.c @  732> (ssize_t) uk_syscall_r_write((int) 0x1, (const void *) 0x400201270, (size_t) 0x32)
[    3.807149] dbg:  [libvfscore] <main.c @  686> (ssize_t) uk_syscall_r_writev((int) 0x1, (const struct iovec *) 0x5fec0f750, (int) 0x1)
symlink call returned 0
fd=-1
open failed errno=2
[    3.850627] dbg:  [libposix_process] <process.c @  564> (int) uk_syscall_r_exit_group((int) 0x1)
[    3.873975] dbg:  [libposix_process] <process.c @  367> Terminating PID 0: Self-killing TID 0...
[    3.896157] dbg:  [libuksched] <sched.c @  284> 0x400042018: thread 0x5fec8b018 (elfapp) terminated
[    3.920107] dbg:  [libuksched] <thread.c @  205> uk_thread 0x5fec8b018 (elfapp) term: Call termination 0x11e270()...
[    3.943385] dbg:  [libposix_process] <process.c @  448> thread 0x5fec8b018 (elfapp): Releasing thread with TID: 0 (PID: 0)
[    3.971122] dbg:  [libposix_process] <process.c @  322> Process PID 0 released
[...]

With the second config, I get:

SeaBIOS (version 1.15.0-1)
Booting from ROM...
Powered by
o.   .o       _ _               __ _
Oo   Oo  ___ (_) | __ __  __ _ ' _) :_
oO   oO ' _ `| | |/ /  _)' _` | |_|  _)
oOo oOO| | | | |   (| | | (_) |  _) :_
 OoOoO ._, ._:_:_,\_._,  .__,_:_, \___)
             Epimetheus 0.12.0~734f2489
symlink call returned 0
fd=3

On the first config, debug messages are enabled, on the second one, debug messages are disabled. There are no other differences as far as I can tell. In both cases, abc.txt exists in rootfs and the symlink does not exist.

Also, if a symlink inside rootfs points to a file outside rootfs, operations performed on the file (tested with read and write on an fd obtained with open(symlink)), the file is not modified in any way, but no error is thrown. It's not necessarily an issue, but I thought I should mention it just in case.

Thank you for the shared details, Teo. I'll try to reproduce and identify what happens.

@andraprs
Copy link
Contributor Author

Hi, I tested this and have encountered a problem. The symlink call and open on a symlink work separately, however if I try to run both in a single program, it sometimes fails, based on the elfloader config. I compiled the example program below with gcc -g -static-pie -fPIC -o symlink_test symlink.c.
config_notworking.txt config_working.txt
Test program:

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>

#define FILENAME "symlink_inside_2"
#define ORIG_FILENAME "abc.txt"

int main()
{
        char buf[10];
        strcpy(buf, "abcdefg");
        int ret = symlink(ORIG_FILENAME, FILENAME);
        printf("symlink call returned %d\n", ret);
        if (ret < 0) {
                printf("failed to create symlink errno=%d\n", errno);
                return 1;
        }

        int fd = open(FILENAME, O_RDWR);
        printf("fd=%d\n", fd);
        if (fd < 0) {
                printf("open failed errno=%d\n", errno);
                return 1;
        }
        ret = write(fd, "abcd", 4);  //read(fd, buf, 9);
        buf[10]='\0';
        if (ret < 0) {
                printf("write failed errno=%d\n", errno);
                return 1;
        }
        //printf("read buffer: %s\n", buf);
        ret = close(fd);
        if (ret < 0) {
                printf("close failed errno=%d\n", errno);
                return 1;
        }
        return 0;
}

With the first config, I get the output:

[...]
[    3.574611] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.597988] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.617457] dbg:  [libuk9p] <9p.c @  787> RREADLINK
[    3.631817] dbg:  [libuk9p] <9p.c @  427> TCLUNK fid 2
[    3.647331] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.667536] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.686380] dbg:  [libuk9p] <9p.c @  431> RCLUNK
[    3.695698] dbg:  [libuk9p] <9p.c @  222> TWALK fid 0 newfid 2 nwname 1 name abc.txt��
[    3.716689] dbg:  [libkvmvirtio] <virtio_ring.c @  367> Old head:0, new head:2, total_desc:2
[    3.738683] dbg:  [libkvmvirtio9p] <virtio_9p.c @  285> notify queue 0
[    3.755510] dbg:  [libuk9p] <9preq.c @  226> RERROR No such file or directory 2
[    3.774880] dbg:  [libvfscore] <main.c @  732> (ssize_t) uk_syscall_r_write((int) 0x1, (const void *) 0x400201270, (size_t) 0x32)
[    3.807149] dbg:  [libvfscore] <main.c @  686> (ssize_t) uk_syscall_r_writev((int) 0x1, (const struct iovec *) 0x5fec0f750, (int) 0x1)
symlink call returned 0
fd=-1
open failed errno=2
[    3.850627] dbg:  [libposix_process] <process.c @  564> (int) uk_syscall_r_exit_group((int) 0x1)
[    3.873975] dbg:  [libposix_process] <process.c @  367> Terminating PID 0: Self-killing TID 0...
[    3.896157] dbg:  [libuksched] <sched.c @  284> 0x400042018: thread 0x5fec8b018 (elfapp) terminated
[    3.920107] dbg:  [libuksched] <thread.c @  205> uk_thread 0x5fec8b018 (elfapp) term: Call termination 0x11e270()...
[    3.943385] dbg:  [libposix_process] <process.c @  448> thread 0x5fec8b018 (elfapp): Releasing thread with TID: 0 (PID: 0)
[    3.971122] dbg:  [libposix_process] <process.c @  322> Process PID 0 released
[...]

With the second config, I get:

SeaBIOS (version 1.15.0-1)
Booting from ROM...
Powered by
o.   .o       _ _               __ _
Oo   Oo  ___ (_) | __ __  __ _ ' _) :_
oO   oO ' _ `| | |/ /  _)' _` | |_|  _)
oOo oOO| | | | |   (| | | (_) |  _) :_
 OoOoO ._, ._:_:_,\_._,  .__,_:_, \___)
             Epimetheus 0.12.0~734f2489
symlink call returned 0
fd=3

On the first config, debug messages are enabled, on the second one, debug messages are disabled. There are no other differences as far as I can tell. In both cases, abc.txt exists in rootfs and the symlink does not exist.
Also, if a symlink inside rootfs points to a file outside rootfs, operations performed on the file (tested with read and write on an fd obtained with open(symlink)), the file is not modified in any way, but no error is thrown. It's not necessarily an issue, but I thought I should mention it just in case.

Thank you for the shared details, Teo. I'll try to reproduce and identify what happens.

I added a fix to this PR, for the mentioned issue. I checked it out using the provided test program. Let me know if the logic works fine now. Thank you. :)

With regard to the access outside rootfs, I tried out other security model than passthrough [1] e.g. mapped-xattr, but that doesn't let symlinks be opened at all. So it might be that a more fine grained access control logic needs to be enabled / developed. Thank you for mentioning this.

[1] https://wiki.qemu.org/Documentation/9psetup#Starting_the_Guest_directly

Copy link
Contributor

@John-Ted John-Ted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works fine now, thanks!

Reviewed-by: Ioan-Teodor Teugea ioan_teodor.teugea@stud.acs.upb.ro

@unikraft-bot unikraft-bot added the area/lib Internal Unikraft Microlibrary label Apr 13, 2023
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
4a37b8d lib/vfscore: Fix the mountpoint length for the symlink support
142c7c6 lib/vfscore: Initialize the fp variable to an empty string
45d00c7 lib/9pfs: Enable the symlink support for UK_9P_PROTO_2000U 9pfs version

@skuenzer skuenzer self-assigned this Apr 17, 2023
@flpostolache
Copy link
Contributor

Hello! Sorry for being so late. It also works on my side. To test this, I used this repo. It made easier to test symlink across multiple modes (ubuntu, simple-unikraft, unikraft with elfloader static executable and unikraft with elfloader dynamic executable).

I just have one question. I tried to create the ELOOP return code with symlink on Unikraft, but couldn't. The reason is in the symlink sycall implementation. Is there a reason for forcing the error to be ENOENT and not keep the error returned by lookup?

Otherwise, it works for me.

Copy link
Contributor

@flpostolache flpostolache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Florin Postolache florin.postolache.of@gmail.com

@andraprs
Copy link
Contributor Author

Hello! Sorry for being so late. It also works on my side. To test this, I used this repo. It made easier to test symlink across multiple modes (ubuntu, simple-unikraft, unikraft with elfloader static executable and unikraft with elfloader dynamic executable).

I just have one question. I tried to create the ELOOP return code with symlink on Unikraft, but couldn't. The reason is in the symlink sycall implementation. Is there a reason for forcing the error to be ENOENT and not keep the error returned by lookup?

Otherwise, it works for me.

No problem, thank you for trying it out. :)

With regard to the ENOENT error code, it seems this codebase has been imported as is. The following commit contains this codebase logic.

54e65bbb6

As we've discussed during the Application Compatibility community meeting, the next step is to create a GitHub issue to track this.

Thank you for looking into it.

Opening a symlink file doesn't successfully complete. The mountpoint
length provided to the function that resolves a symlink needs to be
adjusted.

Update the mountpoint length to be the difference in size between the
full path of the symlink file and the relative path of the symlink file
to the mountpoint.

Signed-off-by: Andra Paraschiv <andra@unikraft.io>
Sometimes the fp local string variable includes data that was
used before e.g. previous file paths. Thus, fp can contain characters
that are not part of the currently processed file path and different
errors can appear e.g. ENOENT (No such file or directory), when trying
to open a file.

Initialize the fp variable to an empty string before using it.

Signed-off-by: Andra Paraschiv <andra@unikraft.io>
Till now, symlink files could be created / opened only when using the
UK_9P_PROTO_2000L 9pfs version.

The current default 9pfs version is UK_9P_PROTO_2000U.

Enable the symlink support also for the UK_9P_PROTO_2000U 9pfs version.

Signed-off-by: Andra Paraschiv <andra@unikraft.io>
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @andraprs.

Reviewed-by: Razvan Deaconescu razvand@unikraft.io
Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Apr 24, 2023
Sometimes the fp local string variable includes data that was
used before e.g. previous file paths. Thus, fp can contain characters
that are not part of the currently processed file path and different
errors can appear e.g. ENOENT (No such file or directory), when trying
to open a file.

Initialize the fp variable to an empty string before using it.

Signed-off-by: Andra Paraschiv <andra@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #830
unikraft-bot pushed a commit that referenced this pull request Apr 24, 2023
Till now, symlink files could be created / opened only when using the
UK_9P_PROTO_2000L 9pfs version.

The current default 9pfs version is UK_9P_PROTO_2000U.

Enable the symlink support also for the UK_9P_PROTO_2000U 9pfs version.

Signed-off-by: Andra Paraschiv <andra@unikraft.io>
Reviewed-by: Florin Postolache <florin.postolache.of@gmail.com>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #830
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch/x86_64 area/lib Internal Unikraft Microlibrary bug/fix This PR fixes a bug ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lib/vfscore VFS Core Interface lib/9pfs 9p client plat/kvm Unikraft for KVM
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants