interfaces/opengl: don't udev tag nvidia devices and use snap-confine instead (2.28) #4022

Merged
merged 17 commits into from Oct 11, 2017

Conversation

Projects
None yet
4 participants
Collaborator

mvo5 commented Oct 11, 2017

This should fix the regression in the opengl interface with the nvidia driver.

This is the 2.28 version of #3938

jdstrand and others added some commits Sep 18, 2017

interfaces/opengl: don't udev tag nvidia devices and use snap-confine…
… instead

/dev/nvidia* devices are created via the proprietary nvidia module. Proprietary
nvidia modules are not allowed to use sysfs, therefore they cannot be udev
tagged and ultimately won't be added to the device cgroup when the opengl
interface is connected. For now, adjust snap-confine to add /dev/nvidia* to the
device cgroup if they exist, and let AppArmor handle the mediation.
cmd: make fmt
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

codecov-io commented Oct 11, 2017

Codecov Report

Merging #4022 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4022   +/-   ##
=======================================
  Coverage   75.88%   75.88%           
=======================================
  Files         431      431           
  Lines       36915    36915           
=======================================
  Hits        28014    28014           
  Misses       6948     6948           
  Partials     1953     1953
Impacted Files Coverage Δ
interfaces/builtin/opengl.go 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4cbfef...60fbf76. Read the comment docs.

cmd/snap-confine: log devices added to device cgroup
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

I'm marking Approve for now. Your rules are fine but I suggest you use the ones I mentioned.

I'm surprised that 2.28 didn't have the nvidia code changes-- I specifically did the PR with priority so it would be in 2.28. I guess 2.28 branched before the PR. Darn

+ /sys/module/nvidia_uvm/uevent r,
+ /sys/module/nvidia_modeset/uevent r,
+ /sys/module/nvidia_drm/uevent r,
+ /sys/module/nvidia/uevent r,
@jdstrand

jdstrand Oct 11, 2017

Contributor

I suggest for future-proofing:

/sys/module/nvidia/* r,
/sys/**/nvidia*/uevent r,
@jdstrand

jdstrand Oct 11, 2017

Contributor

Let's make this even better:

/sys/module/nvidia{,_*}/* r,
/sys/**/drivers/nvidia{,_*}/* r,
@zyga

zyga Oct 11, 2017

Contributor

I applied both in sequence.

zyga added some commits Oct 11, 2017

cmd/snap-confine: future-proof nvidia confinement
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
cmd/snap-confine: more nvidia profile proofing
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga approved these changes Oct 11, 2017

+1 (tested on nvidia hardware)

@mvo5 mvo5 changed the base branch from master to release/2.28 Oct 11, 2017

@@ -249,6 +249,12 @@
# nvidia handling, glob needs /usr/** and the launcher must be
# able to bind mount the nvidia dir
/sys/module/nvidia/version r,
+ /sys/**/drivers/nvidia{,_*}/* r,
+ /sys/**/nvidia*/uevent r,
+ /sys/module/nvidia{,_*}/* r,
@jdstrand

jdstrand Oct 11, 2017

Contributor

This covers the first rule (/sys/module/nvidia/version r,) so you can omit the first rule.

@zyga

zyga Oct 11, 2017

Contributor

Let's remove this in the post-mortem move and let mvo release the point release.

@mvo5 mvo5 merged commit 775007d into snapcore:release/2.28 Oct 11, 2017

5 of 7 checks passed

artful-amd64 autopkgtest running
Details
artful-i386 autopkgtest running
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment