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

[Docs] Add "Gramine features" (in Markdown) #1194

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

dimakuv
Copy link

@dimakuv dimakuv commented Feb 21, 2023

Description of the changes

NOTE: It's unclear how to render this document in RestructedText / ReadTheDocs as it seems to not support nested inline markup. So the document is written in GitHub-flavoured Markdown format.

How to test this PR?

Read it: https://github.com/gramineproject/gramine/blob/dimakuv/add-gramine-features-doc/Documentation/misc/features.md

UPDATE: Now located here: https://github.com/gramineproject/gramine/blob/dimakuv/add-gramine-features-doc/Documentation/devel/features.md

Or here in ReadTheDocs: https://gramine.readthedocs.io/en/dimakuv-add-gramine-features-doc/devel/features.html


This change is Reviewable

Copy link
Contributor

@llly llly left a 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 1 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)


Documentation/misc/features.md line 631 at r1 (raw file):

  <sup>[1](#file-system-operations)</sup>

- :x:`_sysctl()`

extra '_'


Documentation/misc/features.md line 1547 at r1 (raw file):

### Process and thread identifiers

Gramine fully supports the following indentifiers: Process IDs (PIDs), Parent Process IDs (PPIDs),

identifiers

Code quote:

indentifiers

Documentation/misc/features.md line 1897 at r1 (raw file):

### User and group identifiers

Gramine has dummy support for the following indentifiers:

identifiers

Code quote:

indentifiers

Documentation/misc/features.md line 2062 at r1 (raw file):

currently not supported.

Gramine support file truncation (via `truncate()` and `ftruncate()`). There is one exception

supports

Code quote:

support

Copy link
Author

@dimakuv dimakuv left a 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 1 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @llly)


Documentation/misc/features.md line 631 at r1 (raw file):

Previously, llly (Li Xun) wrote…

extra '_'

Done.


Documentation/misc/features.md line 1547 at r1 (raw file):

Previously, llly (Li Xun) wrote…

identifiers

Done.


Documentation/misc/features.md line 1897 at r1 (raw file):

Previously, llly (Li Xun) wrote…

identifiers

Done.


Documentation/misc/features.md line 2062 at r1 (raw file):

Previously, llly (Li Xun) wrote…

supports

Done.

Copy link
Contributor

@llly llly left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners

Copy link
Contributor

@kailun-qin kailun-qin left a 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, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)


Documentation/misc/features.md line 1 at r3 (raw file):

# Gramine features

This is a great summary! But would still like to understand - what's the target audience? I would say it might be a bit technical for some. And if we do target different readers, we may add a section which can guide/point them to different sections quickly.

And why we put it in miscrather than somewhere looks to be more crucial?


Documentation/misc/features.md line 11 at r3 (raw file):

Gramine **intercepts all application requests** to the host OS. Some of these requests are processed
entirely inside Gramine, and some are forwarded to the host OS. Either way, each application's
request and each host's reply is verified for correctness and consistency. For these verifications,

Mention Iago attacks here?

Code quote:

request and each host's reply is verified for correctness and consistency.

Documentation/misc/features.md line 17 at r3 (raw file):

standards like POSIX ("bug-for-bug compatibility"). At the same time, Gramine is minimalistic, and
implements **only the most important subset of Linux functionality**, enough to run typical
cloud-native applications.

It's hard to classify which are "cloud-native" actually :).

Code quote:

cloud-native

Documentation/misc/features.md line 22 at r3 (raw file):

- **Linux features**: features can be (1) supported, (2) generally supported, (3) with limited
  support, or (4) not supported at all in Gramine. If the feature is not supported, we also specify

If we do categorize our feature support into these 4 levels, then for each feature below we should state its status by following these words.

Code quote:

- **Linux features**: features can be (1) supported, (2) generally supported, (3) with limited
  support, or (4) not supported at all in Gramine. If the feature is not supported, we also specify

Documentation/misc/features.md line 23 at r3 (raw file):

- **Linux features**: features can be (1) supported, (2) generally supported, (3) with limited
  support, or (4) not supported at all in Gramine. If the feature is not supported, we also specify
  whether there are plans to support it in the future (and if not, the rationale why not).

We in fact also documented CANs and CAN'Ts of the partially supported features.


Documentation/misc/features.md line 32 at r3 (raw file):

    not detected).

- **Gramine-specific features**: additional features, e.g., attestation primitives.

GSC? existless ocalls? security features (ASLR etc.)?

Support on debugging, profiling, pluggable signing?

We do not consider these as features?


Documentation/misc/features.md line 60 at r3 (raw file):

- **Linux kernel-to-userspace ABI**, in particular, two standards:

  - **System V ABI**: defines how applications invoke system calls and receive signals,

.

Code quote:

,

Documentation/misc/features.md line 63 at r3 (raw file):

  - **Executable and Linking Format (ELF)**: defines how applications are loaded from binary files.

<details><summary>Gramine also has internal APIs, used by Gramine developers</summary>

We mainly want to talk about Gramine features in this doc. Any special reason we mention internal APIs here? These terminologies seem also not be used in the following sections.

Code quote:

Gramine also has internal APIs, used by Gramine developers

Documentation/misc/features.md line 64 at r3 (raw file):

<details><summary>Gramine also has internal APIs, used by Gramine developers</summary>

We also basically manage the structure of the doc by grouping syscalls. That maybe why checkpoints and ELF loader etc. are not mentioned. Do we want to include these?


Documentation/misc/features.md line 72 at r3 (raw file):

  `lib_SHA256Init()`, `lib_AESGCMEncrypt()`, `lib_SSLRead()`, `lib_HKDF_SHA256()`.

- Data structures: doubly-linked lists, AVL trees, hash tables. Implemenations of lists and AVL

Implementations

Code quote:

Implemenations

Documentation/misc/features.md line 101 at r3 (raw file):

6.0](https://github.com/torvalds/linux/blob/v6.0/arch/x86/entry/syscalls/syscall_64.tbl).

<details><summary>:blue_book: List of all system calls in Gramine</summary>

It's actually the status of system call support in Gramine (as it's seems to be a full list whether or not we support it).

Code quote:

List of all system calls in Gramine

Documentation/misc/features.md line 1456 at r3 (raw file):

