Skip to content

Ensure that filemode is set for device nodes #275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elezar
Copy link
Contributor

@elezar elezar commented Jun 19, 2025

This change ensures that the filemode for a CDI device is properly set from the host device node when adding devices to the CDI spec.

Without this the device node permissions are 000 in the container under certain conditions (such as Kata).

See also NVIDIA/nvidia-container-toolkit#960

@elezar elezar requested a review from klihub June 19, 2025 13:28
@elezar elezar self-assigned this Jun 19, 2025
@elezar elezar requested a review from zvonkok June 19, 2025 13:29
@elezar
Copy link
Contributor Author

elezar commented Jun 19, 2025

@zvonkok for the Kata use case the rust implementation must also be updated.

This change ensures that the filemode for a CDI device is
properly set from the host device node when adding devices
to the CDI spec.

Without this the device node permissions are 000 in the container
under certain conditions (such as Kata).

Signed-off-by: Evan Lezar <[email protected]>
@elezar elezar force-pushed the add-extra-device-info branch from 4120810 to 8288672 Compare June 19, 2025 13:36
@klihub klihub requested a review from bart0sh June 23, 2025 06:38
@@ -65,22 +86,31 @@ func (d *DeviceNode) fillMissingInfo() error {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also set d.FileMode here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you set it to in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is an early return meaning that we don't query the device node. I can make things a bit more consistent though. Let me see what I can come up with.

d.FileMode = &di.fileMode
}

if d.Type == "p" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if d.Type == "p" {
if d.Type == fifoDevice {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use "p" elsewhere in this function. I don't mind changing this to a constant, but would do so consistently across this function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants