Queue Rng device events only when active#643
Open
Golui wants to merge 1 commit intocontainers:mainfrom
Open
Conversation
Golui
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc,
slp and
tylerfanelli
as code owners
Golui
force-pushed
the
user/golui/fix-rng-panic
branch
from
5eb0db0 to
904ef73
Compare
Collaborator
|
Please add a Otherwise change looks good, thanks for the fix! |
When an Rng virtio device is deactivated, its event queues are set to None, but the remaining Subscribers may still attempt to queue events to the device, resulting in a panic. Signed-off-by: Golui <golui889@gmail.com>
Golui
force-pushed
the
user/golui/fix-rng-panic
branch
from
904ef73 to
e7450bf
Compare
Author
|
Done, thanks! |
Collaborator
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the process method in the VirtIO RNG event handler to prioritize activation events and defer request queue access until the device is activated. A potential race condition was identified where the activation handler might be invoked before the device queues are fully initialized, which could lead to a panic; adding a check for the existence of queues before handling the activation event is recommended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an Rng virtio device is deactivated, its event queues are set to None.
However, Subscribers may still attempt to queue events to the device, resulting in a panic.
This pull request addresses the issue by simply moving the event queueing logic only if the device is active, otherwise warns about a spurious event.
This happens when launching a FreeBSD guest with krunkit*, which results in ~80% of boots failing. I may have had a similar issue with alpine 3.23.3 once, but my memory is a bit fuzzy.
Updated the code to allow the device to be re-activated.
*to get FreeBSD to boot in krunkit at all, legacy console needs to be used (wip), and the GPU needs to be explicitly disabled. Came across this when looking into that.