supports executing them as
[entrypoints](https://gramine.readthedocs.io/en/stable/manifest-syntax.html#libos-entrypoint) and
via `execve()` system call. In case of SGX backend, `execve()` execution replaces a calling program

We don't mention any currently supported or future to support backend in this doc.

Code quote:

backend

Documentation/misc/features.md line 1618 at r3 (raw file):

Gramine does *not* perform scheduling of threads, instead it relies on the host OS to perform
scheduling. In case of SGX backend, trying to perform or control scheduling would be futile because

The statement is for the current backends, right? Should we put the potential support (of scheduling) for other backends here (in this doc)? Maybe not, in that case, we may need add few others as plans e.g., memory management.

Code quote:

Gramine does *not* perform scheduling of threads, instead it relies on the host OS to perform
scheduling. In case of SGX backend, trying to perform or control scheduling would be futile because

Documentation/misc/features.md line 1664 at r3 (raw file):

### Memory synchronization (futexes)

Gramine largely implements futexes.

There are too many terminologies - largely, fully, generally, we should stick to the levels we defined in the starting section of this doc.

Code quote:

largely

Documentation/misc/features.md line 1786 at r3 (raw file):

- :x: Message queues: not implemented
- :x: Semaphores: not implemented
- :x: Shared memory: not implemented

Hmm, here we change the illustration of feature status into using legends.

Code quote:

- :white_check_mark: Signals and process state changes: implemented
- :white_check_mark: Pipes: implemented
- :white_check_mark: FIFOs (named pipes): implemented
- :ballot_box_with_check: UNIX domain sockets: implemented, with some limitations
- :ballot_box_with_check: File locking: partially implemented
- :x: Message queues: not implemented
- :x: Semaphores: not implemented
- :x: Shared memory: not implemented

Documentation/misc/features.md line 1836 at r3 (raw file):

Gramine implements signal dispositions, but some rarely used features are not implemented:
- core dump files are never produced,

is this rarely used?

Code quote:

core dump files are never produced,

Documentation/misc/features.md line 2030 at r3 (raw file):

</details>

### File system operations

Why do we put "File system operations", "File locking", "Monitoring filesystem events (inotify, fanotify)", "Hard links and soft links (symbolic links)" etc. at the same level of "File systems"?


Documentation/misc/features.md line 3187 at r3 (raw file):

Instead of performing a context switch from userland (ring-3) to kernelspace (ring-0), Gramine
relies on the system call being routed directly into Gramine process. There are two paths how the
application's system call requests ends up in Gramine emulation:

end

Code quote:

ends

Copy link
Author

@dimakuv dimakuv left a 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 1 files reviewed, 18 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @llly)


Documentation/misc/features.md line 1 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

This is a great summary! But would still like to understand - what's the target audience? I would say it might be a bit technical for some. And if we do target different readers, we may add a section which can guide/point them to different sections quickly.

And why we put it in miscrather than somewhere looks to be more crucial?

Done, both comments.


Documentation/misc/features.md line 11 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Mention Iago attacks here?

Done.


Documentation/misc/features.md line 17 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

It's hard to classify which are "cloud-native" actually :).

Done. I removed this word as it is heavily overused.

I used cloud-native term in the sense that is exemplified in "OS abstraction" item on this page: https://tanzu.vmware.com/cloud-native

Basically, Gramine targets applications that are portable (don't use super-fancy Linux features and can run even on older kernels) and hardware-independent (e.g., no special tricks with network card interfaces).


Documentation/misc/features.md line 22 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

If we do categorize our feature support into these 4 levels, then for each feature below we should state its status by following these words.

Yeah. I had this distinction between features and syscalls:

  • Features are supported/.../unsupported
  • Syscalls are implemented/.../unimplemented

But I can see how this is confusing. I'll just use a single word implemented everywhere (I find "implemented" better fitting for both features and syscalls). Done.


Documentation/misc/features.md line 23 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

We in fact also documented CANs and CAN'Ts of the partially supported features.

Done.


Documentation/misc/features.md line 32 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

GSC? existless ocalls? security features (ASLR etc.)?

Support on debugging, profiling, pluggable signing?

We do not consider these as features?

Done. I explained what I cover in this document.


Documentation/misc/features.md line 60 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

.

Done.


Documentation/misc/features.md line 63 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

We mainly want to talk about Gramine features in this doc. Any special reason we mention internal APIs here? These terminologies seem also not be used in the following sections.

I liked the idea of keeping this info somewhere. But you're right, let me just remove this.


Documentation/misc/features.md line 64 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

We also basically manage the structure of the doc by grouping syscalls. That maybe why checkpoints and ELF loader etc. are not mentioned. Do we want to include these?

No, I don't want to include internal details of Gramine.

Note that the ELF loading quirks I explain in the corresponding section. So I do include it.

But e.g. checkpointing is an internal detail of Gramine that gives zero information to a software engineer who wants to figure out whether Gramine is able to run his/her application. Checkpointing is fully transparent to the application (well, it may be buggy, but it's a different story).


Documentation/misc/features.md line 72 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Implementations

Not relevant anymore


Documentation/misc/features.md line 101 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

It's actually the status of system call support in Gramine (as it's seems to be a full list whether or not we support it).

Done.


Documentation/misc/features.md line 1456 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

We don't mention any currently supported or future to support backend in this doc.

What do you mean? That I need to add the info about Gramine backends in the intro? Ok, done.


Documentation/misc/features.md line 1618 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

The statement is for the current backends, right? Should we put the potential support (of scheduling) for other backends here (in this doc)? Maybe not, in that case, we may need add few others as plans e.g., memory management.

Good point. Let's not talk about such future possibilities for now. I would prefer to keep the descriptions to the minimum (the document is already huuuge).


Documentation/misc/features.md line 1664 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

There are too many terminologies - largely, fully, generally, we should stick to the levels we defined in the starting section of this doc.

Done.

  • largely implemented -> partially implemented
  • fully implemented -> implemented
  • generally implemented -> implemented

We lose some "shades of implemented" by using this rigid terminology, but I agree that it's better to have just 3 levels, instead of these vague descriptions.

I kept "fully" and "largely" words in couple places, where I use them in different context.


Documentation/misc/features.md line 1786 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Hmm, here we change the illustration of feature status into using legends.

Done, good catch. I wrote those "implemented"/"not implemented" phrases before I introduced the legend. Removed them now.


Documentation/misc/features.md line 1836 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

is this rarely used?

What's the exact question? I think core dump files are never used by normal users (only by developers during debug), and that's what I tried to convey in my phrasing.


Documentation/misc/features.md line 2030 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why do we put "File system operations", "File locking", "Monitoring filesystem events (inotify, fanotify)", "Hard links and soft links (symbolic links)" etc. at the same level of "File systems"?

Done. Good catch.


Documentation/misc/features.md line 3187 at r3 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

end

Done.

Copy link
Member

@mkow mkow left a 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 1 files reviewed, 18 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @llly)


Documentation/misc/features.md line 17 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. I removed this word as it is heavily overused.

I used cloud-native term in the sense that is exemplified in "OS abstraction" item on this page: https://tanzu.vmware.com/cloud-native

Basically, Gramine targets applications that are portable (don't use super-fancy Linux features and can run even on older kernels) and hardware-independent (e.g., no special tricks with network card interfaces).

+1 to Kailun, "cloud-native" is rather a marketing term than anything technical.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Pls kindly update the link to https://github.com/gramineproject/gramine/blob/dimakuv/add-gramine-features-doc/Documentation/devel/features.md.

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @llly)


Documentation/devel/features.md line 16 at r4 (raw file):

Gramine **intercepts all application requests** to the host OS. Some of these requests are processed
entirely inside Gramine, and some are forwarded to the host OS. Either way, each application's
request and each host's reply is verified for correctness and consistency. For these verifications,

are

Code quote:

is

Documentation/devel/features.md line 1464 at r4 (raw file):

