Skip to content
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

interfaces: Add more kernel fusion driver files to opengl #15068

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pedro-avalos
Copy link
Contributor

@pedro-avalos pedro-avalos commented Feb 12, 2025

This is a follow-up of #14685.

I am trying to create a snap of rocminfo, and I noticed that rocminfo is accessing the Kernel Fusion Driver via the /sys/devices/virtual/kfd path to retrieve the GPUs' properties. I've added the files that rocminfo was specifically accessing, but there are other subdirectories/files that are not included here.

These denials seem to also be blocking rocm-validation-suite from querying these properties when running some of its tests.

I also noticed that the Mir team is also interested in including rocminfo in their graphics-test-tools, so I'm sure they'd run into these issues eventually.

Let me know if you need anything or if I missed anything.

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@6f9106f). Learn more about missing BASE report.
Report is 236 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master   #15068   +/-   ##
=========================================
  Coverage          ?   78.06%           
=========================================
  Files             ?     1180           
  Lines             ?   157753           
  Branches          ?        0           
=========================================
  Hits              ?   123153           
  Misses            ?    26949           
  Partials          ?     7651           
Flag Coverage Δ
unittests 78.06% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Feb 13, 2025

Tue Feb 18 18:34:48 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13393202730

Failures:

Preparing:

  • openstack:debian-12-64
  • openstack:debian-sid-64
  • openstack:debian-12-64:tests/main/snapd-reexec:core
  • openstack:debian-12-64:tests/main/interfaces-fuse-support:parallel
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • google:ubuntu-22.04-64:tests/main/preseed-reset

Executing:

  • openstack:debian-12-64:tests/main/cgroup-devices-v2
  • google-core:ubuntu-core-22-64:tests/core/services
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups-self-manage
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups-required-or-optional
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups-serial-port
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups:kmsg
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups-helper
  • google:ubuntu-25.04-64:tests/main/cgroup-devices-v2
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups:uinput
  • google:ubuntu-22.04-64:tests/main/progress

Restoring:

  • openstack:debian-12-64:tests/unit/c-unit-tests-clang
  • openstack:debian-12-64:tests/unit/
  • openstack:debian-12-64:tests/unit/c-unit-tests-gcc
  • openstack:debian-12-64:tests/unit/
  • openstack:debian-12-64:tests/main/snap-quota-thread
  • openstack:debian-12-64:tests/main/snapd-reexec:core
  • openstack:debian-12-64:tests/main/
  • openstack:debian-12-64
  • openstack:debian-12-64:tests/main/interfaces-fuse-support:parallel
  • openstack:debian-12-64:tests/main/
  • openstack:debian-12-64
  • openstack:debian-12-64:tests/main/
  • openstack:debian-12-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • openstack:opensuse-tumbleweed-64
  • google:ubuntu-25.04-64:tests/main/security-device-cgroups-strict-enforced
  • google:ubuntu-22.04-64:tests/main/preseed-reset

@Meulengracht Meulengracht added the Needs security review Can only be merged once security gave a :+1: label Feb 13, 2025
@@ -198,6 +198,11 @@ unix (send, receive) type=dgram peer=(addr="@var/run/nvidia-xdriver-*"),
# Kernel Fusion Driver for AMD GPUs
/dev/kfd rw,
/sys/module/amdgpu/initstate r,
/sys/devices/virtual/kfd/kfd/dev r,
/sys/devices/virtual/kfd/kfd/uevent rw,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is write access definitely needed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC write is useful if you want to synthesize events from userspace. With how crazy the drivers are I wouldn't be surprised it's actually used for something. In any case, getting a conformation for needing write access would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them as the rocminfo utility seemed to access them. I added these all based on the app armor allows when using it in devmode.

I am off today and Monday, but if I get a chance tonight, I can re run it and capture the logs if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, actually I don't see it in the logs when I run rocm-validation-suite or rocminfo, perhaps this isn't needed (at least in the usecase of rvs or rocminfo). I can bump that to read-only for now.

I've attached logs from a full run of rocminfo and a full run of rocm-validation-suite (which runs various different test programs that the Certification team uses for GPGPU testing).

snappy-debug-rocminfo.log
snappy-debug-rvs.log

Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for confirming that write permission is not needed.

@alexmurray alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Feb 19, 2025
@bboozzoo bboozzoo requested a review from zyga February 19, 2025 06:55
@bboozzoo bboozzoo added this to the 2.68.1 milestone Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants