krun: ensure has_kvm and has_nitro are initialized #1861
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
Reviewer's GuideThis patch ensures that has_kvm and has_nitro flags are set before any early exit in modify_oci_configuration, removes brittle null-checks by allocating missing linux and resources structs, and refactors device array resizing and addition into a single coherent workflow to reliably inject /dev/kvm, /dev/sev, and /dev/nitro entries. Class diagram for updated krun device initializationclassDiagram
class krun_config {
+bool has_kvm
+bool has_nitro
}
class runtime_spec_schema_config_linux {
+resources: runtime_spec_schema_config_linux_resources
}
class runtime_spec_schema_config_linux_resources {
+devices: Device[]
+devices_len: size_t
}
class Device
krun_config --> runtime_spec_schema_config_linux
runtime_spec_schema_config_linux --> runtime_spec_schema_config_linux_resources
runtime_spec_schema_config_linux_resources --> Device
Flow diagram for device array allocation and injection in modify_oci_configurationflowchart TD
A[Check /dev/kvm, /dev/sev, /dev/nitro] --> B[Set has_kvm, has_nitro]
B --> C{has_kvm or has_nitro?}
C -- No --> D[Return]
C -- Yes --> E[Ensure linux and resources structs exist]
E --> F[Resize devices array]
F --> G[Inject /dev/kvm, /dev/sev, /dev/nitro as needed]
G --> H[Update devices_len]
H --> I[Return]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
iczero
force-pushed
the
krun-uninit-fix
branch
from
38c8caa to
ec0e8d2
Compare
tylerfanelli
suggested changes
Aug 28, 2025
There was a problem hiding this comment.
Since the OCI docs state that def->linux->resources->devices lists the devices that must be available in the container, I think we should allocate the array and add the specific devices needed.
We could check if def->linux->resources->devices == NULL and xmalloc the array if so.
|
Looks like that code was originally added to fix #1130 so krun won't be blocked by ebpf device filtering in cgroups. I'll fix it. |
iczero
force-pushed
the
krun-uninit-fix
branch
2 times, most recently
from
2d574e2 to
3e31393
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
1 similar comment
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
@sourcery-ai review |
![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 - here's some feedback:
- It looks like
kconf->has_sevisn’t being initialized alongsidehas_sev, so you should assignkconf->has_sev = has_sevto avoid using an uninitialized value. - Consider refactoring the repetitive device‐addition logic for KVM, SEV, and Nitro into a small helper function—this will make the length calculations and array writes clearer and reduce duplication.
- Double-check the
xrealloc(..., device_size * (new_len + 1))call to ensure the extra+1slot is intentional (e.g., for a sentinel) and avoid any off-by-one mistakes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- It looks like `kconf->has_sev` isn’t being initialized alongside `has_sev`, so you should assign `kconf->has_sev = has_sev` to avoid using an uninitialized value.
- Consider refactoring the repetitive device‐addition logic for KVM, SEV, and Nitro into a small helper function—this will make the length calculations and array writes clearer and reduce duplication.
- Double-check the `xrealloc(..., device_size * (new_len + 1))` call to ensure the extra `+1` slot is intentional (e.g., for a sentinel) and avoid any off-by-one mistakes.
## Individual Comments
### Comment 1
<location> `src/libcrun/handlers/krun.c:751` </location>
<code_context>
+ if (def->linux->resources == NULL)
+ def->linux->resources = xmalloc0 (sizeof (runtime_spec_schema_config_linux_resources));
+ old_len = def->linux->resources->devices_len;
+ new_len = old_len;
+ if (has_kvm)
</code_context>
<issue_to_address>
Consider refactoring device appending logic into helper functions to simplify and clarify the code.
```suggestion
Instead of manually tracking old_len/new_len and doing a “+1” trick, pull out two small helpers (`ensure_resources()` and `append_device()`) so you can just append each device inline. This preserves the new feature checks but removes all the repetitive bookkeeping.
1) Add at the top (e.g. above `libkrun_modify_oci_configuration`):
```c
static void ensure_resources(runtime_spec_schema_config_schema *def) {
if (!def->linux)
def->linux = xmalloc0(sizeof *def->linux);
if (!def->linux->resources)
def->linux->resources = xmalloc0(sizeof *def->linux->resources);
}
static void append_device(runtime_spec_schema_config_schema *def,
runtime_spec_schema_defs_linux_device_cgroup dev) {
size_t old = def->linux->resources->devices_len;
def->linux->resources->devices =
xrealloc(def->linux->resources->devices,
sizeof dev * (old + 1));
def->linux->resources->devices[old] = dev;
def->linux->resources->devices_len = old + 1;
}
```
2) Replace your block with:
```c
if (!has_kvm && !has_nitro)
return 0;
ensure_resources(def);
if (has_kvm) {
append_device(def, make_oci_spec_dev("a", st_kvm.st_rdev, true, "rwm"));
if (has_sev)
append_device(def, make_oci_spec_dev("a", st_sev.st_rdev, true, "rwm"));
}
if (has_nitro)
append_device(def, make_oci_spec_dev("a", st_nitro.st_rdev, true, "rwm"));
```
This keeps all functionality, drops `old_len`/`new_len` arithmetic, and makes appends much clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
iczero
force-pushed
the
krun-uninit-fix
branch
from
3e31393 to
fde9f4b
Compare
|
Last change fixes indent only |
iczero
force-pushed
the
krun-uninit-fix
branch
from
c1c35ed to
52cd684
Compare
|
please amend the last change in the first patch |
Fixes containers#1856 Signed-off-by: iczero <iczero4@gmail.com>
iczero
force-pushed
the
krun-uninit-fix
branch
from
52cd684 to
28d60d8
Compare
giuseppe
approved these changes
Aug 28, 2025
|
Already merged, but also LGTM. Thanks for addressing this @iczero |
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.
The OCI config path
.linux.resources.devicesis not set by rootless podman because rootless cannot access devices. This causes initialization ofhas_kvmandhas_nitroto be skipped, resulting in funny cases where krun claims that /dev/kvm doesn't exist on very specific platforms.Not sure if this is the preferred way to fix this. Maybe has_kvm shouldn't be set in modify_oci_configuration?Fixes #1856
Summary by Sourcery
Ensure has_kvm and has_nitro flags are initialized before early exits and consistently inject the /dev/kvm and /dev/nitro devices into the OCI configuration even when the devices array is initially missing.
Bug Fixes:
Enhancements: