Implement json frontend #9

alindima · 2021-08-16T10:46:18Z

Adds the feature-guarded JSON frontend for compiling BPF filters.

andreeaflorescu

Review part 1. I just went through the first 3 commits. Will continue the review in the next couple of days.

tools/generate_syscall_tables.sh

src/syscall_table/aarch64.rs

src/syscall_table/mod.rs

tools/generate_syscall_tables.sh

andreeaflorescu · 2021-08-24T13:36:40Z

README.md

@@ -256,7 +256,7 @@ let filters: BpfMap = seccompiler::compile_from_json(
 categories to BPF programs.

 ```rust
-pub type BpfMap = HashMap<String, Arc<BpfProgram>>;
+pub type BpfMap = HashMap<String, BpfProgram>;


nit: can you add some details about what is changed in the documentation.

This type was not exported beforehand, it's introduced by this PR.
It was using an Arc in the beginning as a leftover from the use case we have in Firecracker.
But it's actually an implementation detail that needs to be handled in Firecracker (i.e. Map the BpfProgram to an Arc), since it's usecase-specific.

I meant to add some details in the commit message, sorry about that.

Cargo.toml

andreeaflorescu · 2021-08-24T13:43:43Z

src/backend/mod.rs

@@ -26,7 +27,7 @@ use bpf::{
 pub use bpf::{sock_filter, BpfProgram, BpfProgramRef};

 /// Backend Result type.
-pub type Result<T> = std::result::Result<T, Error>;


Why is this now private?

Well it was not required to be public.
Users of the library will not interact directly with the Backend and therefore do not need access to this type.
I think there should only be one Result type exported and that is the Result from src/lib.rs

Cargo.toml

andreeaflorescu · 2021-08-24T13:45:27Z

src/backend/mod.rs

@@ -101,7 +102,8 @@ impl TryFrom<&str> for TargetArch {
 }

 /// Comparison to perform when matching a condition.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Deserialize)]
+#[serde(rename_all = "snake_case")]


Why? Can you also add the response to this question as a comment in the code?

well it's just more idiomatic for JSON to name properties in snake_case format. That's the only reason, syntactic sugar. Do you still think it's worth adding this as a comment?

src/frontend/json.rs

andreeaflorescu · 2021-08-25T09:46:47Z

src/frontend/json.rs

+#[serde(deny_unknown_fields)]
+pub(crate) struct JsonCondition {
+    /// Index of the argument that is to be compared.
+    #[serde(rename = "index")]


What is the reason for the renames?

In the JSON filter, a sycall rule object uses an args property for specifying a condition vector.
Therefore, the arg_* names in this case are a little redundant, since it's obvious that they refer to arguments.

For the other fields, they are usually abbreviations so that the filters are easier to read&write.

src/frontend/json.rs

tools/generate_syscall_tables.sh

src/frontend/json.rs

src/syscall_table/aarch64.rs

tests/json.rs

andreeaflorescu · 2021-08-25T13:25:24Z

.buildkite/pipeline.yml

+
+  - label: "build-musl-arm-json"
+    commands:
+     - cargo build --release --features=json --target aarch64-unknown-linux-musl


We should also add a cargo check with the json feature on both platforms.

well this is already done here, right? https://github.com/rust-vmm/rust-vmm-ci/blob/ae7db2d98a071f52de3d60af9c937204b1f087a4/.buildkite/pipeline.yml#L147

Yes, what is not checked is the reversed I think, that there are no warnings with no features.

I believe this should be done by adding the -D warnings flag to the cargo build command, in the default rust-vmm-ci pipeline. WDYT?

This way we'd make this a default in rust-vmm, instead of having a custom step

Sounds good to me, do you want to open a PR with your suggestion?

yep, will do that

See rust-vmm/rust-vmm-ci#86

.buildkite/pipeline.yml

Signed-off-by: alindima <alindima@amazon.com>

Also adds a test that validates that the committed syscall tables were not manually altered. Signed-off-by: alindima <alindima@amazon.com>

Also add a custom pipeline that runs the syscall table validations. Signed-off-by: alindima <alindima@amazon.com>

- Modify the type declaration of BpfMap to not be tied to `Arc` filter ownership, since this is consumer-specific. Also removed the associated documentation note. - Fix broken link in the SeccompFilter cargo docs. Signed-off-by: alindima <alindima@amazon.com>

This enables compiling BPF filters from JSON code as an alternative of expressing the filters. This is done via a new library method: compile_from_json. Signed-off-by: alindima <alindima@amazon.com>

Signed-off-by: alindima <alindima@amazon.com>

Added a type that is used in the integration tests for expressing whether the last reported errno should be equal or not equal to a value or ignored. Signed-off-by: alindima <alindima@amazon.com>

Signed-off-by: alindima <alindima@amazon.com>

petreeftime · 2021-09-23T11:11:49Z

src/syscall_table/aarch64.rs

+use std::collections::HashMap;
+
+pub(crate) fn make_syscall_table() -> HashMap<&'static str, i64> {
+    vec![


Nit: I think these are easier to read in syscall number order.

They're sorted alphabetically. I don't have strong feelings about doing this either way, I think it depends on preference

petreeftime · 2021-09-23T11:17:09Z

Cargo.toml

@@ -10,4 +10,6 @@ license = "Apache-2.0 OR BSD-3-Clause"
 edition = "2018"

 [dependencies]
-libc = ">=0.2.98"
+libc = ">=0.2.39"
+serde = { version = ">=1.0.27", features = ["derive"] }


This matches also version 2.0.0 or larger, I think you'd want to specify it as ^1.0.27, which is equivalent to >=1.0.27 <2.0.0, so at least the major version wouldn't change. Alternatively, you could specify it as ~1.0.27 or >=1.0.27 <1.1.0

good call! will do

petreeftime · 2021-09-23T11:23:15Z

src/frontend/json.rs

+// Need the 'newtype' pattern so that we can implement a custom deserializer.
+pub(crate) struct JsonFilterMap(pub HashMap<String, JsonFilter>);
+
+// Implement a custom deserializer, that returns an error for duplicate thread keys.


You can do this with serde_with: https://docs.rs/serde_with/1.10.0/serde_with/rust/maps_duplicate_key_is_error/index.html.

I was also thinking about that, but serde_with pulls even more dependencies that we can otherwise avoid.

yep, I considered that as well, but I think it's best to avoid dependencies that we can otherwise work around quite easily

petreeftime · 2021-09-23T11:33:21Z

.github/dependabot.yml

@@ -3,7 +3,7 @@ updates:
 - package-ecosystem: cargo
  directory: "/"
  schedule:
-    interval: daily
+    interval: weekly


There are too many ops related PRs otherwise. This is a change at the rust-vmm org level.

Signed-off-by: alindima <alindima@amazon.com>

lauralt

Rubber stamp.

alindima self-assigned this Aug 16, 2021

alindima force-pushed the json_frontend branch 3 times, most recently from 92c9c98 to cf1da46 Compare August 16, 2021 13:30

andreeaflorescu reviewed Aug 24, 2021

View reviewed changes

tools/generate_syscall_tables.sh Outdated Show resolved Hide resolved

andreeaflorescu reviewed Aug 24, 2021

View reviewed changes

andreeaflorescu reviewed Aug 25, 2021

View reviewed changes

andreeaflorescu reviewed Sep 10, 2021

View reviewed changes

.buildkite/pipeline.yml Outdated Show resolved Hide resolved

alindima force-pushed the json_frontend branch 2 times, most recently from a51dc7b to 36b0d4d Compare September 13, 2021 09:25

alindima mentioned this pull request Sep 13, 2021

Bump rust-vmm-ci from 8901e77 to ae7db2d #15

Closed

Update rust-vmm-ci to ae7db2d

a8fc6fb

Signed-off-by: alindima <alindima@amazon.com>

alindima force-pushed the json_frontend branch from cf51d0e to 7755312 Compare September 14, 2021 10:39

alindima added 10 commits September 14, 2021 14:55

Script for generating and validating syscall tbls

187eda1

Also adds a test that validates that the committed syscall tables were not manually altered. Signed-off-by: alindima <alindima@amazon.com>

Add syscall tables

58086fc

Also add a custom pipeline that runs the syscall table validations. Signed-off-by: alindima <alindima@amazon.com>

Implement JSON frontend

84970f3

This enables compiling BPF filters from JSON code as an alternative of expressing the filters. This is done via a new library method: compile_from_json. Signed-off-by: alindima <alindima@amazon.com>

Add json integration tests

8af1e52

Signed-off-by: alindima <alindima@amazon.com>

add json compile-time feature

14c1460

Signed-off-by: alindima <alindima@amazon.com>

replace deprecated std::u32::max usage

392cf2d

Signed-off-by: alindima <alindima@amazon.com>

Impl std::error::Error for Error types

354ad77

Signed-off-by: alindima <alindima@amazon.com>

tests: add Errno type for validation functions

87ec6d3

Added a type that is used in the integration tests for expressing whether the last reported errno should be equal or not equal to a value or ignored. Signed-off-by: alindima <alindima@amazon.com>

update dependabot to run weekly

32fc41e

Signed-off-by: alindima <alindima@amazon.com>

alindima force-pushed the json_frontend branch from 7755312 to 32fc41e Compare September 14, 2021 12:02

petreeftime previously approved these changes Sep 23, 2021

View reviewed changes

Fix dependency versions to specific major num

12b8926

Signed-off-by: alindima <alindima@amazon.com>

alindima dismissed petreeftime’s stale review via 12b8926 September 23, 2021 14:19

andreeaflorescu approved these changes Sep 23, 2021

View reviewed changes

petreeftime approved these changes Sep 23, 2021

View reviewed changes

lauralt approved these changes Sep 23, 2021

View reviewed changes

andreeaflorescu merged commit c262897 into rust-vmm:main Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement json frontend #9

Implement json frontend #9

alindima commented Aug 16, 2021

andreeaflorescu left a comment

andreeaflorescu Aug 24, 2021

alindima Sep 10, 2021

andreeaflorescu Sep 13, 2021

alindima Sep 14, 2021

andreeaflorescu Aug 24, 2021

alindima Sep 10, 2021

andreeaflorescu Aug 24, 2021

alindima Sep 10, 2021

andreeaflorescu Aug 25, 2021

alindima Sep 10, 2021

andreeaflorescu Aug 25, 2021

alindima Sep 10, 2021

andreeaflorescu Sep 13, 2021

alindima Sep 14, 2021

andreeaflorescu Sep 14, 2021

alindima Sep 14, 2021

alindima Sep 14, 2021

petreeftime Sep 23, 2021

alindima Sep 23, 2021

petreeftime Sep 23, 2021

alindima Sep 23, 2021

alindima Sep 23, 2021

petreeftime Sep 23, 2021

andreeaflorescu Sep 23, 2021

alindima Sep 23, 2021

petreeftime Sep 23, 2021

andreeaflorescu Sep 23, 2021

lauralt left a comment

Implement json frontend #9

Implement json frontend #9

Conversation

alindima commented Aug 16, 2021

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauralt left a comment

Choose a reason for hiding this comment