Ensure that filemode is set for device nodes #275
Conversation
|
@zvonkok for the Kata use case the rust implementation must also be updated. |
elezar
force-pushed
the
add-extra-device-info
branch
from
4120810 to
8288672
Compare
| } | ||
|
|
||
| if d.Type != "" && (d.Major != 0 || d.Type == "p") { | ||
| return nil |
There was a problem hiding this comment.
Should we also set d.FileMode here?
There was a problem hiding this comment.
What would you set it to in this case?
There was a problem hiding this comment.
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.
pkg/cdi/container-edits_unix.go
Outdated
| d.FileMode = &di.fileMode | ||
| } | ||
|
|
||
| if d.Type == "p" { |
There was a problem hiding this comment.
| if d.Type == "p" { | |
| if d.Type == fifoDevice { |
There was a problem hiding this comment.
We use "p" elsewhere in this function. I don't mind changing this to a constant, but would do so consistently across this function.
|
This PR is stale because it has been open 90 days with no activity. This PR will be closed in 30 days unless new comments are made or the stale label is removed. To skip these checks, apply the "lifecycle/frozen" label. |
|
@elezar There are a couple of seemingly not yet addressed review comments in this PR. Should we wait until they're addressed or proceed to merge this PR as is? |
Signed-off-by: Evan Lezar <[email protected]>
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
force-pushed
the
add-extra-device-info
branch
from
8288672 to
457489d
Compare
|
@bart0sh I have revisted this PR and hopefully addressed your comments. |
|
@klihub Can you take a look at the latest changes and merge if everything is ok? |
| // Even for minimally-specified device nodes, we update the file mode if | ||
| // required. This is useful for rootless containers where device node | ||
| // requests may be treated as bind mounts. |
There was a problem hiding this comment.
@elezar A question related to me trying to understand the essence of that comment better. If we have a rootless container, where IIUC devices are bind-mounted into the container instead of being mknod'ded, will the devices not always end up with the permissions of the bind-mounted device on the host side ?
There was a problem hiding this comment.
You're right. I may have been mixing behaviour that I saw in another case. I will update the comment in a follow-up.
3 participants
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