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

[RFC] Remove timerfd_create() from seccomp whitelist. #1281

Closed

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Sep 23, 2019

Signed-off-by: Andrei Sandu sandreim@amazon.com

Reason for This PR

Fixes #962.

The problem with the current design is that timerfd_create() is called during virtio device activate():

let handler = NetEpollHandler {
                rx: RxVirtio::new(
                    rx_queue,
                    rx_queue_evt,
                    self.rx_rate_limiter.take().unwrap_or_default(),
                ),
                tap,
                mem,
                tx: TxVirtio::new(
                    tx_queue,
                    tx_queue_evt,
                    self.tx_rate_limiter.take().unwrap_or_default(),
                ),
                interrupt_status: status,
                interrupt_evt,
                acked_features: self.acked_features,
                mmds_ns,
                guest_mac: self.guest_mac(),
                epoll_fd: self.epoll_config.epoll_raw_fd,
                rx_tap_listening: false,
                rx_tap_epoll_token: self.epoll_config.rx_tap_token,

                #[cfg(test)]
                test_mutators: tests::TestMutators::default(),
            };

The proposed solution adds flexibility when using timers with respect to minimalist seccomp whitelist.
One important note here is that the implemented Singleton is not Global State, it is just an OS resource store.

Description of Changes

Documented in commit message.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • This PR is linked to an issue and the reason for this PR is
    clearly provided.
  • The description of changes is clear and encompassing.
  • No docs need to be updated as part of this PR,
  • Code-level documentation for touched
    code is included in this PR.
  • No API changes are included in this PR.
  • The changes in this PR have no user impact.

@sandreim sandreim self-assigned this Sep 23, 2019
@sandreim sandreim changed the title Remove timerfd_create() from seccomp whitelist. [RFC] Remove timerfd_create() from seccomp whitelist. Sep 23, 2019
@sandreim sandreim force-pushed the timerfd_create_pool branch 3 times, most recently from d4ed7e5 to ffe04e6 Compare September 24, 2019 07:51
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

I like the strategy and the model, I think we will be making use of this model further down the line but in this particular context I believe it to be overkill.

default passive rate-limiters are being created even for devices with rate-limiting un-configured. Right now they're being created at device activation which is runtime (post-seccomp), but if I'm not mistaken it would be a simple surgical fix to create the default passive rate-limiters during device configuration (pre-seccomp).

The only other user of timer_fd is the metrics timer which can also be created pre-boot/pre-seccomp.

The end result would be the same, but with practically zero code/functionality additions.


As per our offline discussions I agree with you that the global resource store is the way to go in case of hot-plug for example, but I am sticking to the principle of keeping it simple and adding framework at the moment of need and not pre-emptively.

Let's get @firecracker-microvm/compute-capsule involved for some fresh viewpoints.

fc_util/src/timer_pool.rs Outdated Show resolved Hide resolved
Implement a timerfd pool with the Singleton pattern.
Currently only monotonic timers are supported by the pool.
The interface allows to provision timer fds before seccomp
filter is applied by issuing reserve_monotonic() calls.
Block and Net device emulation can later acquire the
provisioned resources using get_monotonic() calls.

Signed-off-by: Andrei Sandu <sandreim@amazon.com>
Co-Authored-By: Adrian Catangiu <31917973+acatangiu@users.noreply.github.com>
@dianpopa
Copy link
Contributor

dianpopa commented Dec 2, 2019

Given our decoupling desires, I am closing this for now. However, we should revisit this once the Vmm controller/builder is in place. See #1427.

@dianpopa dianpopa closed this Dec 2, 2019
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.

Remove timerfd_create() syscall from the seccomp allow-list
3 participants