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

[nspawn] fails with unclear error message if /dev is mounted in the target directory #3695

Closed
safinaskar opened this Issue Jul 10, 2016 · 5 comments

Comments

3 participants
@safinaskar

safinaskar commented Jul 10, 2016

  • Bug report

systemd version the issue has been seen with

230

Used distribution

Debian sid

Steps to reproduce: create chroot environment (for example, in /b), then:

mount --bind /dev /b/dev
systemd-nspawn -D /b

nspawn will not run chroot environment and will say instead:

Spawning conainer b on /b
Press ^] three times within 1s to kill container.
Failed to create directory /b/sys/fs/selinux: Read-only file system
Failed to create directory /b/sys/fs/selinux: Read-only file system
mknod(/b/dev/null) failed: File exists

(I already reported that "Read-only file system" here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=830693 , this bug report is not about it).
Expected behavior is to run the container.

Okey, why this is so important? Well, I often create some chroot environment, then mount some file systems (/proc, /dev, etc) and enter to them using typical chroot. Sometimes I use systemd-nspawn. Sometimes I mount some file systems, forget about them and then try to use systemd-nspawn. And it fails with very unclear error message (on systemd 215 on debian stable jessie error message is even more weird: just "container failed with code 1"). And this is very difficult to understand why.

So, please, support entering into any systems with some kernel file systems mounted (/proc, /sys, /dev, etc). Or at least print some helpful error message, for example: "/dev is mounted, unmount it and try again"

@poettering poettering added the nspawn label Aug 31, 2016

@poettering poettering added this to the v232 milestone Aug 31, 2016

tixxdz added a commit to endocode/systemd that referenced this issue Sep 27, 2016

nspawn: make sure that we fail in case some kernel fs is already mounted
This fixes bug systemd#3695

At the same time it adds a protection igainst userns chown of inodes of
a shared mount point.

tixxdz added a commit to endocode/systemd that referenced this issue Sep 27, 2016

nspawn: make sure that we fail in case some kernel backed fs is alrea…
…dy mounted

This fixes bug systemd#3695

At the same time it adds a protection igainst userns chown of inodes of
a shared mount point.
@tixxdz

This comment has been minimized.

Show comment
Hide comment
@tixxdz

tixxdz Sep 27, 2016

Member

@safinaskar @poettering fix for this is here #4226 however I'm not sure if any third party software may fail due to this change... cc @lucab @alban @jonboulle in any case if a shared mount point is backed by a kernel fs doing userns chown is wrong.

@safinaskar now it will print:
Failed to mount '/mnt/code/containers/fedora-tree-24/dev' already a mount point.

Thanks

Member

tixxdz commented Sep 27, 2016

@safinaskar @poettering fix for this is here #4226 however I'm not sure if any third party software may fail due to this change... cc @lucab @alban @jonboulle in any case if a shared mount point is backed by a kernel fs doing userns chown is wrong.

@safinaskar now it will print:
Failed to mount '/mnt/code/containers/fedora-tree-24/dev' already a mount point.

Thanks

tixxdz added a commit to endocode/systemd that referenced this issue Sep 28, 2016

@tixxdz

This comment has been minimized.

Show comment
Hide comment
@tixxdz

tixxdz Sep 28, 2016

Member

Ok updated in the appropriate PR, and for now we just warn the user, if someone wants to support pre-mounted kernel filesystems feel free...

Thanks!

Member

tixxdz commented Sep 28, 2016

Ok updated in the appropriate PR, and for now we just warn the user, if someone wants to support pre-mounted kernel filesystems feel free...

Thanks!

tixxdz added a commit to endocode/systemd that referenced this issue Oct 3, 2016

@keszybz keszybz closed this in 41eb436 Oct 5, 2016

@safinaskar

This comment has been minimized.

Show comment
Hide comment
@safinaskar

safinaskar Oct 6, 2016

@tixxdz, so, /dev is required to be empty now? This is very strange. Environments created by debootstrap have non-empty /dev by default

safinaskar commented Oct 6, 2016

@tixxdz, so, /dev is required to be empty now? This is very strange. Environments created by debootstrap have non-empty /dev by default

@tixxdz

This comment has been minimized.

Show comment
Hide comment
@tixxdz

tixxdz Oct 7, 2016

Member

@safinaskar AFAIK with nspawn this has always been the case "/dev/" was always populated/cleaned by nspawn, the code didn't change due to this PR.

Now quoting your message:

So, please, support entering into any systems with some kernel file systems mounted (/proc, /sys, /dev, etc). Or at least print some helpful error message, for example: "/dev is mounted, unmount it and try again"

I was lazy and don't have time right now, so I just did go with the second approach that you proposed for 2 reasons: 1) knowing that to support pre-mounted /proc, /sys, /dev we have to change nspawn to ignore completely user namespaces in that case and test it. 2) We are planning for a v232 release soon and the logic of the code didn't change.

So I took what you proposed ;-) now please open a new PR, feature request or issue to support pre-populated /dev nodes, reference this one, and let others comment on it.

Thank you!

Member

tixxdz commented Oct 7, 2016

@safinaskar AFAIK with nspawn this has always been the case "/dev/" was always populated/cleaned by nspawn, the code didn't change due to this PR.

Now quoting your message:

So, please, support entering into any systems with some kernel file systems mounted (/proc, /sys, /dev, etc). Or at least print some helpful error message, for example: "/dev is mounted, unmount it and try again"

I was lazy and don't have time right now, so I just did go with the second approach that you proposed for 2 reasons: 1) knowing that to support pre-mounted /proc, /sys, /dev we have to change nspawn to ignore completely user namespaces in that case and test it. 2) We are planning for a v232 release soon and the logic of the code didn't change.

So I took what you proposed ;-) now please open a new PR, feature request or issue to support pre-populated /dev nodes, reference this one, and let others comment on it.

Thank you!

@safinaskar

This comment has been minimized.

Show comment
Hide comment
@safinaskar

safinaskar Nov 6, 2016

@tixxdz .

At first I thought new code will print ".../dev/ should be an empty directory" in all cases when /dev is non-empty. And this worried me. Because I know debootstrap creates non-empty static /dev.

But now I performed some tests with fixed version (232) and everything is clear now for me. I see that non-empty /dev (as created by debootstrap) is OK if /dev is static (i. e. not mounted). But if /dev is non-empty AND mounted, then systemd (232) fails with error.

So, all is okey now.

But I think you should fix error message. Non-empty /dev is OK if it is static.
So, change this:

".../dev/ should be an empty directory"

to something like this:

"error: ../dev/ is mounted and it is not empty"

safinaskar commented Nov 6, 2016

@tixxdz .

At first I thought new code will print ".../dev/ should be an empty directory" in all cases when /dev is non-empty. And this worried me. Because I know debootstrap creates non-empty static /dev.

But now I performed some tests with fixed version (232) and everything is clear now for me. I see that non-empty /dev (as created by debootstrap) is OK if /dev is static (i. e. not mounted). But if /dev is non-empty AND mounted, then systemd (232) fails with error.

So, all is okey now.

But I think you should fix error message. Non-empty /dev is OK if it is static.
So, change this:

".../dev/ should be an empty directory"

to something like this:

"error: ../dev/ is mounted and it is not empty"

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