Fix SELinux detection in driver scripts#626
Conversation
e484641 to
d07feae
Compare
|
/ok-to-test d07feae |
|
Thanks @Ashutosh0x. We removed some dirs which were not maintained by us. Please rebase the PR from latest main so that someone from the team can review the PR @Shivkumar13 please take a look once updated. |
|
@Ashutosh0x Are you able to update your PR as per @rahulait's suggestion? |
d07feae to
1abd0ce
Compare
|
Hi @rahulait @rajathagasthya @Shivkumar13, I have successfully rebased the branch onto the latest |
|
|
||
| echo "Check SELinux status" | ||
| if [ -e /sys/fs/selinux ]; then | ||
| if command -v selinuxenabled >/dev/null 2>&1 && selinuxenabled; then |
There was a problem hiding this comment.
selinuxenabled is not installed by default in any of the base images. It requires libselinux-utils. So this check will always return false even if SELinux is enabled.
Make chcon calls non-fatal by appending '|| true' so that the script does not abort when SELinux is disabled but /sys/fs/selinux is still mounted. This avoids requiring libselinux-utils (which provides selinuxenabled) to be installed in the container images. For nvidia-driver scripts (rhel8, rhel9, rhel10, precompiled): retain the existing [ -e /sys/fs/selinux ] check and add '|| true' to chcon. For ocp_dtk_entrypoint scripts: keep the original unconditional chcon call but make it non-fatal with '|| true'. For vgpu-manager scripts: keep the original unconditional chcon call but make it non-fatal with '|| true'. Resolves #1489. Signed-off-by: Ashutosh Kumar Singh <ashutoshkumarsingh0x@gmail.com>
1abd0ce to
29e9a32
Compare
|
@rajathagasthya Good catch, thanks for pointing that out! You're right — I've updated the approach: instead of using
This avoids adding any new package dependencies while still preventing the script abort reported in #1489. |
rajathagasthya
left a comment
There was a problem hiding this comment.
I think this is reasonable. selinuxenabled doesn't really help as it also checks for /etc/selinux/config file, which isn't present inside the container. But I'd like another set of eyes on this. cc @tariq1890 @rahulait
@Ashutosh0x How are you planning to test this?
…us and enforce file presence
|
Hi @rajathagasthya @rahulait @Shivkumar13, I have refactored the approach to resolve both the dependency concern and the risk of swallowing legitimate errors. Instead of using if grep -qsw "selinuxfs" /proc/mounts && [ -f /sys/fs/selinux/enforce ]; then
# Run chcon strictly here
fiHere is the test matrix I've used to validate this change:
Please let me know if this looks good to merge! |
|
This change looks good to me. @tariq1890 do you have any concerns with this? |
|
My understanding is that OpenShift clusters have SELinux enabled by default, so the DTK entrypoint changes don't really address the issue. If we want to change the DTK entrypoint, we can explore that in a different PR.
I have the same question. |
This PR replaces the outdated filesystem check [ -e /sys/fs/selinux ] with a more robust call to selinuxenabled across all driver scripts.
Resolves NVIDIA/gpu-operator#1489