opengl: add /usr/share/nvidia #12840

ZoopOTheGoop · 2023-05-23T17:03:32Z

This allows access to /usr/share/nvidia in the opengl plug. Nvidia apparently requests this be available in containers as per discussion in #12794

codecov-commenter · 2023-05-23T17:25:52Z

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (54a3194) 78.69% compared to head (d415854) 78.88%.
Report is 555 commits behind head on master.

Files	Patch %	Lines
osutil/kcmdline/kcmdline.go	67.74%	19 Missing and 1 partial ⚠️
interfaces/builtin/opengl.go	86.04%	4 Missing and 2 partials ⚠️
boot/cmdline.go	50.00%	1 Missing ⚠️
bootloader/grub.go	0.00%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12840      +/-   ##
==========================================
+ Coverage   78.69%   78.88%   +0.18%     
==========================================
  Files        1001     1031      +30     
  Lines      124257   130948    +6691     
==========================================
+ Hits        97788   103302    +5514     
- Misses      20316    21216     +900     
- Partials     6153     6430     +277

Flag	Coverage Δ
unittests	`78.88% <79.56%> (+0.18%)`	⬆️

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.

mvo5

The rule itself seems fine but will it work in the expected way? The path /usr/share/nvidia will be the path inside the mount namespace which will have the coreXX dirs under /usr/share and not the host system dirs? But maybe I'm missing something?

ZoopOTheGoop · 2023-05-24T19:05:38Z

No you're right I forgot to do a mount of the host fs, need to find where to do that

ZoopOTheGoop · 2023-07-31T22:36:22Z

finally got it working, change was more minor than I had thought

jhenstridge

I think you're going to need additional code to add a spec.AddUpdateNSf call so that snap-update-ns's AppArmor profile will allow it to perform this mount.

We should also test that this works in the spread tests. I'd suggest updating tests/main/interfaces-opengl-nvidia/task.yaml to do:

Create the /usr/share/nvidia directory in the prepare step, and add a file inside named as you'd expect the quirks json to be named.
In the execute step, check that the gl-core16 snap can read the file from its sandbox.
Remove the directory in the restore step.

ZoopOTheGoop · 2023-08-01T19:58:28Z

I'm not sure if I got the tests right, but the changes requested should be done modulo that. I had tested the plug in the steam snap where it worked presumably due to steam interface rules compounding with the opengl rules, but the AddUpdateNSf stuff was necessary for a test snap that only had the opengl plug.

jhenstridge

This is looking a lot closer to ready. The spread test needs some changes though, since it's not actually testing whether the paths are accessible within the sandbox.

interfaces/builtin/opengl.go

tests/main/interfaces-opengl-nvidia/task.yaml

jhenstridge

It looks like the spread tests are in as good a state as we can expect.

There's one last change, and I think the branch will be ready to go: please update the opengl_test.go unit tests to:

in the AppArmor test, check that spec.UpdateNS() includes the rules letting snap-update-ns mount /usr/share/nvidia.
Add a TestMountSpec test that demonstrates that the interface adds the mount rules.

You can use the tests in desktop_test.go as a guide for what these should look like.

jhenstridge

With these last tweaks to the unit tests, I think the branch will be ready.

interfaces/builtin/opengl_test.go

jhenstridge

Looks good!

alexmurray

LGTM!

pedronis

the addition to dirs.go seems a bit unwarranted, there's already a bit too many things there already as it is and interfaces usually don't add to it

pedronis · 2023-10-13T08:26:56Z

dirs/dirs.go