Currently, Gramine does *not* fully support fork in multi-threaded applications. There is a known
bug in Gramine that if one thread is performing fork and another thread modifies the internal
Gramine state, the state may get corrupted (which may lead to failures).

Add a link to the tracking issue?

Code quote:

Currently, Gramine does *not* fully support fork in multi-threaded applications. There is a known
bug in Gramine that if one thread is performing fork and another thread modifies the internal
Gramine state, the state may get corrupted (which may lead to failures).

Documentation/devel/features.md line 2526 at r4 (raw file):

- `SO_REUSEADDR` (ignored, same as in Linux).

<details><summary>:speech_balloon: Note on named UDSes</summary>

construction?

Code quote:

speech_balloon

Documentation/devel/features.md line 3155 at r4 (raw file):

Report* and *SGX Quote* accordingly, in case of SGX backend) through the `/dev/attestation/`
pseudo-filesystem. Manipulating with the `/dev/attestation/` pseudo-files allows to program local
attestation and remote attestation flows. Additionally, the `dev/attestation/` pseudo-filesystem

Should be/dev/attestation/ (or more precisely /dev/attestation/keys/).

Code quote:

`dev/attestation/`

Documentation/devel/features.md line 3224 at r4 (raw file):

In case of SGX backend, Gramine needs a hint whether the first loaded binary is PIE or non-PIE. This
hint may be provided via the [`sgx.nonpie_binary` manifest
option](https://gramine.readthedocs.io/en/stable/manifest-syntax.html#non-pie-binaries).

I understand this is a living doc (w/ the last major update happened in Feb 2023). But we just removed this and it seemed useless.

Code quote:

In case of SGX backend, Gramine needs a hint whether the first loaded binary is PIE or non-PIE. This
hint may be provided via the [`sgx.nonpie_binary` manifest
option](https://gramine.readthedocs.io/en/stable/manifest-syntax.html#non-pie-binaries).

Documentation/misc/features.md line 64 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, I don't want to include internal details of Gramine.

Note that the ELF loading quirks I explain in the corresponding section. So I do include it.

But e.g. checkpointing is an internal detail of Gramine that gives zero information to a software engineer who wants to figure out whether Gramine is able to run his/her application. Checkpointing is fully transparent to the application (well, it may be buggy, but it's a different story).

Gotcha, thanks for the explanation!


Documentation/misc/features.md line 1456 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What do you mean? That I need to add the info about Gramine backends in the intro? Ok, done.

Yes, thanks!


Documentation/misc/features.md line 1664 at r3 (raw file):

We lose some "shades of implemented" by using this rigid terminology

;)


Documentation/misc/features.md line 1836 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What's the exact question? I think core dump files are never used by normal users (only by developers during debug), and that's what I tried to convey in my phrasing.

OK, thanks! I'm fine w/ it, resolving.

@dimakuv dimakuv force-pushed the dimakuv/add-gramine-features-doc branch 2 times, most recently from 4a7e036 to 46ab452 Compare March 2, 2023 14:40
Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r5.
Reviewable status: 6 of 7 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @llly)

a discussion (no related file):
Please add python3-recommonmark to Build-Depends and BuildRequires in debian/control and gramine.spec, respectively, this will fix CI for packaging pipelines.

Also, I thought about it a bit and I prefer that addition of recommonmark was a separate commit before the actual features.md, because if someone were to rewrite this to reST after all, then this separate commit would be easy to revert.



Documentation/index.rst line 95 at r5 (raw file):

   :maxdepth: 1

   devel/features

Are you sure this is for devel/ ("Developing Gramine")? I thought this document was for prospective user, evaluating if his/her app will run. If I got to choose, I'd place it just in Documentation/ (sphinx toplevel).

@dimakuv dimakuv force-pushed the dimakuv/add-gramine-features-doc branch from 46ab452 to 1caa64b Compare March 7, 2023 12:52
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

@kailun-qin I did it already? Maybe it was an old comment...

Reviewable status: 6 of 9 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin, @llly, and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Please add python3-recommonmark to Build-Depends and BuildRequires in debian/control and gramine.spec, respectively, this will fix CI for packaging pipelines.

Also, I thought about it a bit and I prefer that addition of recommonmark was a separate commit before the actual features.md, because if someone were to rewrite this to reST after all, then this separate commit would be easy to revert.

Done both. For the second part of the comment, I had to rebase (to re-arrange into two commits).



Documentation/index.rst line 95 at r5 (raw file):

I thought this document was for prospective user, evaluating if his/her app will run.

This document is for very technical users (it's practically impossible for a casual user to figure out whether Gramine will support some app). So it feels strange to put it as top-level document.

But maybe we should add a new section in ToC? I don't know. I will leave the doc as is for now, there is anyway a parallel effort to rearrange the docs.


Documentation/devel/features.md line 16 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

are

Done.


Documentation/devel/features.md line 1464 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Add a link to the tracking issue?

Done.


Documentation/devel/features.md line 2526 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

construction?

Not relevant anymore, I decided to remove these emojis/symbols all together, they are just complicating reading.


Documentation/devel/features.md line 3155 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Should be/dev/attestation/ (or more precisely /dev/attestation/keys/).

Done.


Documentation/devel/features.md line 3224 at r4 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

I understand this is a living doc (w/ the last major update happened in Feb 2023). But we just removed this and it seemed useless.

Done.

woju
woju previously approved these changes Mar 29, 2023
Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kailun-qin)


Documentation/index.rst line 95 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I thought this document was for prospective user, evaluating if his/her app will run.

This document is for very technical users (it's practically impossible for a casual user to figure out whether Gramine will support some app). So it feels strange to put it as top-level document.

But maybe we should add a new section in ToC? I don't know. I will leave the doc as is for now, there is anyway a parallel effort to rearrange the docs.

OK. Let's think about this during/after rearranging the docs.

Copy link

@monavij monavij left a 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, 5 unresolved discussions (waiting on @kailun-qin)


Documentation/index.rst line 95 at r5 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

OK. Let's think about this during/after rearranging the docs.

I think this doc does belong in "Developing Gramine" as its mainly useful for someone interested in details of Gramine internals. The list of system calls is potentially useful for some users, but we can probably add a little blurb on that to some top level page pointing to this document.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Maybe it was an old comment...

Yes, it was an old comment.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dimakuv)


Documentation/devel/features.md line 26 at r6 (raw file):

Gramine currently has two backends: direct execution on the host Linux OS (called `gramine-direct`)
and execution inside an Intel SGX enclave (called `gramine-sgx`). If some feature has quirks and
pecularities in some backend, we describe it explicitly. More backends are possible in the future.

peculiarities

Code quote:

pecularities

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 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: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @woju)


Documentation/devel/features.md line 26 at r6 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

peculiarities

Done.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r5, 2 of 3 files at r6, all commit messages.
Reviewable status: 8 of 9 files reviewed, 44 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, @monavij, and @woju)


-- commits line 6 at r6:

Suggestion:

reStructuredText

-- commits line 11 at r6:
I think there's no point to say this in the one-liner, it's an implementation detail. I'd move it to the message body.

Code quote:

 (in Markdown)

-- commits line 13 at r6:
ditto

Code quote:

