gnt: Avoid double unmapping on domain cleanup #5
+25
−14
Conversation
This file contains 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
|
This resolves the issue I was seeing with PVH unikernels, confirmed in manual testing. |
lindig
approved these changes
Jan 22, 2025
| caml_failwith ("Failed to unmap grant"); | ||
| } | ||
| xengnttab_handle* xgt = _G(xgh); | ||
| void* start_address = _M(array)->addr; |
There was a problem hiding this comment.
Would the code clearer without the alias that we create here for M(array)->addr? So not declaring start_address and using the pointer instead in all places? Because then the NULL-ing becomes more obvious.
There was a problem hiding this comment.
_M accesses an OCaml object, so we need some kind of alias anyway to only access C objects inside the critical section
robhoes
approved these changes
Jan 22, 2025
Oxenstored itself, when cleaning up domains and connections, can call Gnt.unmap transitively several times. Under certain conditions, this can later result in a segfault bringing oxenstored down when it tries to use a freed grant. Set the mmap_interface pointer to NULL after the first attempt so that it can be detected on the subsequent ones. Since we are already at it, bring out accesses to OCaml fields out of the critical section. Signed-off-by: Andrii Sultanov <[email protected]>
stub_gnttab_map_fresh Signed-off-by: Andrii Sultanov <[email protected]>
last-genius
force-pushed
the
private/asultanov/double-unmap
branch
from
013ea17 to
24b70bc
Compare
|
Addressed the suggestions, retested - looks good. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Oxenstored itself, when cleaning up domains and connections, can call Gnt.unmap transitively several times. Under certain conditions, this can later result in a segfault bringing oxenstored down when it tries to use a freed grant.
Set the mmap_interface pointer to NULL after the first attempt so that it can be detected on the subsequent ones.
Since we are already at it, bring out accesses to OCaml fields out of the critical section.