@@ -115,6 +115,8 @@ var (
 	SnapDeviceSaveDir string
 	SnapDataSaveDir   string

+	NvidiaProfilesDir string


we probably shouldn't define this here, but simply compute what we need in opengl.go

This avoids making the crowded surface of dirs.go even larger. Tweak the code to differentiate between /usr/share/nvidia in the host and in the per-snap mount namespace. Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2024-02-01T11:32:31Z

@ZoopOTheGoop @pedronis I've pushed an update that removes the change from dirs.go while retaining the semantics. I've adjusted the code in opengl.go a little, to make the mount namespace manipulation clearer by making a distinction between paths in host mount namespace and that of the snap application. Have look please.

bboozzoo

LGTM with a little naming tweak

bboozzoo · 2024-02-01T11:55:29Z

interfaces/builtin/opengl.go

@@ -172,6 +185,10 @@ unix (bind,listen) type=seqpacket addr="@cuda-uvmfd-[0-9a-f]*",
 unix (send, receive) type=dgram peer=(addr="@var/run/nvidia-xdriver-*"),
 `

+type openGlInterface struct {


Suggested change

type openGlInterface struct {

type openglInterface struct {

to be consistent with rest of the names used in this file

Signed-off-by: Zygmunt Krynicki <[email protected]>

zyga · 2024-02-09T08:55:14Z

I will refrain from self-reviewing this but I think it is good to merge.

pedronis

thanks

* add mount rules to opengl plug * properly allow mounts in apparmor * add tests for /usr/share/nvidia * update tests * generate apparmor rules properly * fix file naming in tests * unit tests * fix tests without /usr/share/nvidia * add TearDownTest * fix apparmor test dir ordering * convert back to raw string * remove superfluous characters * i/builtin: move /usr/share/nvidia to opengl internals This avoids making the crowded surface of dirs.go even larger. Tweak the code to differentiate between /usr/share/nvidia in the host and in the per-snap mount namespace. Signed-off-by: Zygmunt Krynicki <[email protected]> * i/builtin: rename openGlInterface to openglInterface Signed-off-by: Zygmunt Krynicki <[email protected]> * tests: add details to tests/main/interfaces-opengl-nvidia/task.yaml Signed-off-by: Zygmunt Krynicki <[email protected]> --------- Signed-off-by: Zygmunt Krynicki <[email protected]> Co-authored-by: Zygmunt Krynicki <[email protected]>

mvo5 added the Needs security review Can only be merged once security gave a :+1: label May 24, 2023

mvo5 reviewed May 24, 2023

View reviewed changes

pedronis requested a review from jhenstridge June 15, 2023 08:30

pedronis added the ⛔ Blocked label Jun 15, 2023

ZoopOTheGoop force-pushed the stage-nvidia branch from bd870f4 to 62c1f2c Compare August 1, 2023 00:24

add mount rules to opengl plug

55b1725

ZoopOTheGoop force-pushed the stage-nvidia branch from 62c1f2c to 55b1725 Compare August 1, 2023 00:25

jhenstridge requested changes Aug 1, 2023

View reviewed changes

Zoe Spellman added 2 commits August 1, 2023 12:49

properly allow mounts in apparmor

3301a30

add tests for /usr/share/nvidia

96369aa

jhenstridge requested changes Aug 2, 2023

View reviewed changes

interfaces/builtin/opengl.go Show resolved Hide resolved

tests/main/interfaces-opengl-nvidia/task.yaml Outdated Show resolved Hide resolved

Zoe Spellman added 3 commits August 2, 2023 10:10

update tests

4a5a31c

generate apparmor rules properly

1e4b96c

fix file naming in tests

ef7868f

jhenstridge reviewed Aug 3, 2023

View reviewed changes

unit tests

971f1e7

jhenstridge requested changes Aug 9, 2023

View reviewed changes

interfaces/builtin/opengl_test.go Outdated Show resolved Hide resolved

interfaces/builtin/opengl_test.go Show resolved Hide resolved

Zoe Spellman added 2 commits August 9, 2023 12:01

fix tests without /usr/share/nvidia

52f27d1

add TearDownTest

eee088d

jhenstridge reviewed Aug 11, 2023

View reviewed changes

interfaces/builtin/opengl_test.go Outdated Show resolved Hide resolved

Zoe Spellman added 3 commits August 11, 2023 03:27

fix apparmor test dir ordering

783ab31

convert back to raw string

251b069

remove superfluous characters

7378b6d

jhenstridge approved these changes Aug 15, 2023

View reviewed changes

jhenstridge removed the ⛔ Blocked label Aug 15, 2023

ZoopOTheGoop requested a review from mvo5 August 21, 2023 21:32

pedronis added this to the 2.61 milestone Sep 7, 2023

alexmurray approved these changes Sep 14, 2023

View reviewed changes

alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Sep 14, 2023

mvo5 modified the milestones: 2.61, 2.62 Oct 4, 2023

mvo5 requested review from pedronis and removed request for mvo5 October 12, 2023 11:19

pedronis reviewed Oct 13, 2023

View reviewed changes

pedronis self-assigned this Oct 13, 2023

zyga self-requested a review February 1, 2024 10:36

i/builtin: move /usr/share/nvidia to opengl internals

fc2ab70

This avoids making the crowded surface of dirs.go even larger. Tweak the code to differentiate between /usr/share/nvidia in the host and in the per-snap mount namespace. Signed-off-by: Zygmunt Krynicki <[email protected]>

bboozzoo approved these changes Feb 1, 2024

View reviewed changes

zyga added 2 commits February 1, 2024 13:47

i/builtin: rename openGlInterface to openglInterface

bbaf73b

Signed-off-by: Zygmunt Krynicki <[email protected]>

tests: add details to tests/main/interfaces-opengl-nvidia/task.yaml

d415854

Signed-off-by: Zygmunt Krynicki <[email protected]>

Meulengracht requested a review from pedronis February 9, 2024 09:35

pedronis approved these changes Feb 9, 2024

View reviewed changes

zyga added ⛔ Blocked and removed ⛔ Blocked labels Feb 14, 2024

Meulengracht merged commit a1703df into canonical:master Feb 15, 2024

opengl: add /usr/share/nvidia #12840

opengl: add /usr/share/nvidia #12840

Uh oh!

Conversation

ZoopOTheGoop commented May 23, 2023

Uh oh!

codecov-commenter commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mvo5 left a comment

Choose a reason for hiding this comment

Uh oh!

ZoopOTheGoop commented May 24, 2023

Uh oh!

ZoopOTheGoop commented Jul 31, 2023

Uh oh!

jhenstridge left a comment

Choose a reason for hiding this comment

Uh oh!

ZoopOTheGoop commented Aug 1, 2023

Uh oh!

jhenstridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhenstridge left a comment

Choose a reason for hiding this comment

Uh oh!

jhenstridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhenstridge left a comment

Choose a reason for hiding this comment

Uh oh!

alexmurray left a comment

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

pedronis Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

zyga commented Feb 1, 2024

Uh oh!

bboozzoo left a comment

Choose a reason for hiding this comment

Uh oh!

bboozzoo Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

zyga commented Feb 9, 2024

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented May 23, 2023 •

edited

Loading