RestructedText

Documentation/index.rst line 95 at r5 (raw file):

Previously, monavij (Mona Vij) wrote…

I think this doc does belong in "Developing Gramine" as its mainly useful for someone interested in details of Gramine internals. The list of system calls is potentially useful for some users, but we can probably add a little blurb on that to some top level page pointing to this document.

+1 to placing it under devel/.

One thing though: it IMO shouldn't be the first on the list, there are many more important documents for users which are reading this section (e.g. "Building") nor it seems to logically make sense to be the first entry.


Documentation/devel/features.md line 1 at r6 (raw file):

<!-- Cannot render this doc in RestructedText as it has no support for nested inline markup. -->

ditto

Code quote:

RestructedText

Documentation/devel/features.md line 5 at r6 (raw file):

# Gramine features

> ⚠ Highly technical document intended for software engineers with knowledge of OS kernels.

Suggestion:

This is a highly technical document

Documentation/devel/features.md line 9 at r6 (raw file):

> ⛏ This is a living document. The last major update happened in **March 2023**.

Gramine strives to **support native, unmodified Linux applications** on any platform. The SGX

(I think this explains better what we're actually doing)

Suggestion:

Gramine strives to **run

Documentation/devel/features.md line 14 at r6 (raw file):

Gramine intercepts all application requests to the host OS. Some of these requests are processed
entirely inside Gramine, and some are forwarded to the host OS.

I don't like this wording, it will make people think that forwarding things to the host is ok in Gramine. We aren't really forwarding things, we're implementing them based on the small API to the host which provides I/O operations. Sometimes this is close to how literal forwarding would look like, but it's always (I think) just implementing something using PAL API.

This is especially confusing because some SGX frameworks actually do forward syscalls to the host with some light filtering, but that is a completely different design than Gramine.


Documentation/devel/features.md line 24 at r6 (raw file):

hardware-independent applications.

Gramine currently has two backends: direct execution on the host Linux OS (called `gramine-direct`)

This sounds very misleading to me - sounds like in this mode we're just forwarding all syscalls directly to the host.

Code quote:

direct execution on the host Linux OS

Documentation/devel/features.md line 32 at r6 (raw file):

- **Linux features**: features can be (1) implemented, (2) partially implemented, or (3) not
  implemented at all in Gramine. If the feature is partially implemented, then we also document the
  parts that are implemented and the parts that are implemented. If the feature is not implemented

Suggestion:

parts that are implemented and the parts that are not implemented

Documentation/devel/features.md line 62 at r6 (raw file):

Similarly to Linux, Gramine provides two interfaces to user applications:

- **Linux kernel-to-userspace API**, consisting of two sub-interfaces:

Not userspace-to-kernel?

Code quote:

Linux kernel-to-userspace API

Documentation/devel/features.md line 64 at r6 (raw file):

- **Linux kernel-to-userspace API**, consisting of two sub-interfaces:

  - **Linux System Call Interface**: a set of system calls which allow applications to access system

This is actually ABI, not API. Maybe list instead interfaces and then standards below? This split between API and ABI here seems weird.


Documentation/devel/features.md line 81 at r6 (raw file):

- ☑ implemented (no serious limitations)
- ☐ partially implemented (serious limitations or quirks)

I find this empty box meaning "partial support" very confusing, I've read this legend but still got troubles when reading the list, which is mostly ☑ and ☐ interleaved - easy to forget that this is a tri-state box and the empty one doesn't actually mean "not implemented".
Any other ideas how could we mark partial support? Maybe one of ± ▣ ◫ ? (with the original ones potentially permuted)


Documentation/devel/features.md line 88 at r6 (raw file):

Gramine implements ~170 system calls out of ~360 system calls available on Linux. Many system calls
are implemented only partially, typically because real world workloads do not use the unimplemented
functionality (for example, `O_ASYNC` flag in `open()` is not used). Some system calls are not

What do you mean here? That it's not implemented in Gramine or not used widely?

Code quote:

for example, `O_ASYNC` flag in `open()` is not used

Documentation/devel/features.md line 103 at r6 (raw file):

  <sup>[1](#file-system-operations)</sup>
  <sup>[2](#pipes-and-fifos-named-pipes)</sup>
  <sup>[3](#tcpip-and-udpip-sockets)</sup>

This link doesn't work for me. Same for other places where the same anchor is used.


Documentation/devel/features.md line 141 at r6 (raw file):

  <sup>[3](#tcpip-and-udpip-sockets)</sup>
  <sup>[4](#unix-domain-sockets)</sup>
  <sup>[5](#io-multiplexing)</sup>

another broken link


Documentation/devel/features.md line 234 at r6 (raw file):

-`dup()`
  <sup>[1](#misc)</sup>

I don't like that the same numbers mean different things for each syscall. You can't know what they mean without clicking. If we'd keep the numbering consistent then you could remember what caveat each number represents.


Documentation/devel/features.md line 698 at r6 (raw file):

  <sup>[1](#advanced-unimplemented-features)</sup>

-`quotactl()`

missing space (it renders misaligned)


Documentation/devel/features.md line 780 at r6 (raw file):

-`io_setup()`
  <sup>[1](#asynchronous-io)</sup>

also broken


Documentation/devel/features.md line 1296 at r6 (raw file):

Only a subset of most widely used pseudo-files is implemented. The list of implemented pseudo-files
grows with time, as Gramine adds functionality required by real world workloads.

Suggestion:

real-world workloads

Documentation/devel/features.md line 1450 at r6 (raw file):

same Gramine instance.

Gramine can execute ELF binaries (executables and libraries) and scripts (aka shebangs). Gramine

"shebang" is the name for that #!<path> line in a script, not executable scripts. Executable scripts are just "executable scripts" ;)

Code quote:

scripts (aka shebangs)

Documentation/devel/features.md line 1452 at r6 (raw file):

Gramine can execute ELF binaries (executables and libraries) and scripts (aka shebangs). Gramine
supports executing them as
[entrypoints](https://gramine.readthedocs.io/en/stable/manifest-syntax.html#libos-entrypoint) and

Any reason why this is an external link? Ditto for other such places.

Code quote:

[entrypoints](https://gramine.readthedocs.io/en/stable/manifest-syntax.html#libos-entrypoint)

Documentation/devel/features.md line 1456 at r6 (raw file):

with a new program *in the same SGX enclave*.

Gramine supports creating child processes using `fork()`, `vfork()` and `clone(..no CLONE_VM..)`

But we do support CLONE_VM, just not all possible combinations with other flags.

Code quote:

`clone(..no CLONE_VM..)`

Documentation/devel/features.md line 1467 at r6 (raw file):

Gramine supports process termination using [...] exit_group() (even if multiple threads)

I don't understand this sentence, what do you mean? Even if multiple threads what?


Documentation/devel/features.md line 1482 at r6 (raw file):

-`execve()`
-`execveat()`: not used by applications
-`clone()`: except exotic combination `CLONE_VM & !CLONE_THREAD`

Not exactly, it works in this combination: CLONE_VM & !CLONE_THREAD & CLONE_VFORK.


Documentation/devel/features.md line 1496 at r6 (raw file):

- `LD_LIBRARY_PATH` environment variable is always propagated into new process, see
  https://github.com/gramineproject/graphene/issues/2081.

This link isn't formatted


Documentation/devel/features.md line 1506 at r7 (raw file):

Gramine implements per-thread:
- stack and signal (alternate) stack,

Hmm, this isn't true I think? It's app's libc implementing this, Gramine doesn't have control over it?


Documentation/devel/features.md line 1527 at r7 (raw file):

Gramine does not support dynamically-growing stacks (as Linux does)

Maybe I'm misremembering something, but I thought glibc doesn't grow its stack, only the first one grows, which was allocated by kernel.


Documentation/devel/features.md line 1656 at r7 (raw file):

-`sched_setattr()`: not used by applications
-`ioprio_get()`: not used by applications
-`ioprio_set()`: not used by applications

This is IMO slightly misleading, I'd rather say something like "very rarely used by applications" (because it is used, it's just quite exotic).

ditto for the whole document

Code quote:

not used by applications

Documentation/devel/features.md line 1710 at r7 (raw file):

`mprotect()` supports all flags except `PROT_SEM` and `PROT_GROWSUP`. We haven't encountered any
applications that would use these flags. In case of SGX backend, `mprotect()` behavior differs:
- on [EDMM-enabled systems](https://gramine.readthedocs.io/en/stable/sgx-intro.html#term-edmm),

ditto below

Suggestion:

on [systems supporting EDMM]

Documentation/devel/features.md line 1812 at r7 (raw file):

- For Linux IPC overview, we recommend reading [Beej's Guide to Unix
  IPC](https://beej.us/guide/bgipc/html/single/bgipc.html).

404

Code quote:

https://beej.us/guide/bgipc/html/single/bgipc.html

Documentation/devel/features.md line 1915 at r7 (raw file):

manifest options are not set, then all IDs are equal to zero, which means root user.

During execution, the application may modify these IDs, and the changes will be visible internally

I'm not sure if this sentence makes sense, how would this even work? The user/group IDs are emulated and exist only inside Gramine, the host theoretically may be even Windows where this concept doesn't exist at all.

Code quote:

During execution, the application may modify these IDs, and the changes will be visible internally
in Gramine but will *not* propagate to the host OS.

Documentation/devel/features.md line 1922 at r7 (raw file):

Gramine has dummy support for Supplementary group IDs. The corresponding system calls are
`getgroups()` and `setgroups()`. Gramine starts the applications with an empty set of supplementary

ditto

Code quote:

Gramine has dummy support for Supplementary group IDs. The corresponding system calls are
`getgroups()` and `setgroups()`. Gramine starts the applications with an empty set of supplementary
groups. The application may modify this set, and the changes will be visible internally in Gramine
but will *not* propagate to the host OS.

Documentation/devel/features.md line 1928 at r7 (raw file):

Currently, there are only two usages of user/group IDs in Gramine:
- changing ownership of a file via `chown()` and similar system calls;
- injecting SIGCHLD on terminated child processes.

How is this related to user IDs?

Code quote:

- injecting SIGCHLD on terminated child processes.

Documentation/devel/features.md line 1968 at r7 (raw file):

files: this is controlled by the manifest's [FS mount points
(`fs.mounts`)](https://gramine.readthedocs.io/en/stable/manifest-syntax.html#fs-mount-points). This
feature is similar to the *chroot* concept on Linux and *jail* concept on FreeBSD. This Gramine

Isn't this a bad analogy? We have our own emulated filesystem where you can optionally mount directories from the host, while chroots and jails are just a limited views on some other FS, mounted as /.
Gramine FS namespace is more like the filesystem in a docker container or in a VM, where you can optionally mount some selected directories from the host.

IMO we should describe Gramine everywhere as something conceptually similar to docker containers or VMs. Places like this hint the reader into thinking that we're just passing through syscalls to the host with some filtering (as a few other SGX frameworks do). And this is a popular misconception about Gramine, looking at the number of PRs where people add a new syscall by just passing it through to the host.

Code quote:

similar to the *chroot* concept on Linux and *jail* concept on FreeBSD

Documentation/devel/features.md line 1971 at r7 (raw file):

feature is introduced for security.

Another peculiarity is that Gramine provides several types of files:

Suggestion:

Gramine provides several types of filesystem mounts

Documentation/devel/features.md line 1972 at r7 (raw file):

Another peculiarity is that Gramine provides several types of files:
- plain files (unencrypted files, see below),

ditto in the same way for the whole section

Suggestion:

passthrough mounts

Documentation/devel/features.md line 1980 at r7 (raw file):

Additionally, files may be hosted in one of two ways:
- on the host OS (passthrough-to-host, in *chroot* mounts),

These are not literal passthroughs, see e.g. protected fs. Also, these are not "chroot mounts", I don't know who introduced this name to Gramine, but chroot works completely differently in Linux and is IMO very confusing for non-gramine-veterans reading this text.

Code quote:

- on the host OS (passthrough-to-host, in *chroot* mounts),

Documentation/devel/features.md line 2011 at r7 (raw file):

General FS limitations in Gramine include:
- no support for dynamic mounting: all mounts must be specified beforehand in the manifest;
- no operations across mounts (e.g. no rename of file located in one mount to another one);

Does this work on Linux?

Code quote:

- no operations across mounts (e.g. no rename of file located in one mount to another one);

Documentation/devel/features.md line 2024 at r7 (raw file):

- https://github.com/gramineproject/gramine/issues/12
- https://github.com/gramineproject/gramine/issues/584
- https://github.com/gramineproject/gramine/issues/578

These links aren't formatted.


Documentation/devel/features.md line 2037 at r7 (raw file):

Gramine) and `O_TMPFILE` (bug in Gramine: should not be silently ignored).

Trusted files can be opened only for read. Already-existing encrypted files can be opened only if

Suggestion:

Trusted files can be opened only for reading

Documentation/devel/features.md line 2080 at r7 (raw file):

Gramine has dummy support for file owner and group manipulations. In Gramine, users and groups are
dummy; see the ["User and group identifiers" section](#user-and-group-identifiers) for details.
Therefore, `chown()`, `fchownat()`, `fchown()` system calls updated UID and GID internally in

Past tense? A typo, I guess?

Code quote:

system calls updated UID and GID internally

Documentation/devel/features.md line 2081 at r7 (raw file):

dummy; see the ["User and group identifiers" section](#user-and-group-identifiers) for details.
Therefore, `chown()`, `fchownat()`, `fchown()` system calls updated UID and GID internally in
Gramine, but do not propagate these changes to the host.

ditto

Code quote:

do not propagate these changes to the host

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 53 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @monavij)


Documentation/devel/features.md line 2231 at r7 (raw file):

  -`F_GETLK`: see notes above

-`flock()`: may be implemented in future

Suggestion:

in the future

Documentation/devel/features.md line 2256 at r7 (raw file):

1. Host OS's links: Gramine sees them as normal files. These links are currently always followed
   during directory/file lookup.

Ditto, as above. This depends both on the host and the mount type, it sounds like here you silently assumed allowed_files mount with the host being Linux?


Documentation/devel/features.md line 2422 at r7 (raw file):

<details><summary>Note on domain names configuration</summary>

- To use name-resolving Berkeley socket APIs like `gethostbyname()`, `gethostbyaddr()`,

Suggestion:

To use libc name-resolving Berkeley socket APIs like

Documentation/devel/features.md line 2530 at r7 (raw file):

- There is an effort to make named UDSes visible on the Gramine filesystem, see
  https://github.com/gramineproject/gramine/pull/1021.

not clickable


Documentation/devel/features.md line 2666 at r7 (raw file):

POSIX semaphores are technically not a Linux kernel API. Instead, they are implemented on top of the
POSIX shared memory functionality of Linux (i.e., via `/dev/shm` pseudo-filesystem).

Suggestion:

POSIX shared memory functionality of Linux by libc

Documentation/devel/features.md line 3036 at r7 (raw file):

</details><br />

### Advanced (unimplemented) features

(some of them aren't really advanced, just infeasible to implement in Gramine)

Suggestion:

### Advanced/infeasible, unimplemented features

Documentation/devel/features.md line 3182 at r7 (raw file):

> ⚠ Below description assumes x86-64 architecture.

Gramine implements the system-call entry point (analogous to the `SYSCALL` x86 instruction).

I don't understand this reference - how our entrypoint code can be similar to an x86 instruction?

Code quote:

system-call entry point (analogous to the `SYSCALL` x86 instruction)

Documentation/devel/features.md line 3217 at r7 (raw file):

## Notes on application loading

Gramine can execute only ELF binaries (executables and libraries) and scripts (aka shebangs). Other

ditto

Code quote:

scripts (aka shebangs)

Documentation/devel/features.md line 3220 at r7 (raw file):

formats are not supported.

Gramine does *not* perform any dynamic linking during binary loading. Instead it just executes load

What's the point of this note? Linux does exactly the same.

Code quote:

Gramine does *not* perform any dynamic linking during binary loading.

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 9 files reviewed, 53 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, @mkow, and @woju)


-- commits line 6 at r6:
Will do during final rebase


-- commits line 11 at r6:

Previously, mkow (Michał Kowalczyk) wrote…

I think there's no point to say this in the one-liner, it's an implementation detail. I'd move it to the message body.

Will do during final rebase


-- commits line 13 at r6:

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Will do during final rebase


Documentation/index.rst line 95 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

+1 to placing it under devel/.

One thing though: it IMO shouldn't be the first on the list, there are many more important documents for users which are reading this section (e.g. "Building") nor it seems to logically make sense to be the first entry.

Done. Didn't know where to put exactly, so it seemed logical to me to move to the very end.


Documentation/devel/features.md line 1 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.


Documentation/devel/features.md line 5 at r6 (raw file):

# Gramine features

> ⚠ Highly technical document intended for software engineers with knowledge of OS kernels.

Done.


Documentation/devel/features.md line 9 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

(I think this explains better what we're actually doing)

Done.


Documentation/devel/features.md line 14 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Gramine intercepts all application requests to the host OS. Some of these requests are processed
entirely inside Gramine, and some are forwarded to the host OS.

I don't like this wording, it will make people think that forwarding things to the host is ok in Gramine. We aren't really forwarding things, we're implementing them based on the small API to the host which provides I/O operations. Sometimes this is close to how literal forwarding would look like, but it's always (I think) just implementing something using PAL API.

This is especially confusing because some SGX frameworks actually do forward syscalls to the host with some light filtering, but that is a completely different design than Gramine.

Done. What about the new wording, with a fancy funnel through?


Documentation/devel/features.md line 24 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This sounds very misleading to me - sounds like in this mode we're just forwarding all syscalls directly to the host.

Done. Couldn't come up with anything smart, so just deleted the word direct


Documentation/devel/features.md line 32 at r6 (raw file):

- **Linux features**: features can be (1) implemented, (2) partially implemented, or (3) not
  implemented at all in Gramine. If the feature is partially implemented, then we also document the
  parts that are implemented and the parts that are implemented. If the feature is not implemented

Done.


Documentation/devel/features.md line 62 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Not userspace-to-kernel?

Done. Lol, nobody noticed this blooper till now


Documentation/devel/features.md line 64 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This is actually ABI, not API. Maybe list instead interfaces and then standards below? This split between API and ABI here seems weird.

Done. I didn't intend to compare API vs ABI, that was just bad phrasing


Documentation/devel/features.md line 81 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I find this empty box meaning "partial support" very confusing, I've read this legend but still got troubles when reading the list, which is mostly ☑ and ☐ interleaved - easy to forget that this is a tri-state box and the empty one doesn't actually mean "not implemented".
Any other ideas how could we mark partial support? Maybe one of ± ▣ ◫ ? (with the original ones potentially permuted)

Done. I like ▣.


Documentation/devel/features.md line 88 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What do you mean here? That it's not implemented in Gramine or not used widely?

Done.


Documentation/devel/features.md line 103 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This link doesn't work for me. Same for other places where the same anchor is used.

Done. Some other anchors were also wrong, fixed now (checked in RST page)


Documentation/devel/features.md line 141 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

another broken link

Done.


Documentation/devel/features.md line 234 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't like that the same numbers mean different things for each syscall. You can't know what they mean without clicking. If we'd keep the numbering consistent then you could remember what caveat each number represents.

Oh my. Do you really want this? I can do it, but it feels like a lot of manual work for small gains.


Documentation/devel/features.md line 698 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

missing space (it renders misaligned)

Done.


Documentation/devel/features.md line 780 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

also broken

Done.


Documentation/devel/features.md line 1296 at r6 (raw file):

Only a subset of most widely used pseudo-files is implemented. The list of implemented pseudo-files
grows with time, as Gramine adds functionality required by real world workloads.

Done.


Documentation/devel/features.md line 1450 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

"shebang" is the name for that #!<path> line in a script, not executable scripts. Executable scripts are just "executable scripts" ;)

Done.


Documentation/devel/features.md line 1452 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Any reason why this is an external link? Ditto for other such places.

What should be the correct way? I'm not sure how to add Markdown links to sections in a relative RST file...


Documentation/devel/features.md line 1456 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But we do support CLONE_VM, just not all possible combinations with other flags.

This is in the context of Processes (I have a similar note on clone() in the section about Threads). But I see that it is misleading... How can I make it obvious?


Documentation/devel/features.md line 1467 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Gramine supports process termination using [...] exit_group() (even if multiple threads)

I don't understand this sentence, what do you mean? Even if multiple threads what?

Done. Removed the parentheses, I have no idea what I meant there.


Documentation/devel/features.md line 1482 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Not exactly, it works in this combination: CLONE_VM & !CLONE_THREAD & CLONE_VFORK.

Done. What about now?


Documentation/devel/features.md line 1496 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This link isn't formatted

Done.


Documentation/devel/features.md line 1506 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, this isn't true I think? It's app's libc implementing this, Gramine doesn't have control over it?

Yes, libc allocates the stacks (other than the main thread's stack) and passes them to e.g. clone(), so Gramine does not manage the lifetime of the child-threads' stacks.

But what I meant is that Gramine is fully aware of stacks and signal stacks. How can I reformulate?


Documentation/devel/features.md line 1527 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Gramine does not support dynamically-growing stacks (as Linux does)

Maybe I'm misremembering something, but I thought glibc doesn't grow its stack, only the first one grows, which was allocated by kernel.

Yes, that's what I meant -- only the main stack.

I rephrased.


Documentation/devel/features.md line 1656 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This is IMO slightly misleading, I'd rather say something like "very rarely used by applications" (because it is used, it's just quite exotic).

ditto for the whole document

Done.


Documentation/devel/features.md line 1710 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto below

Done.


Documentation/devel/features.md line 1812 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

404

Done. Pretty cool, they changed the whole website in February this year.


Documentation/devel/features.md line 1915 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'm not sure if this sentence makes sense, how would this even work? The user/group IDs are emulated and exist only inside Gramine, the host theoretically may be even Windows where this concept doesn't exist at all.

Done. Does this read better?


Documentation/devel/features.md line 1922 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.


Documentation/devel/features.md line 1928 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

How is this related to user IDs?

I mean these:

This is simply for siginfo_t::si_uid, see https://man7.org/linux/man-pages/man2/sigaction.2.html. How should I rephrase?


Documentation/devel/features.md line 1968 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Isn't this a bad analogy? We have our own emulated filesystem where you can optionally mount directories from the host, while chroots and jails are just a limited views on some other FS, mounted as /.
Gramine FS namespace is more like the filesystem in a docker container or in a VM, where you can optionally mount some selected directories from the host.

IMO we should describe Gramine everywhere as something conceptually similar to docker containers or VMs. Places like this hint the reader into thinking that we're just passing through syscalls to the host with some filtering (as a few other SGX frameworks do). And this is a popular misconception about Gramine, looking at the number of PRs where people add a new syscall by just passing it through to the host.

Done.


Documentation/devel/features.md line 1971 at r7 (raw file):

feature is introduced for security.

Another peculiarity is that Gramine provides several types of files:

Done.


Documentation/devel/features.md line 1972 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto in the same way for the whole section

Done.


Documentation/devel/features.md line 1980 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

These are not literal passthroughs, see e.g. protected fs. Also, these are not "chroot mounts", I don't know who introduced this name to Gramine, but chroot works completely differently in Linux and is IMO very confusing for non-gramine-veterans reading this text.

Done.


Documentation/devel/features.md line 2011 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Does this work on Linux?

This doesn't work on Linux too, yep. Just wanted to highlight that this is impossible. And because in Gramine mounts are typically more fine-grained, it seems important to mention it here.


Documentation/devel/features.md line 2024 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

These links aren't formatted.

Done.


Documentation/devel/features.md line 2037 at r7 (raw file):

Gramine) and `O_TMPFILE` (bug in Gramine: should not be silently ignored).

Trusted files can be opened only for read. Already-existing encrypted files can be opened only if

Done.


Documentation/devel/features.md line 2080 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Past tense? A typo, I guess?

Done.


Documentation/devel/features.md line 2081 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done. I still want to mention that the files on the host do not change their owner/group.


Documentation/devel/features.md line 2231 at r7 (raw file):

  -`F_GETLK`: see notes above

-`flock()`: may be implemented in future

Done.


Documentation/devel/features.md line 2256 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Ditto, as above. This depends both on the host and the mount type, it sounds like here you silently assumed allowed_files mount with the host being Linux?

Done. I mention Linux now, but I don't get your comment about allowed_files -- trusted files are also just followed. How could I rephrase?


Documentation/devel/features.md line 2422 at r7 (raw file):

<details><summary>Note on domain names configuration</summary>

- To use name-resolving Berkeley socket APIs like `gethostbyname()`, `gethostbyaddr()`,

Done.


Documentation/devel/features.md line 2530 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

not clickable

Done.


Documentation/devel/features.md line 2666 at r7 (raw file):

POSIX semaphores are technically not a Linux kernel API. Instead, they are implemented on top of the
POSIX shared memory functionality of Linux (i.e., via `/dev/shm` pseudo-filesystem).

Done.


Documentation/devel/features.md line 3036 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

(some of them aren't really advanced, just infeasible to implement in Gramine)

Done.


Documentation/devel/features.md line 3182 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't understand this reference - how our entrypoint code can be similar to an x86 instruction?

Does ABI help?


Documentation/devel/features.md line 3217 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.


Documentation/devel/features.md line 3220 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What's the point of this note? Linux does exactly the same.

Yeah, just wanted to be explicit. But you're right.

Copy link
Member

@mkow mkow left a 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 r8, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)


Documentation/devel/features.md line 1 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

You missed one more typo, look at the suggested diff in a discussion above :)


Documentation/devel/features.md line 14 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. What about the new wording, with a fancy funnel through?

I like the new phrasing :)


Documentation/devel/features.md line 64 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. I didn't intend to compare API vs ABI, that was just bad phrasing

Aaah, I see, now I understand this split.


Documentation/devel/features.md line 234 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Oh my. Do you really want this? I can do it, but it feels like a lot of manual work for small gains.

Why manual? Just find & replace on the whole lines?


Documentation/devel/features.md line 1452 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What should be the correct way? I'm not sure how to add Markdown links to sections in a relative RST file...

The current way is really problematic, e.g. this document viewed at latest will now still link to stable. And rendered locally it links remotely. And many more issues...

Won't changing this to a relative link work?


Documentation/devel/features.md line 1456 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is in the context of Processes (I have a similar note on clone() in the section about Threads). But I see that it is misleading... How can I make it obvious?

Maybe just leave clone()?


Documentation/devel/features.md line 1506 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, libc allocates the stacks (other than the main thread's stack) and passes them to e.g. clone(), so Gramine does not manage the lifetime of the child-threads' stacks.

But what I meant is that Gramine is fully aware of stacks and signal stacks. How can I reformulate?

But it's not aware of them, the only thing we know is what value to put into RSP when calling signal handlers? So, it's IMO very misleading when you say "Gramine implements [...] stack and signal (alternate) stack".
I'd just replace this point with "saved information about signal (alternate) stack".


Documentation/devel/features.md line 1812 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Pretty cool, they changed the whole website in February this year.

lol


Documentation/devel/features.md line 1915 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Does this read better?

Yup, now it's good.


Documentation/devel/features.md line 1928 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I mean these:

This is simply for siginfo_t::si_uid, see https://man7.org/linux/man-pages/man2/sigaction.2.html. How should I rephrase?

Hmm, so it's not used for injecting SIGCHLD, it's just passed to the user app receiving the signal as additional chunk of information? Or am I missing something? The current wording ("usages of user/group IDs in Gramine: [...] injecting SIGCHLD on terminated child processes" sounds like this user ID was somehow used to deliver the signal itself.


Documentation/devel/features.md line 2256 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. I mention Linux now, but I don't get your comment about allowed_files -- trusted files are also just followed. How could I rephrase?

I think it's good now, resolving.


Documentation/devel/features.md line 1979 at r8 (raw file):

In case of SGX backend, passthrough mounts must be of one of two kinds:
- containing allowed files (insecure, not protected in any way, only for testing purposes),
- containing trusted files (secure, cryptographically hashed).

I wouldn't say secure/insecure, security is always relative to a threat model/app design. Allowed mounts may be secure if the app does the encryption by itself or the contents there doesn't matter for security. Same for trusted files, they may be insecure if you store some secrets there :)


Documentation/devel/features.md line 1979 at r8 (raw file):

In case of SGX backend, passthrough mounts must be of one of two kinds:
- containing allowed files (insecure, not protected in any way, only for testing purposes),
- containing trusted files (secure, cryptographically hashed).

Suggestion:

cryptographically hashed - effectively, their contents are mixed into MRENCLAVE on SGX

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


Documentation/devel/features.md line 1 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

You missed one more typo, look at the suggested diff in a discussion above :)

Done.


Documentation/devel/features.md line 234 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why manual? Just find & replace on the whole lines?

Done.


Documentation/devel/features.md line 1452 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

The current way is really problematic, e.g. this document viewed at latest will now still link to stable. And rendered locally it links remotely. And many more issues...

Won't changing this to a relative link work?

Now that I dealt with your other comments, let me try this...


Documentation/devel/features.md line 1456 at r6 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Maybe just leave clone()?

Done.


Documentation/devel/features.md line 1506 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But it's not aware of them, the only thing we know is what value to put into RSP when calling signal handlers? So, it's IMO very misleading when you say "Gramine implements [...] stack and signal (alternate) stack".
I'd just replace this point with "saved information about signal (alternate) stack".

Done. Not sure why you had saved word in your proposal, so just removed it.


Documentation/devel/features.md line 1928 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, so it's not used for injecting SIGCHLD, it's just passed to the user app receiving the signal as additional chunk of information? Or am I missing something? The current wording ("usages of user/group IDs in Gramine: [...] injecting SIGCHLD on terminated child processes" sounds like this user ID was somehow used to deliver the signal itself.

Done. Your understanding is correct.


Documentation/devel/features.md line 1979 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I wouldn't say secure/insecure, security is always relative to a threat model/app design. Allowed mounts may be secure if the app does the encryption by itself or the contents there doesn't matter for security. Same for trusted files, they may be insecure if you store some secrets there :)

Done.


Documentation/devel/features.md line 1979 at r8 (raw file):

In case of SGX backend, passthrough mounts must be of one of two kinds:
- containing allowed files (insecure, not protected in any way, only for testing purposes),
- containing trusted files (secure, cryptographically hashed).

Done.

@dimakuv dimakuv force-pushed the dimakuv/add-gramine-features-doc branch 2 times, most recently from 1e5b44f to a36f456 Compare April 5, 2023 12:49
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


Documentation/devel/features.md line 1452 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Now that I dealt with your other comments, let me try this...

Done. Seems to be working.

@dimakuv dimakuv force-pushed the dimakuv/add-gramine-features-doc branch from a36f456 to 82c8100 Compare April 5, 2023 13:45
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


Documentation/devel/features.md line 1988 at r11 (raw file):

All files potentially used by the application must be specified in the manifest file. Instead of
single files, whole directories can be specified. Refer to the [manifest documentation for more
details](../manifest-syntax.html#manifest-syntax).

Relative linking is super-brittle in Recommonmark (that we use as a Sphinx plugin in our docs), see this: readthedocs/recommonmark#130 (comment)

That's why I added a dummy anchor here, instead of just the manifest-syntax.html target.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)


Documentation/devel/features.md line 1506 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Not sure why you had saved word in your proposal, so just removed it.

I wanted to stress that it's just one value saved once and that's all, i.e. we aren't doing anything active with these stacks. But that's not that important with the new wording.


Documentation/devel/features.md line 1928 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Your understanding is correct.

Now it's clear to me what this point means, thanks!


Documentation/devel/features.md line 104 at r11 (raw file):

-`read()`
  <sup>[9.1](#file-system-operations)</sup>

Hmm, I don't like these sub-numbers, it's rather hard to read because these dots are pretty small when rendered. Maybe just use consecutive numbers?
What was the idea behind using them? To group similar issues together?

Code quote:

9.1

Copy link
Author

@dimakuv dimakuv left a 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, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


Documentation/devel/features.md line 104 at r11 (raw file):

What was the idea behind using them? To group similar issues together?

Yes. Because e.g. 9.1, 9.2, etc are all subsections of one section "File Systems" (see the sections on the left pane in the rendered doc: https://gramine.readthedocs.io/en/dimakuv-add-gramine-features-doc/devel/features.html).

I could use consecutive numbers, but then it won't map nicely to Sections - Subsections in the document. Well, there are already some exceptions in the current mapping (e.g. Attestation section is marked consequtively even though it is located under Gramine-specific features). So whatever, I wouldn't mind changing 9.1 to 9, 9.2 to 10, etc, and shifting all other numbers too.

Alternatively, if the problem is in the dot only, I can suggest the following alternatives:

  • 9-1, 9-2, etc
  • 9(1), 9(2), etc

Copy link
Member

@mkow mkow left a 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, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)


Documentation/devel/features.md line 104 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What was the idea behind using them? To group similar issues together?

Yes. Because e.g. 9.1, 9.2, etc are all subsections of one section "File Systems" (see the sections on the left pane in the rendered doc: https://gramine.readthedocs.io/en/dimakuv-add-gramine-features-doc/devel/features.html).

I could use consecutive numbers, but then it won't map nicely to Sections - Subsections in the document. Well, there are already some exceptions in the current mapping (e.g. Attestation section is marked consequtively even though it is located under Gramine-specific features). So whatever, I wouldn't mind changing 9.1 to 9, 9.2 to 10, etc, and shifting all other numbers too.

Alternatively, if the problem is in the dot only, I can suggest the following alternatives:

  • 9-1, 9-2, etc
  • 9(1), 9(2), etc

Hmm, maybe 9a, 9b, ...? That should be readable.

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 9 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


Documentation/devel/features.md line 104 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, maybe 9a, 9b, ...? That should be readable.

Done.

mkow
mkow previously approved these changes Apr 6, 2023
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)

Dmitrii Kuvaiskii added 2 commits April 11, 2023 00:12
Some documentation is hard to impossible to write properly in
reStructuredText. This commit adds support for Markdown format in the
documentation and our CI.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
It's unclear how to render this document in reStructuredText as it seems
to not support nested inline markup. So the document is written in
Markdown format.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
@dimakuv dimakuv force-pushed the dimakuv/add-gramine-features-doc branch from 9a38bc6 to d51ff41 Compare April 11, 2023 07:18
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 9 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin, @mkow, and @woju)


-- commits line 6 at r6:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during final rebase

Done.


-- commits line 11 at r6:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during final rebase

Done.


-- commits line 13 at r6:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during final rebase

Done.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kailun-qin)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv dimakuv merged commit d51ff41 into master Apr 11, 2023
@dimakuv dimakuv deleted the dimakuv/add-gramine-features-doc branch April 11, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants