event_monitor: refactor the implementation to support concurrent access

[brkp]

This patch modifies event_monitor to ensure that concurrent access to event_log from multiple threads is safe. Previously, the event_log function would acquire a reference to the event log file and write to it without doing any synchronization, which made it prone to data races. This issue likely went under the radar because the relevant SAFETY comment on the unsafe block was incomplete.

The new implementation spawns a dedicated thread named event-monitor solely for writing to the file. It uses the MPMC channel exposed by flume to pass messages to the event-monitor thread. Since flume::Sender<T> implements Sync, it is safe for multiple threads to share it and send messages to the event-monitor thread.

I looked into doing this with the unbounded MPSC in the standard library but unfortunately, it's !Sync, which actually is considered to be an API mistake. Meaning, the following snippet has soundness issues and is not safe if tx were to be a std::sync::mpsc::Sender<T>:

[...]
    if let Some(monitor_handle) = unsafe { MONITOR.as_ref() } {
        [...]
        if let Ok(event) = serde_json::to_string_pretty(&event) {|
            // invocation of this snippet from multiple threads
            // in the case `tx` is `!Sync` is not safe
            monitor_handle.tx.send(event).ok();
        }
    }
[...]

If anyone is aware of a workaround/better pattern for implementing this with the MPSC in the standard library, I'd love to hear about it.

Here are some links that can provide more context for this PR:

• https://doc.rust-lang.org/std/marker/trait.Sync.html
• https://doc.rust-lang.org/nomicon/send-and-sync.html
• https://doc.rust-lang.org/reference/behavior-considered-undefined.html

[likebreath]

The current implementation looks good to me. I wonder how does the the "event-monitor" thread exit gracefully? Also, looks like this thread won't terminate together with other thread upon exit_evt.

[rbradford]

The current implementation looks good to me. I wonder how does the the "event-monitor" thread exit gracefully? Also, looks like this thread won't terminate together with other thread upon exit_evt.

exit_evt is used for VM specific threads - this thread is a VMM thread and like e.g. the http/dbus API threads it will be exited when the VMM process terminates. Unless it needs to do some specific cleanup (FDs will be automatically closed by the OS) then there is no need to do any special handling.

[likebreath]

The current implementation looks good to me. I wonder how does the the "event-monitor" thread exit gracefully? Also, looks like this thread won't terminate together with other thread upon exit_evt.

exit_evt is used for VM specific threads - this thread is a VMM thread and like e.g. the http/dbus API threads it will be exited when the VMM process terminates. Unless it needs to do some specific cleanup (FDs will be automatically closed by the OS) then there is no need to do any special handling.

Thanks for the explanation, Rob.

[brkp]

Do you think that we should write to exit_evt when the event-monitor thread panics? This is the behavior implemented by other VMM threads as well, e.g. HTTP/D-Bus API threads. @likebreath @rbradford

[rbradford]

The current implementation looks good to me. I wonder how does the the "event-monitor" thread exit gracefully? Also, looks like this thread won't terminate together with other thread upon exit_evt.

exit_evt is used for VM specific threads - this thread is a VMM thread and like e.g. the http/dbus API threads it will be exited when the VMM process terminates. Unless it needs to do some specific cleanup (FDs will be automatically closed by the OS) then there is no need to do any special handling.

Thanks for the explanation, Rob.

Thanks to @brkp's point my reply was slightly off. There is an exit_evt in Vmm and it is used by support threads in the VMM (like the HTTP API server) but only to signal they have panicked/prematurely exited; they don't listen for the event and react to it like the VM support threads.

[rbradford]

Do you think that we should write to exit_evt when the event-monitor thread panics? This is the behavior implemented by other VMM threads as well, e.g. HTTP/D-Bus API threads. @likebreath @rbradford

Yes - please make it consistent with the other VMM support threads. I think there should also be some new seccomp rules too?

[rbradford]

@brkp I've drafted this as I think there are still some more bits to do...?

[brkp]

@brkp I've drafted this as I think there are still some more bits to do...?

Thanks -- yeah, sorry I haven't had the time to continue working on this. I also want to improve the error handling in here, alongside the previously mentioned things (seccomp rules, making the thread behavior more consistent with the rest of the VMM threads, etc.).

[brkp]

I've modified the event_monitor::set_monitor function so that now it only initializes the necessary global state and hands out a Monitor struct instance.

This change separates the logging logic from event_monitor, giving the caller more flexibility. vmm then uses this Monitor struct to spawn a thread called event-monitor and implements its own logging logic, which currently only writes to a file.

@rbradford @likebreath

[likebreath]

Changes look good to me. Just one thing about the seccomp filter. I think it is better to include brk and mmap to avoid potential violations for allocating memory on the event_monitor thread.

new changes

[brkp]

Thanks for catching that! @likebreath Added brk and mmap to the list of allowed calls as well.

[peng6662001]

@brkp Could you please share a way to verify this patch on aarch64?

[brkp]

@peng6662001 Would you mind providing a bit more context? I'm not quite sure what you mean by "verify".

[peng6662001]

@brkp Have you ever encountered a "concurrent access" bug?How to reproduce it?
I use --event-monitor path=/tmp/event.json to start the vm and the file /tmp/event.json looks fine with old version of CloudHypervisor.

[brkp]

@peng6662001 Hey, sorry for the late reply.

The previous implementation of event-monitor allows users to call the event_log function, which writes to a log file, from multiple threads without doing any synchronization.

This becomes problematic when two threads race to write to the log file at the same time. While this may not have been a significant issue in the past due to the scarce use of event-monitor throughout the code base, it is still an incorrect implementation going forward.