search

rust-vmm / seccompiler

Provides easy-to-use Linux seccomp-bpf jailing.
https://crates.io/crates/seccompiler
Apache License 2.0
71 stars 11 forks source link

Backend implementation #7

Closed alindima closed 3 years ago

alindima commented 3 years ago

This PR implements the Rust IR and the logic for compiling it into BPF code.

In order for the design to be fully implemented, we need to add the JSON frontend, which will come in a future PR.

likebreath commented 3 years ago

@alindima Thank you for the great work. I finished a draft of integrating Cloud Hypervisor with your PR, and can confirm it works as expected with both SeccompAction Trap and Log.

Overall, I think the new interface (APIs and Types) are pretty good and cleaner. Admittedly it requires lots of changes due to the dramatic interface changes.

Only one comment/open-question: I wonder whether we should include some similar macros to make it easier (or more readable) to define complex SeccompCondition, for example the kvm ioctls rules in Cloud Hypervisor.

alindima commented 3 years ago

@alindima Thank you for the great work. I finished a draft of integrating Cloud Hypervisor with your PR, and can confirm it works as expected with both SeccompAction Trap and Log.

Thank you @likebreath for your work on integrating seccompiler in Cloud hypervisor, it's great to hear it works as expected! I've taken a quick look over your draft PR and it looks good.

Overall, I think the new interface (APIs and Types) are pretty good and cleaner. Admittedly it requires lots of changes due to the dramatic interface changes.

Great to hear the interface has been indeed improved. Unfortunately it does require quite a lot of changes, but most of them are repetitive (and automatable with regex find&replace) and the one-time character of the changes I think make the effort worth it 😄

Only one comment/open-question: I wonder whether we should include some similar macros to make it easier (or more readable) to define complex SeccompCondition, for example the kvm ioctls rules in Cloud Hypervisor.

That is a good question. I've given the issue some thought. The two macros have a pretty small implementation and are quite an opinionated way of instantiating the objects. For example, other users of the library may not want to unwrap() the result of SeccompRule::new(vec![$($x),*]), like you are doing in your integration.

Given the above, I think it would be fine to keep those small macros in CH for now. What do you think?

Of course, we can revisit this choice later on if we come up with better abstractions that make the library easier to use for everybody.

likebreath commented 3 years ago

That is a good question. I've given the issue some thought. The two macros have a pretty small implementation and are quite an opinionated way of instantiating the objects. For example, other users of the library may not want to unwrap() the result of SeccompRule::new(vec![$($x),*]), like you are doing in your integration.

Right. That's something I wanted to improve hopefully at the time of landing the integration to CH (after landing your PR of course).

Given the above, I think it would be fine to keep those small macros in CH for now. What do you think?

This sounds good to me.

Of course, we can revisit this choice later on if we come up with better abstractions that make the library easier to use for everybody.

Agree. Improving abstractions over creating complex conditions definitely worth a revisit.