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

lib/9pfs: Fix create call for 9p.2000L #934

Closed

Conversation

StefanJum
Copy link
Member

@StefanJum StefanJum commented Jun 5, 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.uk on your commit series before opening this PR;
  • Updated relevant documentation.

The uk_9pfs_create call for 9p.2000L uses posix flags for open file
permissions. The 9P server however will expect specific 9p2000.L flags.

9p2000.L flags declaration are taken from the 9p protocol manual and
QEMU implementation:

Most of the flags are identical, so using posix flags did not cause any troubles just yet, but
we should use the proper ones.
Discovered while investigating unikraft/run-app-elfloader#16

@StefanJum StefanJum added kind/bug Something isn't working lib/9pfs 9p client lib/uk9p bug/runtime Bug occurs during runtime bug/fix This PR fixes a bug labels Jun 5, 2023
@razvand razvand requested a review from flpostolache June 5, 2023 10:33
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 5, 2023
@razvand
Copy link
Contributor

razvand commented Jun 5, 2023

Hi, @StefanJum, I know this solves the issue, but it doesn't attack the underlying problem of the calls made for 9P.2000L. Moreover, it doesn't explain why the creation of files works in native mode. Only binary-compatibility mode is affected by the issue unikraft/run-app-elfloader#16.

@cbanu96
Copy link
Contributor

cbanu96 commented Jun 9, 2023

I was nerd-sniped by this issue and investigated a bit the logs from unikraft/run-app-elfloader#16.

[ 1.142817] dbg: [libuk9p] <9p.c @ 371> TLCREATE fid 2 mode 33152

The mode in the line above seemed weird, which led me to the stack trace.

unikraft/unikraft#5 uk_9p_lcreate (dev=0x47f900020, fid=0x47f911020, name=name@entry=0x40005e9d1 "a.txt", flags=flags@entry=513, mode=mode@entry=33152, gid=gid@entry=0)
unikraft/unikraft#6 0x0000000000119851 in uk_9pfs_create (dvp=<optimized out>, name=0x40005e9d1 "a.txt", mode=33152) at /media/razvan/c4f6765a-efa5-4ebd-9cf0-7da9908a0189/razvan/unikraft/scripts/workdir/unikraft/lib/9pfs/9pfs_vnops.c:397
unikraft/unikraft#7 0x000000000016d371 in sys_open (path=path@entry=0x40005e9d0 "/a.txt", flags=67, flags@entry=66, mode=<optimized out>, mode@entry=384, fpp=fpp@entry=0x40005e9c8)

It looks like both mode and flags arrive to uk_9p_lcreate from vfscore with different values than they originally had in the open() call:

  • mode: vfscore sets S_IFREG bit before dispatching VOP_CREATE. This can be undone in the 9pfs callback via & ~S_IFMT.
  • flags: sys_open has them, uk_9pfs_create doesn't - it looks like they are just not threaded through from vfscore, so uk_9pfs_create always picks O_WRONLY | O_TRUNC.

Separately the value of mode=384 didn't make sense given the open() call that's being tested. I think that should probably be open(O_RDONLY | O_CREAT, 0x2) instead of open(O_RDONLY | O_CREAT | 0x2) according to open(2):

The mode argument must be supplied if O_CREAT or O_TMPFILE is specified in flags; if it is not supplied, some arbitrary bytes from the stack will be applied as the file mode.

I'm not yet fully satisfied with this answer as it doesn't explain the difference between binary-compat and native, but it might help point in the right direction.


LE: On further thought and investigation, while the above are slightly problematic when using 9P.2000L I don't believe this is why the binary-compat is failing.
Rather, it might be due to the security-model the binary-compatibility scripts are enforcing.

uk_9p_lcreate sends gid=0, and I'm pretty sure qemu-9p will try to chown(gid). If chown fails (i.e. missing CAP_CHOWN or not running as root), the error will be ignored only on security-model=none.

I don't have kernel development stuff installed on my workstation to test this out now, but I'll give it a try tomorrow.

@StefanJum
Copy link
Member Author

StefanJum commented Jun 9, 2023

uk_9p_lcreate sends gid=0, and I'm pretty sure qemu-9p will try to chown(gid). If chown fails (i.e. missing CAP_CHOWN or not running as root), the error will be ignored only on security-model=none.

I don't have kernel development stuff installed on my workstation to test this out now, but I'll give it a try tomorrow.

Hi @cbanu96, thanks for looking into this 🎉.
We've reached the same conclusion, it's the security model fault 😄
The passthrough security model should not really be used in our case, and when it is used qemu should be run as root.
The fail happens here, where fchown will obviously fail if we don't run as root.

We'll still need to update the flags of the lcreate request to use qemu dotl ones for the request to be 100% right, but for now they happen to have the same values so just changing the security model works fine .

@razvand razvand added the size/small Small PR, quick to review label Jun 28, 2023
The `uk_9pfs_create` call for `9p.2000L` uses posix flags for open file
permissions. The 9P server however will expect specific 9p2000.L flags.

9p2000.L flags declaration are taken from the 9p protocol manual and
QEMU implementation:

* http://9p.io/magic/man2html/5/intro
* https://lxr.missinglinkelectronics.com/qemu/hw/9pfs/9p.h#L368

Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>
@StefanJum StefanJum marked this pull request as ready for review July 18, 2023 20:00
@StefanJum StefanJum requested a review from a team as a code owner July 18, 2023 20:00
@StefanJum StefanJum removed the request for review from a team July 19, 2023 06:27
@unikraft-bot unikraft-bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ labels Jul 19, 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
ca5ec33 lib/9pfs: Fix create call for 9p.2000L

Copy link
Contributor

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

All good with me. I tested with most of the apps and seems to work fine. Many thanks @StefanJum

Reviewed-by: Radu Nichita radunichita99@gmail.com

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.

Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary bug/fix This PR fixes a bug bug/runtime Bug occurs during runtime ci/merged Merged by CI kind/bug Something isn't working lang/c Issues or PRs to do with C/C++ lib/uk9p lib/9pfs 9p client size/small Small PR, quick to review
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants