-
Notifications
You must be signed in to change notification settings - Fork 204
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
[LibOS] Use the syscall interface for custom calls #221
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
-- commits, line 13 at r1:
This is because we abuse Meson for patching Glibc :(
If we really want to, I can think of a way to force rebuild: rename our files and directories to glibc-2.34-1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @pwmarcz)
a discussion (no related file):
Hurray! We have reasonable names in the external interface with patched libraries!
Previously, pwmarcz (Paweł Marczewski) wrote…
This is because we abuse Meson for patching Glibc :(
If we really want to, I can think of a way to force rebuild: rename our files and directories to
glibc-2.34-1
.
I don't think we should bother.
LibOS/shim/include/arch/x86_64/gramine_entry_api.h, line 51 at r1 (raw file):
/* Magic syscall number for Gramine custom calls */ #define GRAMINE_CUSTOM_SYSCALL_NR ((unsigned long)-1)
Hm, do we want this looks-like-errno number? This may be confusing during debug.
Maybe some other constant which would look more like "ok, definitely not a regular Linux syscall but also not some errno/corrupted value"?
Looking at this table https://chromium.googlesource.com/chromiumos/docs/+/master/constants/syscalls.md#cross_arch-numbers, maybe we could use smth like 10000
or 1000000
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski, @dimakuv, and @mkow)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't think we should bother.
I'm still not sure, but as a counterpoint: if we don't do anything, this will be annoying not only when you pull new master, but also when working with feature branches that still have the old version.
(If you pull new master and don't rebuild Glibc, Gramine will crash; if you rebuild Glibc and run an old version, debugging will not work because of register_library
).
LibOS/shim/include/arch/x86_64/gramine_entry_api.h, line 51 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, do we want this looks-like-errno number? This may be confusing during debug.
Maybe some other constant which would look more like "ok, definitely not a regular Linux syscall but also not some errno/corrupted value"?
Looking at this table https://chromium.googlesource.com/chromiumos/docs/+/master/constants/syscalls.md#cross_arch-numbers, maybe we could use smth like
10000
or1000000
?
Sounds good. @boryspoplawski @mkow what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski, @mkow, and @pwmarcz)
Gramine will crash
Yeah, true, this is bad. Well, depends on how much effort it will be to implement this, and how much of a hack it looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski, @mkow, and @woju)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Gramine will crash
Yeah, true, this is bad. Well, depends on how much effort it will be to implement this, and how much of a hack it looks like.
I don't think it's too hacky. @woju what do you think? Does renaming everything to glibc-2.34-1
sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow, @pwmarcz, and @woju)
This is because we abuse Meson for patching Glibc :(
What is the alternative? I'm adding musl and just followed what we have for glibc. The process was super annoying, since I had to remove the subdirectory after each change...
+1 to rename
-- commits, line 13 at r1:
Do we want such comment in the commit message? (If we do the rename then probably not, but if we dont?)
LibOS/shim/include/shim_entry.h, line 9 at r1 (raw file):
* This file describes Gramine's entrypoints from userspace (mostly from patched glibc). * * The userspace wrappers for these functions are defined in `gramine_entry_api.h`.
Alternative approach: libos_entry_api.h
, so that this does not get confused with ecalls.
(Not blocking, just raising a discussion).
LibOS/shim/include/arch/x86_64/gramine_entry_api.h, line 1 at r1 (raw file):
/* SPDX-License-Identifier: LGPL-3.0-or-later */
git and reveiwable got lost, this does not show as rename + edit, so had to diff manually and we lost history of this file.
I have no idea how to do it differently thought :(
LibOS/shim/include/arch/x86_64/gramine_entry_api.h, line 51 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Sounds good. @boryspoplawski @mkow what do you think?
This is arch specific file, so we can pick something that fits x64 only (tho I would add a small comment about it). I would go for 0x100000
- high enough and has top bits clear (so we don't accidentally hit X32 bit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 11 of 17 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
What is the alternative? I'm adding musl and just followed what we have for glibc. The process was super annoying, since I had to remove the subdirectory after each change...
The alternative is not to use Meson? Use something else for building and installing these patched projects. This would complicate the build process, but might be better for our sanity :)
I did the rename. A small hack was necessary: the directory inside tarball is glibc-2.34
, so I'm telling Meson to unpack it to a subdirectory, i.e. glibc-2.34-1/glibc-2.34
.
Previously, boryspoplawski (Borys Popławski) wrote…
Do we want such comment in the commit message? (If we do the rename then probably not, but if we dont?)
I thought it would be helpful for developers wondering why Gramine suddenly crashes.
But renaming sounds better. I'm adding a squash!
commit with new wording.
LibOS/shim/include/shim_entry.h, line 9 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Alternative approach:
libos_entry_api.h
, so that this does not get confused with ecalls.
(Not blocking, just raising a discussion).
I prefer gramine_*
because libos_*
is not Gramine-specific.
(I can do renames, but since it's annoying to rebuild the patches, I'll do it at the end if there's consensus).
LibOS/shim/include/arch/x86_64/gramine_entry_api.h, line 51 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This is arch specific file, so we can pick something that fits x64 only (tho I would add a small comment about it). I would go for
0x100000
- high enough and has top bits clear (so we don't accidentally hit X32 bit).
OK. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 6 files at r2, all commit messages.
Reviewable status: 15 of 17 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @mkow and @pwmarcz)
The alternative is not to use Meson? Use something else for building and installing these patched projects. This would complicate the build process, but might be better for our sanity :)
Time to restore our old makefiles? :)
For real now: I think a makefile just downloading, patching and building glibc would be pretty small and easy, especially if we wrote it from scratch.
subprojects/glibc-2.34-1.wrap, line 14 at r2 (raw file):
patch_directory = glibc-2.34 # this unpacks the sources to `glibc-2.34-1/glibc-2.34`
What's the point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 17 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)
Previously, boryspoplawski (Borys Popławski) wrote…
The alternative is not to use Meson? Use something else for building and installing these patched projects. This would complicate the build process, but might be better for our sanity :)
Time to restore our old makefiles? :)
For real now: I think a makefile just downloading, patching and building glibc would be pretty small and easy, especially if we wrote it from scratch.
I guess... but we have quite a few of these projects that we download, patch and build, and they all have the same problem, it's just that some are modified less often than others. If you change the patches for toml
and uthash
, things will explode too. So it'd be good to have the same solution for all these projects.
But yeah, maybe that solution needs to be something different than Meson, seeing how painful it is to use...
subprojects/glibc-2.34-1.wrap, line 14 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
What's the point?
We really want the directory to be subprojects/glibc-2.34-1
, not subprojects/glibc-2.34
. Otherwise, Meson will see the directory exists, and not rebuild anything (even though the subproject name has changed).
Unfortunately, since the the tarball contains the glibc-2.34
directory, there's no way to directly unpack the files into glibc-2.34-1
, i.e. renaming the top-level directory. What you can do is lead_directory_missing
, which makes Meson create a directory and unpack the tarball inside it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 6 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @mkow)
subprojects/glibc-2.34-1.wrap, line 14 at r2 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
We really want the directory to be
subprojects/glibc-2.34-1
, notsubprojects/glibc-2.34
. Otherwise, Meson will see the directory exists, and not rebuild anything (even though the subproject name has changed).Unfortunately, since the the tarball contains the
glibc-2.34
directory, there's no way to directly unpack the files intoglibc-2.34-1
, i.e. renaming the top-level directory. What you can do islead_directory_missing
, which makes Meson create a directory and unpack the tarball inside it.
I thought directory
property forces meson to unpack into such directory, but seems it just specify where to find the result of unpacking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " and "fixup! " found in commit messages' one-liners (waiting on @mkow)
Previously, pwmarcz (Paweł Marczewski) wrote…
I guess... but we have quite a few of these projects that we download, patch and build, and they all have the same problem, it's just that some are modified less often than others. If you change the patches for
toml
anduthash
, things will explode too. So it'd be good to have the same solution for all these projects.But yeah, maybe that solution needs to be something different than Meson, seeing how painful it is to use...
I think I'm annoyed enough to work on this. But first let's hear from others @dimakuv @mkow @woju
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "squash! " and "fixup! " found in commit messages' one-liners
Previously, boryspoplawski (Borys Popławski) wrote…
I think I'm annoyed enough to work on this. But first let's hear from others @dimakuv @mkow @woju
I don't have a strong opinion on this. I'm not too annoyed at periodically deleting the subprojects. But I'm also not working on any such changes, so I don't need to rm them every 5 minutes :)
Anyway, why not the Meson hack with renaming from Pawel? If it works, I would prefer this hack instead of Makefiles.
LibOS/shim/include/shim_entry.h, line 9 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
I prefer
gramine_*
becauselibos_*
is not Gramine-specific.(I can do renames, but since it's annoying to rebuild the patches, I'll do it at the end if there's consensus).
I like gramine_entry_api.h
, as it is now. I don't think this can be confused with ECALLs -- those have sgx_entry
, enclave_entry
names -- very specific to SGX and ECALLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, all discussions resolved, "squash! " and "fixup! " found in commit messages' one-liners
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't have a strong opinion on this. I'm not too annoyed at periodically deleting the subprojects. But I'm also not working on any such changes, so I don't need to rm them every 5 minutes :)
Anyway, why not the Meson hack with renaming from Pawel? If it works, I would prefer this hack instead of Makefiles.
I think rename is a good workaround for now, but this is getting worse with each project.
To sum up:
- No dependency tracking: Meson doesn't rebuild these subprojects if you change a patch or anything else.
- The projects are tied to a specific Meson build, so I have to rebuild glibc for each Gramine version (debug, release, sanitizers...), and rebuild it each time I recreate a build dir for some reason.
- We implement most of the functionality ourselves using a messy
compile.sh
shell script:- Copy the source code to another location, apply patches
- Invoke external build system, then copy the build artifact back where Meson expects them
- Manage build output (so that we don't output megabytes of build logs to console)
- Meson needs to know exactly what files will be produced in order to install them
- For LTP, which installs thousands of files, I gave up and create and then unpack a tarball instead
Looking at the list above, I think that having a separate system for building all of these dependencies FAR outweighs the costs of fighting Meson.
This change modifies Gramine's custom calls (`register_library`, `run_test`) to use the same interface as system calls (with a dummy syscall number). This is so that these calls are executed on LibOS's stack, not the user application stack, which will make it possible to implement ASan stack sanitization. At the same time, names in the syscall interface are updated (`gramine_entry_api.h`, `GRAMINE_SYSCALL` etc.). This change requires rebuilding the patched Glibc. To force Meson to do that automatically, we rename the subproject from `glibc-2.34` to `glibc-2.34-1`. Signed-off-by: Paweł Marczewski <[email protected]>
c85428f
to
1fd075e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
Previously, pwmarcz (Paweł Marczewski) wrote…
I think rename is a good workaround for now, but this is getting worse with each project.
To sum up:
- No dependency tracking: Meson doesn't rebuild these subprojects if you change a patch or anything else.
- The projects are tied to a specific Meson build, so I have to rebuild glibc for each Gramine version (debug, release, sanitizers...), and rebuild it each time I recreate a build dir for some reason.
- We implement most of the functionality ourselves using a messy
compile.sh
shell script:
- Copy the source code to another location, apply patches
- Invoke external build system, then copy the build artifact back where Meson expects them
- Manage build output (so that we don't output megabytes of build logs to console)
- Meson needs to know exactly what files will be produced in order to install them
- For LTP, which installs thousands of files, I gave up and create and then unpack a tarball instead
Looking at the list above, I think that having a separate system for building all of these dependencies FAR outweighs the costs of fighting Meson.
Fully agreed with @pwmarcz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 6 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 12 files at r1, 5 of 6 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
No dependency tracking: Meson doesn't rebuild these subprojects if you change a patch or anything else.
I'm still very negatively surprised by this... I assumed that Meson will have rough edges, as it's a relatively new buildsystem, but I assumed that a modern buildsystem wouldn't include such a broken UX/design...
We implement most of the functionality ourselves using a messy compile.sh shell script:
I think this part could be fixed by this PR: mesonbuild/meson#4570, but it got stuck a few months ago.
Looking at the list above, I think that having a separate system for building all of these dependencies FAR outweighs the costs of fighting Meson.
That unfortunately may be true.
LibOS/shim/include/arch/x86_64/gramine_entry_api.h, line 51 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
OK. Done.
Late to the game, but +1 to the decision you've made :)
Description of the changes
This change modifies Gramine's custom calls (
register_library
,run_test
) to use the same interface as system calls (with a dummy syscall number). This is so that these calls are executed on LibOS's stack, not the user application stack, which will make it possible to implement ASan stack sanitization.At the same time, names in the syscall interface are updated (
gramine_entry_api.h
,GRAMINE_SYSCALL
etc.).This change requires rebuilding the patched Glibc. Unfortunately, this currently does not happen automatically. You can trigger the rebuild by removing the
subprojects/glibc-2.34
directory.Fixes #25.
How to test this PR?
CI.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)