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: update enforcement page #1630

Merged
merged 1 commit into from
Oct 27, 2023
Merged

docs: update enforcement page #1630

merged 1 commit into from
Oct 27, 2023

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Oct 20, 2023

No description provided.

@kkourt kkourt marked this pull request as ready for review October 20, 2023 13:48
@kkourt kkourt requested review from mtardy and a team as code owners October 20, 2023 13:48
@kkourt kkourt added the release-note/docs This PR updates the documentation. label Oct 20, 2023
Comment on lines +18 to +22
Override the return value of a call means that the function will never be executed and, instead, a
value (typically an error) will be returned to the caller. Generally speaking, only system calls and
security check functions allow to change their return value in this manner. Details about how users
can configure tracing policies to override the return value can be found in the [Override
action]({{< ref "/docs/concepts/tracing-policy/selectors#override-action" >}}) documentation.
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention the kernel option? https://elixir.bootlin.com/linux/v5.13.18/source/kernel/trace/Kconfig#L601. Not really a review of this but maybe for another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could have a Requirements section at some point would be another option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the option would be better suited in the Override action documentation (in the selectors) section.

Comment on lines 30 to 31
performed by the process from being executed. For example, a `SIGKILL` might be send in a `write()`
system call does not guarantee that the data will not be written to the file.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
performed by the process from being executed. For example, a `SIGKILL` might be send in a `write()`
system call does not guarantee that the data will not be written to the file.
performed by the process from being executed. For example, a `SIGKILL` sent in a `write()`
system call does not guarantee that the data will not be written to the file.

Copy link
Contributor

@jrfastab jrfastab Oct 20, 2023

Choose a reason for hiding this comment

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

Can we highlight what it does guarantee though. Maybe,

In contrast with overriding the return value, sending a
signal does not always stop the operation being performed
 by the process that triggered the operation. However, it does 
ensure that the process is terminated synchronously (and any 
threads will be stopped). To ensure the operation is also 
stopped the hook must be placed in the kernel carefully 
to ensure the kernel performs a check for any signals before
 the operation is performed.

In many cases it is sufficient to ensure the process is stopped 
and unable to return from the call. Further, the kernel often 
checks signals before doing I/O operations so these can be 
used as long as kernel code paths are audited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! As discussed offline, I pushed the following:

In contrast with overriding the return value, sending a `SIGKILL` signal does not always stop the
operation being performed by the process that triggered the operation. For example, a `SIGKILL` sent
in a `write()` system call does not guarantee that the data will not be written to the file.
However, it does ensure that the process is terminated synchronously (and any threads will be
stopped). In some cases it may be sufficient to ensure the process is stopped and the process does
not handle the return of the call. To ensure the operation is not completed, though, the `Signal`
action should be combined with the `Override` action.

@mtardy
Copy link
Member

mtardy commented Oct 20, 2023

that's weird, the netlify preview was not triggered! the only time we need this it's not triggered 🤔

EDIT: we have it for the other PR that was closed #1629. This thing puzzles me

@kkourt kkourt force-pushed the pr/kkourt/docs-enforcement branch from 2cf4476 to ac240d8 Compare October 23, 2023 06:46
@netlify
Copy link

netlify bot commented Oct 23, 2023

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 424d22d
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/6536173f5ee146000812127e
😎 Deploy Preview https://deploy-preview-1630--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kkourt kkourt force-pushed the pr/kkourt/docs-enforcement branch from ac240d8 to 3d20be9 Compare October 23, 2023 06:47
Signed-off-by: Kornilios Kourtis <[email protected]>
@kkourt kkourt force-pushed the pr/kkourt/docs-enforcement branch from 3d20be9 to 424d22d Compare October 23, 2023 06:48
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

I think we can merge this!

@kkourt kkourt merged commit cd215f2 into main Oct 27, 2023
@kkourt kkourt deleted the pr/kkourt/docs-enforcement branch October 27, 2023 15:19
@kkourt
Copy link
Contributor Author

kkourt commented Oct 27, 2023

Merged. Let's improve later if need be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/docs This PR updates the documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants