krun: avoid failing if sev/nitro are not available #1862
Merged
Conversation
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
It's possible for a user to accidentally install the sev or nitro flavors of libkrun even if their system doesn't actually support those features. Before this change, having them installed on a system that doesn't support them would cause krun to stop working. Let's do the right thing and, if the bindings for "/dev/sev" or "/dev/nitro_enclaves" can't be created, close the handles to those library flavors and keep running with the generic one. If sev or nitro where really needed by the workload, it'll fail gracefully in "libkrun_configure_flavor". Fixes: containers#1120 containers#1300 Signed-off-by: Sergio Lopez <slp@redhat.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduce non-fatal handling for sev and nitro flavors by closing their dynamic library handles on device creation failure and falling back to the generic libkrun implementation. Sequence diagram for flavor fallback in krun container configurationsequenceDiagram
participant krun as krun
participant sev as libkrun_sev
participant nitro as libkrun_nitro
participant generic as libkrun_generic
krun->>sev: Try to create /dev/sev device
alt sev device creation fails
krun->>sev: dlclose handle_sev
krun->>generic: Continue with generic flavor
else sev device creation succeeds
krun->>sev: Use sev flavor
end
krun->>nitro: Try to create /dev/nitro_enclaves device
alt nitro device creation fails
krun->>nitro: dlclose handle_nitro
krun->>generic: Continue with generic flavor
else nitro device creation succeeds
krun->>nitro: Use nitro flavor
end
Class diagram for updated krun flavor handle managementclassDiagram
class KrunConfig {
+void* handle_sev
+void* handle_nitro
+void* handle_generic
}
class krun {
+libkrun_configure_container()
}
krun --> KrunConfig
class libkrun_configure_container {
+configure_flavor()
+create_dev()
}
krun --> libkrun_configure_container
libkrun_configure_container --> KrunConfig
KrunConfig : handle_sev can be NULL if sev unavailable
KrunConfig : handle_nitro can be NULL if nitro unavailable
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/libcrun/handlers/krun.c:585` </location>
<code_context>
+ ret = dlclose (kconf->handle_sev);
+ if (UNLIKELY (ret < 0))
+ return ret;
+ kconf->handle_sev = NULL;
+ }
}
</code_context>
<issue_to_address>
Setting handle_sev to NULL after dlclose is good, but only if dlclose succeeds.
Setting handle_sev to NULL unconditionally may cause resource leaks or double-free errors if dlclose fails. Only set handle_sev to NULL when dlclose succeeds.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
ret = dlclose (kconf->handle_sev);
if (UNLIKELY (ret < 0))
return ret;
kconf->handle_sev = NULL;
=======
ret = dlclose (kconf->handle_sev);
if (UNLIKELY (ret < 0))
return ret;
else
kconf->handle_sev = NULL;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/libcrun/handlers/krun.c:597` </location>
<code_context>
+ ret = dlclose (kconf->handle_nitro);
+ if (UNLIKELY (ret < 0))
+ return ret;
+ kconf->handle_nitro = NULL;
+ }
}
</code_context>
<issue_to_address>
Setting handle_nitro to NULL should depend on dlclose success.
Set handle_nitro to NULL only after confirming dlclose returns success to prevent resource leaks or inconsistent state.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
ret = dlclose (kconf->handle_nitro);
if (UNLIKELY (ret < 0))
return ret;
kconf->handle_nitro = NULL;
=======
ret = dlclose (kconf->handle_nitro);
if (UNLIKELY (ret < 0))
return ret;
else
kconf->handle_nitro = NULL;
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+582
to
+585
| ret = dlclose (kconf->handle_sev); | ||
| if (UNLIKELY (ret < 0)) | ||
| return ret; | ||
| kconf->handle_sev = NULL; |
There was a problem hiding this comment.
suggestion (bug_risk): Setting handle_sev to NULL after dlclose is good, but only if dlclose succeeds.
Setting handle_sev to NULL unconditionally may cause resource leaks or double-free errors if dlclose fails. Only set handle_sev to NULL when dlclose succeeds.
Suggested change
| ret = dlclose (kconf->handle_sev); | |
| if (UNLIKELY (ret < 0)) | |
| return ret; | |
| kconf->handle_sev = NULL; | |
| ret = dlclose (kconf->handle_sev); | |
| if (UNLIKELY (ret < 0)) | |
| return ret; | |
| else | |
| kconf->handle_sev = NULL; |
Comment on lines
+594
to
+597
| ret = dlclose (kconf->handle_nitro); | ||
| if (UNLIKELY (ret < 0)) | ||
| return ret; | ||
| kconf->handle_nitro = NULL; |
There was a problem hiding this comment.
suggestion (bug_risk): Setting handle_nitro to NULL should depend on dlclose success.
Set handle_nitro to NULL only after confirming dlclose returns success to prevent resource leaks or inconsistent state.
Suggested change
| ret = dlclose (kconf->handle_nitro); | |
| if (UNLIKELY (ret < 0)) | |
| return ret; | |
| kconf->handle_nitro = NULL; | |
| ret = dlclose (kconf->handle_nitro); | |
| if (UNLIKELY (ret < 0)) | |
| return ret; | |
| else | |
| kconf->handle_nitro = NULL; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
It's possible for a user to accidentally install the sev or nitro flavors of libkrun even if their system doesn't actually support those features.
Before this change, having them installed on a system that doesn't support them would cause krun to stop working. Let's do the right thing and, if the bindings for "/dev/sev" or "/dev/nitro_enclaves" can't be created, close the handles to those library flavors and keep running with the generic one.
If sev or nitro where really needed by the workload, it'll fail gracefully in "libkrun_configure_flavor".
Fixes: #1120 #1300
Summary by Sourcery
Allow krun to continue running with the generic flavor when /dev/sev or /dev/nitro_enclaves cannot be created by unloading the corresponding dynamic library instead of failing outright.
Enhancements: