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

implement seccomp notify #384

Merged
merged 15 commits into from
Oct 18, 2021
Merged

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented Oct 14, 2021

  1. Implemented seccomp notify listener logic based on the oci spec.
  2. Because seccomp notify listener needs to know the container init process id, I have to make intermediate process to return init process pid right after forking the init process. After that, the intermediate process will return and the init process will sync with the main process directly.

Note: I just realize while implementing this PR that seccomp notify is a fairly new thing that is less than a year old. seccomp v2.5.2 was released Aug 2021 and still includes changes to the seccomp notify. Likely there will be cases where we need a finer grained control over seccomp api level, which is not implemented in the PR. Most of the time, seccomp listener and notify features are not invoked, so this PR code path won't get triggered. I will iron out these kinks with follow up PRs.

Fix #307

@yihuaf yihuaf marked this pull request as draft October 14, 2021 01:28
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2021

Codecov Report

Merging #384 (aa78c74) into main (36df0a1) will decrease coverage by 1.08%.
The diff coverage is 11.36%.

@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
- Coverage   76.92%   75.83%   -1.09%     
==========================================
  Files          52       52              
  Lines        8280     8402     +122     
==========================================
+ Hits         6369     6372       +3     
- Misses       1911     2030     +119     

@yihuaf yihuaf changed the title [WIP] implement seccomp notify implement seccomp notify Oct 17, 2021
@yihuaf yihuaf marked this pull request as ready for review October 17, 2021 03:14
@utam0k
Copy link
Member

utam0k commented Oct 17, 2021

@yihuaf Cool!
Can I ask you to update docs/.draw.io.svg and add an integration test about seccomp notify?

yihuaf and others added 2 commits October 17, 2021 17:04
@yihuaf
Copy link
Collaborator Author

yihuaf commented Oct 18, 2021

I updated the drawio.svg.

add an integration test about seccomp notify?

It is probably easier to do a follow up PR. Likely I have to port over "seccompagent" for the testing:
ref: https://github.com/opencontainers/runc/tree/master/contrib/cmd/seccompagent

@utam0k
Copy link
Member

utam0k commented Oct 18, 2021

@yihuaf Thanks for your update. I think it would be worthwhile to add the interactions between each process about seccomp/seccomp notify to the sequence diagram. What do you think?

Is it difficult to add this into youki's original integration test that we are working on? I have no problem at all with another PR.

It is probably easier to do a follow up PR. Likely I have to port over "seccompagent" for the testing:
ref: https://github.com/opencontainers/runc/tree/master/contrib/cmd/seccompagent

@yihuaf
Copy link
Collaborator Author

yihuaf commented Oct 18, 2021

I can add the seccomp to the diagram.

In terms of integration test, there are two layers for seccomp notify. Youki itself doesn't do much other than sending a fd through a unix domain socket to a process outside Youki, which we can certainly test. If we want to actually test the functionality of seccomp notify, we need some process (aka seccomp agent) to listen on the seccomp notify socket and approve syscalls. That is a bit more complicated to automate, but shouldn't be any issue.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Awesome

@utam0k utam0k merged commit 2a12d40 into containers:main Oct 18, 2021
@yihuaf yihuaf deleted the yihuaf/seccomp_notify branch May 15, 2023 03:14
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.

support for seccomp notify
3 participants