cmd/snap-confine: don't crash if nvidia module is loaded but drivers are not available #2881

Merged
merged 2 commits into from Feb 22, 2017

Conversation

Projects
None yet
3 participants
Contributor

zyga commented Feb 17, 2017

This fixes an issue where the running kernel has the nvidia module inserted but the root filesystem we are executing in does not have the userspace of the driver. This can happen when running in a LXD container.

zyga added some commits Feb 17, 2017

cmd/snap-confine: refactor nvidia mount logic
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
cmd/snap-confine: don't bind-mount missing nvidia libraries
Dont't try to bind-mount the nvidia userspace libraries if they are not
there even if the kernel module is loaded.

Fixes: https://bugs.launchpad.net/snap-confine/+bug/1665592
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
+ }
+ // Bind mount the binary nvidia driver into /var/lib/snapd/lib/gl.
+ debug("bind mounting nvidia driver %s -> %s", src, dst);
+ if (mount(src, dst, NULL, MS_BIND, NULL) != 0) {
@zyga

zyga Feb 21, 2017

Contributor

Maybe this should be MS_BIND | MS_RDONLY? Apparmor is there anyway but this would not hurt.

@chipaca

chipaca Feb 21, 2017

Member

does bind,ro dwiw? cool! do it :-)

@zyga

zyga Feb 22, 2017

Contributor

I'll do a separate branch that changes the mount to read-only

- dst);
- }
+
+ // If there's driver in the kernel then don't mount userspace.
@chipaca

chipaca Feb 21, 2017

Member

if there's no driver, pehaps?

+ // Construct the paths for the driver userspace libraries
+ // and for the gl directory.
+ char src[PATH_MAX], dst[PATH_MAX];
+ sc_must_snprintf(src, sizeof src, "/usr/lib/nvidia-%d",
@chipaca

chipaca Feb 21, 2017

Member

does everybody put this here?

@zyga

zyga Feb 22, 2017

Contributor

This is ubuntu specific code path. On Ubuntu it is there.

+ }
+ // Bind mount the binary nvidia driver into /var/lib/snapd/lib/gl.
+ debug("bind mounting nvidia driver %s -> %s", src, dst);
+ if (mount(src, dst, NULL, MS_BIND, NULL) != 0) {
@zyga

zyga Feb 21, 2017

Contributor

Maybe this should be MS_BIND | MS_RDONLY? Apparmor is there anyway but this would not hurt.

@chipaca

chipaca Feb 21, 2017

Member

does bind,ro dwiw? cool! do it :-)

@zyga

zyga Feb 22, 2017

Contributor

I'll do a separate branch that changes the mount to read-only

This looks fine to me, it is simply changing code to return early if major version is 0 and doing an access() check. I do wonder if the sc_mount_nvidia_driver_arch() function shouldn't also be changed....

@zyga zyga merged commit f24888f into snapcore:master Feb 22, 2017

6 checks passed

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
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:optional-nvidia branch Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment