[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] [PATCH RFC 3/3] rng: leak detection support
On 2/11/23 12:25, Michael S. Tsirkin wrote:
On Fri, Sep 22, 2023 at 05:40:50PM +0200, Babis Chalios wrote:On 22/9/23 17:06, Michael S. Tsirkin wrote:On Tue, Sep 19, 2023 at 12:11:37PM +0200, Babis Chalios wrote:On 19/9/23 12:01, Michael S. Tsirkin wrote:On Tue, Sep 19, 2023 at 09:32:08AM +0200, Babis Chalios wrote:Resending to fix e-mail formatting issues (sorry for the spam) On 18/9/23 18:30, Babis Chalios wrote:Yes, that's what the driver does now in the RFC patch. However, this just decreases the race window, it doesn't eliminate it. If a third leak event happens it might not find any buffers to use: 1. available buffers to queue 1-X 2. available buffers to queue X 3. poll queue X 4. used buffers in queue X <- leak event 1 will use buffers in X 5. avail buffers in queue X 6. poll queue 1-X <- leak event 2 will use buffers in 1-X 7. used buffers in queue 1-X 8. avail buffers in queue 1-X <- leak event 3 (it needs buffers in X, race with step 5) 9. goto 3I don't get it. we added buffers in step 5.What if the leak event 3 arrives before step 5 had time to actually add the buffers in X and make them visible to the device?Then it will see a single event in 1-X instead of two events. A leak is a leak though, I don't see does it matter how many triggered.So the scenario I have in mind is the following: (Epoch here is terminology that I used in the Linux RFC. It is a value maintained by random.c that changes every time a leak event happens). 1. add buffers to 1-X 2. add buffers to X 3. poll queue X 4. vcpu 0: get getrandom() entropy and cache epoch value 5. Device: First snapshot, uses buffers in X 6. vcpu 1: sees used buffers 7. Device: Second snapshot, uses buffers in 1-X 8. vcpu 0: getrandom() observes new epoch value & caches it 9. Device: Third snapshot, no buffers in either queue, (vcpu 1 from step 6 has not yet finished adding new buffers). 10. vcpu 1 adds new buffer in X 11. vcpu 0: getrandom() will not see new epoch and gets stale entropy. In this succession of events, when the third snapshot will happen, the device won't find any buffers in either queue, so it won't increase the RNG epoch value. So, any entropy gathered after step 8 will be the same across all snapshots. Am I missing something? Cheers, BabisYes but notice how this is followed by: 12. vcpu 1: sees used buffers in 1-X Driver can notify getrandom I guess?It could, but then we have the exact race condition that VMGENID had, userspace has already consumed stale entropy and there's nothing we can do about that. Although this is indeed a corner case, it feels like it beats the purpose of having the hardware update directly userspace (via copy on leak). How do you feel about the proposal a couple of emails back? It looks to me that it avoids completely the race condition. Cheers, BabisIt does. The problem of course is that this means that e.g. taking a snapshot of a guest that is stuck won't work well.That is true, but does it matter? The intention of the proposal is that if it is not safe to take snapshots (i.e. no buffers in the queue) don't take snapshots.OK. Basically I think if there's a way for device to detect that guest is stuck and not refilling the queue in a timely manner, then we are ok - host will make its own decisions on whether to snapshot or not. However, I feel in that case we need a way to create a big backlog of buffers for guest to fill such that this ring empty condition is very unlikely. One or even 2 queues does not seem enough then. For example, I can see a "stop" command that will tell device: "stop consuming buffers" and device will stop consuming buffers until the next leak event.
Yup, that seems reasonable to me. In that case, we could have a single queue, where the driver will fill up with multiple batches of commands where the last one is "stop" command, and then back-fill as needed. That should make it very unlikely for a well-behaving guest to run out of entropy leak commands in the queue. However, even with that design, I think there is value in putting something in the spec about the fact that the host might want to consider if it should, or not, take a snapshot when it finds the leak queue empty. Cheers, Babis
I have been thinking of adding MAP/UNMAP descriptors for a while now. Thus it will be possible to modify userspace memory without consuming buffers. Would something like this solve the problem?I am not familiar with MAP/UNMAP descriptors. Is there a link where I can read about them? Cheers, Babis
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]