Regression wrt mounting /proc in user namespace in 3.13

Just testing libvirt with user namespaces on current Fedora rawhide 3.13.0-0.rc0.git3.2.fc21.x86_64 kernel, I'm now getting an error when we attempt to mount /proc

  # virsh -c lxc:/// start shell
  error: Failed to start domain shell
  error: internal error: guest failed to start: Failed to mount proc on /proc type proc flags=e: Operation not permitted

The syscall failing is

mount("proc", "/proc", "proc", MS_NOSUID|MS_NODEV|MS_NOEXEC, NULL) = -1 EPERM (Operation not permitted)

On the host OS the default Fedora environment has the following mounts present

  # grep /proc /proc/mounts 
  proc /proc proc rw,nosuid,nodev,noexec,relatime 0 0
  systemd-1 /proc/sys/fs/binfmt_misc autofs rw,relatime,fd=41,pgrp=1,timeout=300,minproto=5,maxproto=5,direct 0 0
  binfmt_misc /proc/sys/fs/binfmt_misc binfmt_misc rw,relatime 0 0
  sunrpc /proc/fs/nfsd nfsd rw,relatime 0 0

  # ls /proc/fs/nfsd/
  export_features  filehandle      nfsv4gracetime  nfsv4recoverydir  pool_threads  reply_cache_stats        threads            unlock_ip
  exports          max_block_size  nfsv4leasetime  pool_stats        portlist      supported_krb5_enctypes  unlock_filesystem  versions

  # ls /proc/sys/fs/binfmt_misc/
  qemu-alpha  qemu-cris        qemu-microblazeel  qemu-mips64el  qemu-ppc64       qemu-sh4    qemu-sparc32plus  status
  qemu-arm    qemu-m68k        qemu-mips          qemu-mipsel    qemu-ppc64abi32  qemu-sh4eb  qemu-sparc64
  qemu-armeb  qemu-microblaze  qemu-mips64        qemu-ppc       qemu-s390x       qemu-sparc  register

Only if I umount both of the /proc/sys/fs/binfmt_misc/ entries am I able to get past this EPERM error code.

Looking at GIT history I see this change as a likely candidate for something which has changed in this area:

commit e51db73532955dc5eaba4235e62b74b460709d5b
Author: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>
Date: Sat Mar 30 19:57:41 2013 -0700

userns: Better restrictions on when proc and sysfs can be mounted

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Verify that the mounted filesystem is not covered in any significant way. I would love to verify that the previously mounted filesystem has no mounts on top but there are at least the directories /proc/sys/fs/binfmt_misc and /sys/fs/cgroup/ that exist explicitly for other filesystems to mount on top of.

Refactor the test into a function named fs_fully_visible and call that function from the mount routines of proc and sysfs. This makes this test local to the filesystems involved and the results current of when the mounts take place, removing a weird threading of the user namespace, the mount namespace and the filesystems themselves.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

My guess is fs_fully_visible() is returning false, and thus causing the proc_mount() call to return EPERM, but I'm unclear why this would happen, or if this is indeed a correct hypothesis.

Regards,
Daniel

Signature · 8 lines

|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Low on power and no charger, but a quick test printing out if a mount is !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:

[   92.939650] nlink is 1 for ino 8733 (0:3)

(that's major 0 minor 3)

Quoted 5 lines · Serge E. Hallyn

Low on power and no charger, but a quick test printing out if a mount is !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:

[   92.939650] nlink is 1 for ino 8733 (0:3)

Quoted 1 line

(that's major 0 minor 3)

Ok, so that is for binfmt_misc on /proc/sys/fs/binfmt_misc. The underlying directory is empty, and nlink is showing up as 1.

Can we just get the nlink check changed to check for < 3 instead of ==2 ?

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Serge E. Hallyn

Low on power and no charger, but a quick test printing out if a mount is !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:

[   92.939650] nlink is 1 for ino 8733 (0:3)

Quoted 8 lines

Quoted 1 line

(that's major 0 minor 3)

Ok, so that is for binfmt_misc on /proc/sys/fs/binfmt_misc. The underlying directory is empty, and nlink is showing up as 1.

Can we just get the nlink check changed to check for < 3 instead of ==2 ?

I already reported this problem to Eric,hi is working on fix this problem.

nlink is not the right thing to check if a directory is null. since in all of filesystems, parent dir's nlink is increase only when we create sub-dir.

Quoted 16 lines · Gao feng

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Serge E. Hallyn

Low on power and no charger, but a quick test printing out if a mount is !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:

[   92.939650] nlink is 1 for ino 8733 (0:3)

Quoted 8 lines

Quoted 1 line

(that's major 0 minor 3)

Ok, so that is for binfmt_misc on /proc/sys/fs/binfmt_misc. The underlying directory is empty, and nlink is showing up as 1.

Can we just get the nlink check changed to check for < 3 instead of ==2 ?

Quoted 5 lines

I already reported this problem to Eric,hi is working on fix this problem.

nlink is not the right thing to check if a directory is null. since in all of filesystems, parent dir's nlink is increase only when we create sub-dir.

This whole thing feels very brittle. May I also point out that simply setting perms appears to work just fine instead of overmounting. If I chmod 700 /proc/swaps, unshare my pid and mount namespaces and remount /proc, then /proc/swaps is 700 in the new mount. Since our concern is with a new user namespace, which will be limited to world perms, this should suffice and allow us to skip all this nonsense.

Eric?

-serge

Quoted 30 lines · Serge E. Hallyn

Quoted 16 lines · Gao feng

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Serge E. Hallyn

Low on power and no charger, but a quick test printing out if a mount is !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:

[   92.939650] nlink is 1 for ino 8733 (0:3)

Quoted 8 lines

Quoted 1 line

(that's major 0 minor 3)

Ok, so that is for binfmt_misc on /proc/sys/fs/binfmt_misc. The underlying directory is empty, and nlink is showing up as 1.

Can we just get the nlink check changed to check for < 3 instead of ==2 ?

Quoted 5 lines

I already reported this problem to Eric,hi is working on fix this problem.

nlink is not the right thing to check if a directory is null. since in all of filesystems, parent dir's nlink is increase only when we create sub-dir.

This whole thing feels very brittle. May I also point out that simply setting perms appears to work just fine instead of overmounting. If I chmod 700 /proc/swaps, unshare my pid and mount namespaces and remount /proc, then /proc/swaps is 700 in the new mount. Since our concern is with a new user namespace, which will be limited to world perms, this should suffice and allow us to skip all this nonsense.

Quoted 3 lines

Eric?

-serge

So yeah, I think this patch should be reverted, rather than "fixed".

Quoted 31 lines · Serge E. Hallyn

Quoted 30 lines · Serge E. Hallyn

Quoted 16 lines · Gao feng

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Serge E. Hallyn

Low on power and no charger, but a quick test printing out if a mount is !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:

[   92.939650] nlink is 1 for ino 8733 (0:3)

Quoted 8 lines

Quoted 1 line

(that's major 0 minor 3)

Ok, so that is for binfmt_misc on /proc/sys/fs/binfmt_misc. The underlying directory is empty, and nlink is showing up as 1.

Can we just get the nlink check changed to check for < 3 instead of ==2 ?

Quoted 5 lines

I already reported this problem to Eric,hi is working on fix this problem.

nlink is not the right thing to check if a directory is null. since in all of filesystems, parent dir's nlink is increase only when we create sub-dir.

This whole thing feels very brittle. May I also point out that simply setting perms appears to work just fine instead of overmounting. If I chmod 700 /proc/swaps, unshare my pid and mount namespaces and remount /proc, then /proc/swaps is 700 in the new mount. Since our concern is with a new user namespace, which will be limited to world perms, this should suffice and allow us to skip all this nonsense.

Quoted 5 lines

Quoted 3 lines

Eric?

-serge

So yeah, I think this patch should be reverted, rather than "fixed".

I will get back to this shortly. The trivial fix is to kill the nlink
test or make the nlink test look for nlink <= 2.

The point of the original patch was to remove the horrible special case that implemented the test to see if proc was moutned, so that we could trivial prevent mount of proc in chroots or other legacy environments where root did not want proc to be mounted.

The refinement I attempted and failed was to verify that people had only mounted on empty proc directories. Just to verify that nothing becomes possible when mounting a new copy of proc that wasn't possible before.

My test is totally for that last case is totally braindead. Permissions are nice but the actual concern is setups that did silly things that are not namespace aware.

I am totally against reverting the cleanup. But I have no problem killing the nlink test. Because we did not have protection against overmounted directories earlier.

Quoted 31 lines · Serge E. Hallyn

Quoted 30 lines · Serge E. Hallyn

Quoted 16 lines · Gao feng

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Serge E. Hallyn

Low on power and no charger, but a quick test printing out if a mount is !S_ISDIR or has nlink !=2 in fs_fully_visible() gives me:

[   92.939650] nlink is 1 for ino 8733 (0:3)

Quoted 8 lines

Quoted 1 line

(that's major 0 minor 3)

Ok, so that is for binfmt_misc on /proc/sys/fs/binfmt_misc. The underlying directory is empty, and nlink is showing up as 1.

Can we just get the nlink check changed to check for < 3 instead of ==2 ?

Quoted 5 lines

I already reported this problem to Eric,hi is working on fix this problem.

nlink is not the right thing to check if a directory is null. since in all of filesystems, parent dir's nlink is increase only when we create sub-dir.

This whole thing feels very brittle. May I also point out that simply setting perms appears to work just fine instead of overmounting. If I chmod 700 /proc/swaps, unshare my pid and mount namespaces and remount /proc, then /proc/swaps is 700 in the new mount. Since our concern is with a new user namespace, which will be limited to world perms, this should suffice and allow us to skip all this nonsense.

Quoted 5 lines

Quoted 3 lines

Eric?

-serge

So yeah, I think this patch should be reverted, rather than "fixed".

I will get back to this shortly. The trivial fix is to kill the nlink
test or make the nlink test look for nlink <= 2.

The point of the original patch was to remove the horrible special case that implemented the test to see if proc was moutned, so that we could trivial prevent mount of proc in chroots or other legacy environments where root did not want proc to be mounted.

The refinement I attempted and failed was to verify that people had only mounted on empty proc directories. Just to verify that nothing becomes possible when mounting a new copy of proc that wasn't possible before.

My test is totally for that last case is totally braindead. Permissions are nice but the actual concern is setups that did silly things that are not namespace aware.

I am totally against reverting the cleanup. But I have no problem killing the nlink test. Because we did not have protection against overmounted directories earlier.

Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:
"proc: Fix the namespace inode permission checks." converted the namespace files into symlinks. The same commit changed the way namespace bind mounts appear in /proc/mounts:

  $ mount --bind /proc/self/ns/ipc /mnt/ipc
Originally:
  $ cat /proc/mounts | grep ipc
  proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0

After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:

  $ cat /proc/mounts | grep ipc
  proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0

This breaks userspace which expects the 2nd field in /proc/mounts to be a valid path.

Quoted 9 lines · Aditya Kali

Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:
"proc: Fix the namespace inode permission checks." converted the namespace files into symlinks. The same commit changed the way namespace bind mounts appear in /proc/mounts:

  $ mount --bind /proc/self/ns/ipc /mnt/ipc
Originally:
  $ cat /proc/mounts | grep ipc
  proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0

Quoted 6 lines

After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:

  $ cat /proc/mounts | grep ipc
  proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0

This breaks userspace which expects the 2nd field in /proc/mounts to be a valid path.

The symlink /proc/<pid>/ns/{ipc,mnt,net,pid,user,uts} point to dentries allocated with d_alloc_pseudo that we can mount, and that have interesting names printed out with d_dname.

When these files are bind mounted /proc/mounts is not currently displaying the mount point correctly because d_dname is called instead of just displaying the path where the file is mounted.

Solve this by adding an explicit check to distinguish mounted pseudo inodes and unmounted pseudo inodes. Unmounted pseudo inodes always use mount of their filesstem as the mnt_root in their path making these two cases easy to distinguish.

CC [email protected]

Reported-by Aditya Kali <[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/dcache.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

fs/dcache.c +6 -1

index 4bdb300b16e2..f7282ebf1a37 100644
@@ -3061,8 +3061,13 @@ char *d_path(const struct path *path, char *buf, int buflen)
 	 * thus don't need to be hashed.  They also don't need a name until a
 	 * user wants to identify the object in /proc/pid/fd/.  The little hack
 	 * below allows us to generate a name for these objects on demand:
+	 *
+	 * Some pseudo inodes are mountable.  When they are mounted
+	 * path->dentry == path->mnt->mnt_root.  In that case don't call d_dname
+	 * and instead have d_path return the mounted path.
 	 */
-	if (path->dentry->d_op && path->dentry->d_op->d_dname)
+	if (path->dentry->d_op && path->dentry->d_op->d_dname &&
+	    (!IS_ROOT(path->dentry) || path->dentry != path->mnt->mnt_root))
 		return path->dentry->d_op->d_dname(path->dentry, buf, buflen);
 
 	rcu_read_lock();

Signature · 1 line

1.7.5.4

Quoted 4 lines

proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0

This breaks userspace which expects the 2nd field in /proc/mounts to be a valid path.

The symlink /proc/<pid>/ns/{ipc,mnt,net,pid,user,uts} point to dentries allocated with d_alloc_pseudo that we can mount, and that have interesting names printed out with d_dname.

When these files are bind mounted /proc/mounts is not currently displaying the mount point correctly because d_dname is called instead of just displaying the path where the file is mounted.

Solve this by adding an explicit check to distinguish mounted pseudo inodes and unmounted pseudo inodes. Unmounted pseudo inodes always use mount of their filesstem as the mnt_root in their path making these two cases easy to distinguish.

There's a bunch of unpleasant details around the handling of "what if the final vfsmount is detached or internal" (and induced weirdness with d_absolute_path(), etc. callers deciding whether they want to call ->d_dname() manually, since only d_path() calls it); I'll look into that.

Eric, what behaviour do you want there?

Quoted 1 line · Al Viro

Eric, what behaviour do you want there?

Here is the behavior I want.

a) Something reasonable in /proc/self/fd when I just open one of these.

root@unassigned:~# exec 5< /proc/self/ns/net
root@unassigned:~# ls -l /proc/self/fd
total 0
lrwx------ 1 root root 64 Nov 30 13:12 0 -> /dev/ttyS0
lrwx------ 1 root root 64 Nov 30 13:12 1 -> /dev/ttyS0
lrwx------ 1 root root 64 Nov 30 13:12 2 -> /dev/ttyS0
lr-x------ 1 root root 64 Nov 30 13:12 3 -> /proc/718/fd
lr-x------ 1 root root 64 Nov 30 13:12 5 -> net:[4026531955]
root@unassigned:~# ip netns add foo

b) Something reasonable in /proc/mounts when I bind mount one of these.

root@unassigned:~# ip netns add foo
root@unassigned:~# cat /proc/mounts 
[...]
proc /var/run/netns/foo proc rw,nosuid,nodev,noexec,relatime 0 0

Any dentry allocated with d_alloc_pseudo will have the same problem if it is ever in a context where it can be bind mounted. So this is a general issue with d_name.

Quoted 18 lines · Eric W. Biederman

Quoted 1 line · Al Viro

Eric, what behaviour do you want there?

Here is the behavior I want.

a) Something reasonable in /proc/self/fd when I just open one of these.

root@unassigned:~# exec 5< /proc/self/ns/net
root@unassigned:~# ls -l /proc/self/fd
total 0
lrwx------ 1 root root 64 Nov 30 13:12 0 -> /dev/ttyS0
lrwx------ 1 root root 64 Nov 30 13:12 1 -> /dev/ttyS0
lrwx------ 1 root root 64 Nov 30 13:12 2 -> /dev/ttyS0
lr-x------ 1 root root 64 Nov 30 13:12 3 -> /proc/718/fd
lr-x------ 1 root root 64 Nov 30 13:12 5 -> net:[4026531955]
root@unassigned:~# ip netns add foo

Quoted 6 lines

b) Something reasonable in /proc/mounts when I bind mount one of these.

root@unassigned:~# ip netns add foo
root@unassigned:~# cat /proc/mounts 
[...]
proc /var/run/netns/foo proc rw,nosuid,nodev,noexec,relatime 0 0

Sigh... What do you want to see in /proc/self/mountinfo?

Quoted 3 lines

Any dentry allocated with d_alloc_pseudo will have the same problem if it is ever in a context where it can be bind mounted. So this is a general issue with d_name.

Yes, ->d_dname() is called in a wrong place. It should be in prepend_path if not deeper. What you are doing is growing a kludge on top of that mistake.

Note that your patch does nothing for mountinfo.

As for redesign... That's exactly what is needed in that area, instead of letting it fester.

As for check_mnt(old) in do_loopback()... I wonder if we shouldn't just turn that puppy into (check_mnt(old) || (old->mnt.mnt_flags & MNT_INTERNAL)). Note that MS_NOUSER in superblock flags would prevent binding pipefs and all mount_pseudo() users, so we wouldn't change existing behaviour...

Quoted 18 lines · Eric W. Biederman

Quoted 1 line · Al Viro

Eric, what behaviour do you want there?

Here is the behavior I want.

a) Something reasonable in /proc/self/fd when I just open one of these.

root@unassigned:~# exec 5< /proc/self/ns/net
root@unassigned:~# ls -l /proc/self/fd
total 0
lrwx------ 1 root root 64 Nov 30 13:12 0 -> /dev/ttyS0
lrwx------ 1 root root 64 Nov 30 13:12 1 -> /dev/ttyS0
lrwx------ 1 root root 64 Nov 30 13:12 2 -> /dev/ttyS0
lr-x------ 1 root root 64 Nov 30 13:12 3 -> /proc/718/fd
lr-x------ 1 root root 64 Nov 30 13:12 5 -> net:[4026531955]
root@unassigned:~# ip netns add foo

Quoted 6 lines

b) Something reasonable in /proc/mounts when I bind mount one of these.

root@unassigned:~# ip netns add foo
root@unassigned:~# cat /proc/mounts 
[...]
proc /var/run/netns/foo proc rw,nosuid,nodev,noexec,relatime 0 0

Sigh... What do you want to see in /proc/self/mountinfo?

Quoted 3 lines

Any dentry allocated with d_alloc_pseudo will have the same problem if it is ever in a context where it can be bind mounted. So this is a general issue with d_name.

Yes, ->d_dname() is called in a wrong place. It should be in prepend_path if not deeper. What you are doing is growing a kludge on top of that mistake.

Note that your patch does nothing for mountinfo.

As for redesign... That's exactly what is needed in that area, instead of letting it fester.

As for check_mnt(old) in do_loopback()... I wonder if we shouldn't just turn that puppy into (check_mnt(old) || (old->mnt.mnt_flags & MNT_INTERNAL)). Note that MS_NOUSER in superblock flags would prevent binding pipefs and all mount_pseudo() users, so we wouldn't change existing behaviour...

However it would be nice to see:
28 13 0:3 net:[4026532127] /var/run/netns/foo rw,nosuid,nodev,noexec,relatime - proc proc rw

Not having the path be the magical floating path is not causing regressions for people so I don't care much at the moment.

Quoted 4 lines

Quoted 3 lines

Any dentry allocated with d_alloc_pseudo will have the same problem if it is ever in a context where it can be bind mounted. So this is a general issue with d_name.

Quoted 3 lines

Yes, ->d_dname() is called in a wrong place. It should be in prepend_path if not deeper. What you are doing is growing a kludge on top of that mistake.

I disagree. I am not growing a kludge on of a mistake. I am refining the logic at the call site of d_dname. The logic to not call d_dname on a mountpoint should be there wherever we call d_dname.

At a practical level there are improvements to be had by calling d_dname in dentry_path and by adding my avoidance of calling d_dname on a mountpoint into tomoyo_realpath_from_path.

So while I see what you mean by prepend_path needing cleaning up that is really orthogonal to the acidental regression that I am fixing.

Quoted 2 lines

As for redesign... That's exactly what is needed in that area, instead of letting it fester.

I can't backport a redesign to fix the regression, and a redesign results in no practical benefit for me. So I might be persuaded later if I can find the time but right now a redesign is the wrong place to start.

Quoted 4 lines

As for check_mnt(old) in do_loopback()... I wonder if we shouldn't just turn that puppy into (check_mnt(old) || (old->mnt.mnt_flags & MNT_INTERNAL)). Note that MS_NOUSER in superblock flags would prevent binding pipefs and all mount_pseudo() users, so we wouldn't change existing behaviour...

Strictly speaking behavior would change for the proc namespace files, as now you could do things by finding a mount of proc in another mount namespace where the namespace files were available.

I would be a lot more comfortable with changing do_loopback if we could safely remove the check_mnt(old) test altogether. Why do we have the
check_mnt(old) test in do_loopback? If we can't remove the
check_mnt(old) test I need verify that the reasons for that test don't apply to my namespace file descriptor case. Otherwise I can't see any real problems with making that change (time permitting).

FWIW, the other callers of prepend_path() boil down to /proc/mountinfo handling, apparmour d_namespace_path() (separate handling of MNT_INTERNAL, __d_path() or d_absolute_path() for the rest) and tomoyo_get_absolute_path() (this one directly calls ->d_dname() itself).

Quoted 4 lines · Al Viro

FWIW, the other callers of prepend_path() boil down to /proc/mountinfo handling, apparmour d_namespace_path() (separate handling of MNT_INTERNAL, __d_path() or d_absolute_path() for the rest) and tomoyo_get_absolute_path() (this one directly calls ->d_dname() itself).

While we are at it, what's the origin of if (buflen >= 256) checks in tomoyo_get_absolute_path() and tomoyo_get_dentry_path()? The minimal buflen value they can be called with is PAGE_SIZE - 1.

Quoted 4 lines · Al Viro

FWIW, the other callers of prepend_path() boil down to /proc/mountinfo handling, apparmour d_namespace_path() (separate handling of MNT_INTERNAL, __d_path() or d_absolute_path() for the rest) and tomoyo_get_absolute_path() (this one directly calls ->d_dname() itself).

While we are at it, what's the origin of if (buflen >= 256) checks in tomoyo_get_absolute_path() and tomoyo_get_dentry_path()? The minimal buflen value they can be called with is PAGE_SIZE - 1.

BTW, what happens if svc_export_request() ends up with pathname filling almost all space left, so that qword_add(bpp, blen, pth) right after the call of d_path() in there overwrites the beginning of d_path() output? Neil? And while we are at it, handling of overflow in there looks also looks fishy...

Quoted 5 lines · On Sat, 30 Nov 2013 17:02:26 +0000 Al Viro…

BTW, what happens if svc_export_request() ends up with pathname filling almost all space left, so that qword_add(bpp, blen, pth) right after the call of d_path() in there overwrites the beginning of d_path() output? Neil? And while we are at it, handling of overflow in there looks also looks fishy...

In this case the returned *blen will be negative so cache_request() in net/sunrpc/cache.h will return -EAGAIN.
cache_read() would then go into a tight loop, trying again and again to create the request, but it will never fit in the buffer.

I guess maybe an EINVAL might help there, plus code to skip over impossible requests.
Maybe the following. We'd need to double check that no ->cache_request function can fail in a way that is worth retrying, but I doubt they would.

Any thoughts Bruce?

Thanks,
NeilBrown

net/sunrpc/cache.c +1 -1

index a72de074172d..a065f827e2a3 100644
@@ -748,7 +748,7 @@ static int cache_request(struct cache_detail *detail,
 
 	detail->cache_request(detail, crq->item, &bp, &len);
 	if (len < 0)
-		return -EAGAIN;
+		return -EINVAL;
 	return PAGE_SIZE - len;
 }
 

@@ -788,6 +788,11 @@ static ssize_t cache_read(struct file *filp, char __user *buf, size_t count,
 

Quoted 5 lines · On Sat, 30 Nov 2013 17:02:26 +0000 Al Viro…

BTW, what happens if svc_export_request() ends up with pathname filling almost all space left, so that qword_add(bpp, blen, pth) right after the call of d_path() in there overwrites the beginning of d_path() output? Neil? And while we are at it, handling of overflow in there looks also looks fishy...

In this case the returned *blen will be negative so cache_request() in net/sunrpc/cache.h will return -EAGAIN.
cache_read() would then go into a tight loop, trying again and again to create the request, but it will never fit in the buffer.

I guess maybe an EINVAL might help there, plus code to skip over impossible requests.
Maybe the following. We'd need to double check that no ->cache_request function can fail in a way that is worth retrying, but I doubt they would.

Any thoughts Bruce?

Thanks,
NeilBrown

net/sunrpc/cache.c +6 -1

index a72de074172d..a065f827e2a3 100644
@@ -748,7 +748,7 @@ static int cache_request(struct cache_detail *detail,
 
 	detail->cache_request(detail, crq->item, &bp, &len);
 	if (len < 0)
-		return -EAGAIN;
+		return -EINVAL;
 	return PAGE_SIZE - len;
 }
 

@@ -788,6 +788,11 @@ static ssize_t cache_read(struct file *filp, char __user *buf, size_t count,
 
 	if (rq->len == 0) {
 		err = cache_request(cd, rq);
+		if (err == -EINVAL) {
+			spin_lock(&queue_lock);
+			list_move(&rp->q.list, &rq->q.list);
+			spin_unlock(&queue_lock);
+		}
 		if (err < 0)
 			goto out;
 		rq->len = err;

Quoted 9 lines · Aditya Kali

Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:
"proc: Fix the namespace inode permission checks." converted the namespace files into symlinks. The same commit changed the way namespace bind mounts appear in /proc/mounts:

  $ mount --bind /proc/self/ns/ipc /mnt/ipc
Originally:
  $ cat /proc/mounts | grep ipc
  proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0

Quoted 6 lines

After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:

  $ cat /proc/mounts | grep ipc
  proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0

This breaks userspace which expects the 2nd field in /proc/mounts to be a valid path.

The symlink /proc/<pid>/ns/{ipc,mnt,net,pid,user,uts} point to dentries allocated with d_alloc_pseudo that we can mount, and that have interesting names printed out with d_dname.

When these files are bind mounted /proc/mounts is not currently displaying the mount point correctly because d_dname is called instead of just displaying the path where the file is mounted.

Solve this by adding an explicit check to distinguish mounted pseudo inodes and unmounted pseudo inodes. Unmounted pseudo inodes always use mount of their filesstem as the mnt_root in their path making these two cases easy to distinguish.

CC [email protected]

Reported-by Aditya Kali <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/dcache.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

fs/dcache.c +6 -1

index 4bdb300b16e2..f7282ebf1a37 100644
@@ -3061,8 +3061,13 @@ char *d_path(const struct path *path, char *buf, int buflen)
 	 * thus don't need to be hashed.  They also don't need a name until a
 	 * user wants to identify the object in /proc/pid/fd/.  The little hack
 	 * below allows us to generate a name for these objects on demand:
+	 *
+	 * Some pseudo inodes are mountable.  When they are mounted
+	 * path->dentry == path->mnt->mnt_root.  In that case don't call d_dname
+	 * and instead have d_path return the mounted path.
 	 */
-	if (path->dentry->d_op && path->dentry->d_op->d_dname)
+	if (path->dentry->d_op && path->dentry->d_op->d_dname &&
+	    (!IS_ROOT(path->dentry) || path->dentry != path->mnt->mnt_root))
 		return path->dentry->d_op->d_dname(path->dentry, buf, buflen);
 
 	rcu_read_lock();

Signature · 1 line

1.7.5.4

Hi Oleg,

commit 40a0d32d1eaffe6aac7324ca92604b6b3977eb0e :
"fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks" breaks lxc-attach in 3.12. That code forks a child which does setns() and then does a clone(CLONE_PARENT). That way the grandchild can be in the right namespaces (which the child was not) and be a child of the original task, which is the monitor.

lxc-attach in 3.11 was working fine with no side effects that I could see. Is there a real danger in allowing CLONE_PARENT when current->nsproxy->pidns_for_children is not our pidns, or was this done out of an "over-abundance of caution"? Can we safely revert that new extra check?

Quoted 9 lines · Serge Hallyn

Hi Oleg,

commit 40a0d32d1eaffe6aac7324ca92604b6b3977eb0e :
"fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks" breaks lxc-attach in 3.12. That code forks a child which does setns() and then does a clone(CLONE_PARENT). That way the grandchild can be in the right namespaces (which the child was not) and be a child of the original task, which is the monitor.

Quoted 5 lines

lxc-attach in 3.11 was working fine with no side effects that I could see. Is there a real danger in allowing CLONE_PARENT when current->nsproxy->pidns_for_children is not our pidns, or was this done out of an "over-abundance of caution"? Can we safely revert that new extra check?

The two fundamental things I know we can not allow are:
- A shared signal queue aka CLONE_THREAD. Because we compute the pid and uid of the signal when we place it in the queue.

- Changing the pid and by extention pid_namespace of an existing process.

Quoted 1 line

From a parents perspective there is nothing special about the pid

namespace, to deny CLONE_PARENT, because the parent simply won't know or care.

Quoted 1 line

From the childs perspective all that is special really are shared signal

queues.

User mode threading with CLONE_PARENT|CLONE_VM|CLONE_SIGHAND and tasks in different pid namespaces is almost certainly going to break because it is complicated. But shared signal handlers can look at per thread information to know which pid namespace a process is in, so I don't know of any reason not to support CLONE_PARENT|CLONE_VM|CLONE_SIGHAND threads at the kernel level. It would be absolutely stupid to implement but that is a different thing.

So hmm.

Because it can do no harm, and because it is a regression let's remove the CLONE_PARENT check and send it stable.

Cc [email protected]

Acked-by Oleg Nesterov <[email protected]>

Acked-by Andy Lutomirski <[email protected]>

Acked-by Serge E. Hallyn <serge.hallyn-GeWIH/[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 kernel/fork.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/fork.c +1 -1

index 728d5be9548c..f82fa2ee7581 100644
@@ -1171,7 +1171,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	 * do not allow it to share a thread group or signal handlers or
 	 * parent with the forking task.
 	 */
-	if (clone_flags & (CLONE_SIGHAND | CLONE_PARENT)) {
+	if (clone_flags & CLONE_SIGHAND) {
 		if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
 		    (task_active_pid_ns(current) !=
 				current->nsproxy->pid_ns_for_children))

Signature · 1 line

1.7.5.4

Gao feng <[email protected]> reported that commit e51db73532955dc5eaba4235e62b74b460709d5b
userns: Better restrictions on when proc and sysfs can be mounted caused a regression on mounting a new instance of proc in a mount namespace created with user namespace privileges, when binfmt_misc is mounted on /proc/sys/fs/binfmt_misc.

This is an unintended regression caused by the absolutely bogus empty directory check in fs_fully_visible. The check fs_fully_visible replaced didn't even bother to attempt to verify proc was fully visible and hiding proc files with any kind of mount is rare. So for now fix the userspace regression by allowing directory with nlink == 1 as /proc/sys/fs/binfmt_misc has.

I will have a better patch but it is not stable material, or last minute kernel material. So it will have to wait.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/namespace.c +1 -2

index ac2ce8a766e1..be32ebccdeb1 100644
@@ -2886,7 +2886,7 @@ bool fs_fully_visible(struct file_system_type *type)
                        struct inode *inode = child->mnt_mountpoint->d_inode;
                        if (!S_ISDIR(inode->i_mode))
                                goto next;
-                       if (inode->i_nlink != 2)
+                       if (inode->i_nlink > 2)
                                goto next;
                }
                visible = true;
--

1.7.5.4

Quoted 15 lines · Eric W. Biederman

Gao feng <[email protected]> reported that commit e51db73532955dc5eaba4235e62b74b460709d5b
userns: Better restrictions on when proc and sysfs can be mounted caused a regression on mounting a new instance of proc in a mount namespace created with user namespace privileges, when binfmt_misc is mounted on /proc/sys/fs/binfmt_misc.

This is an unintended regression caused by the absolutely bogus empty directory check in fs_fully_visible. The check fs_fully_visible replaced didn't even bother to attempt to verify proc was fully visible and hiding proc files with any kind of mount is rare. So for now fix the userspace regression by allowing directory with nlink == 1 as /proc/sys/fs/binfmt_misc has.

Quoted 2 lines

I will have a better patch but it is not stable material, or last minute kernel material. So it will have to wait.

Is the better fix to fix procfs to set nlink == 2?

Gao feng <[email protected]> reported that commit e51db73532955dc5eaba4235e62b74b460709d5b
userns: Better restrictions on when proc and sysfs can be mounted caused a regression on mounting a new instance of proc in a mount namespace created with user namespace privileges, when binfmt_misc is mounted on /proc/sys/fs/binfmt_misc.

This is an unintended regression caused by the absolutely bogus empty directory check in fs_fully_visible. The check fs_fully_visible replaced didn't even bother to attempt to verify proc was fully visible and hiding proc files with any kind of mount is rare. So for now fix the userspace regression by allowing directory with nlink == 1 as /proc/sys/fs/binfmt_misc has.

I will have a better patch but it is not stable material, or last minute kernel material. So it will have to wait.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/namespace.c +1 -1

index ac2ce8a766e1..be32ebccdeb1 100644
@@ -2886,7 +2886,7 @@ bool fs_fully_visible(struct file_system_type *type)
 			struct inode *inode = child->mnt_mountpoint->d_inode;
 			if (!S_ISDIR(inode->i_mode))
 				goto next;
-			if (inode->i_nlink != 2)
+			if (inode->i_nlink > 2)
 				goto next;
 		}
 		visible = true;

Signature · 1 line

1.7.5.4

Gao feng <[email protected]> reported that commit e51db73532955dc5eaba4235e62b74b460709d5b
userns: Better restrictions on when proc and sysfs can be mounted caused a regression on mounting a new instance of proc in a mount namespace created with user namespace privileges, when binfmt_misc is mounted on /proc/sys/fs/binfmt_misc.

This is an unintended regression caused by the absolutely bogus empty directory check in fs_fully_visible. The check fs_fully_visible replaced didn't even bother to attempt to verify proc was fully visible and hiding proc files with any kind of mount is rare. So for now fix the userspace regression by allowing directory with nlink == 1 as /proc/sys/fs/binfmt_misc has.

I will have a better patch but it is not stable material, or last minute kernel material. So it will have to wait.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/namespace.c +0 -0

index ac2ce8a766e1..be32ebccdeb1 100644

Quoted 30 lines · Eric W. Biederman

Gao feng <[email protected]> reported that commit e51db73532955dc5eaba4235e62b74b460709d5b
userns: Better restrictions on when proc and sysfs can be mounted caused a regression on mounting a new instance of proc in a mount namespace created with user namespace privileges, when binfmt_misc is mounted on /proc/sys/fs/binfmt_misc.

This is an unintended regression caused by the absolutely bogus empty directory check in fs_fully_visible. The check fs_fully_visible replaced didn't even bother to attempt to verify proc was fully visible and hiding proc files with any kind of mount is rare. So for now fix the userspace regression by allowing directory with nlink == 1 as /proc/sys/fs/binfmt_misc has.

I will have a better patch but it is not stable material, or last minute kernel material. So it will have to wait.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/namespace.c +0 -0

index ac2ce8a766e1..be32ebccdeb1 100644

fs/namespace.c bool fs_fully_visible(struct file_system_type *type) · line 2892

Quoted 8 lines

fs/namespace.c +1 -1

@@ -2886,7 +2886,7 @@ bool fs_fully_visible(struct file_system_type *type)
 			struct inode *inode = child->mnt_mountpoint->d_inode;
 			if (!S_ISDIR(inode->i_mode))
 				goto next;
-			if (inode->i_nlink != 2)
+			if (inode->i_nlink > 2)
 				goto next;
 		}
 		visible = true;

As a quick fix.

Acked-by Gao feng <[email protected]>

Tested-by Gao feng <[email protected]>

looking forward to your following patch. :)

Quoted 30 lines · Eric W. Biederman

Gao feng <[email protected]> reported that commit e51db73532955dc5eaba4235e62b74b460709d5b
userns: Better restrictions on when proc and sysfs can be mounted caused a regression on mounting a new instance of proc in a mount namespace created with user namespace privileges, when binfmt_misc is mounted on /proc/sys/fs/binfmt_misc.

This is an unintended regression caused by the absolutely bogus empty directory check in fs_fully_visible. The check fs_fully_visible replaced didn't even bother to attempt to verify proc was fully visible and hiding proc files with any kind of mount is rare. So for now fix the userspace regression by allowing directory with nlink == 1 as /proc/sys/fs/binfmt_misc has.

I will have a better patch but it is not stable material, or last minute kernel material. So it will have to wait.

Cc [email protected]

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/namespace.c +0 -0

index ac2ce8a766e1..be32ebccdeb1 100644

fs/namespace.c bool fs_fully_visible(struct file_system_type *type) · line 2892

Quoted 8 lines

fs/namespace.c +1 -1

@@ -2886,7 +2886,7 @@ bool fs_fully_visible(struct file_system_type *type)
 			struct inode *inode = child->mnt_mountpoint->d_inode;
 			if (!S_ISDIR(inode->i_mode))
 				goto next;
-			if (inode->i_nlink != 2)
+			if (inode->i_nlink > 2)
 				goto next;
 		}
 		visible = true;

As a quick fix.

Acked-by Gao feng <[email protected]>

Tested-by Gao feng <[email protected]>

looking forward to your following patch. :)

To all: sorry for noise, I can't comment this patch.

But Eric, could you please help me to understand? I am totally confused.

So, afaics, initially (even after MS_KERNMOUNT) fs_fully_visible("proc") should return false.

After the normal "mout -t proc none /proc/" it becomes true.

And it is still true after, say, "mount -t ramfs none /proc/sys" because "ls -ld /proc/sys" shows ->i_nlink == 1.

However, say, "mount -t ramfs none /proc/tty/" should make fs_fully_visible() == F, because in this case ->i_nlink == 4.

Correct?

If yes, could you explain what this "!CAP_SYS_ADMIN && !fs_fully_visible" check actually tries to prevent and why?

Quoted 18 lines · Oleg Nesterov

To all: sorry for noise, I can't comment this patch.

But Eric, could you please help me to understand? I am totally confused.

So, afaics, initially (even after MS_KERNMOUNT) fs_fully_visible("proc") should return false.

After the normal "mout -t proc none /proc/" it becomes true.

And it is still true after, say, "mount -t ramfs none /proc/sys" because "ls -ld /proc/sys" shows ->i_nlink == 1.

However, say, "mount -t ramfs none /proc/tty/" should make fs_fully_visible() == F, because in this case ->i_nlink == 4.

Correct?

Quoted 2 lines

If yes, could you explain what this "!CAP_SYS_ADMIN && !fs_fully_visible" check actually tries to prevent and why?

The idea is that some admin on a host where /a/b/c/d exists, c/d should be hidden, so overmounts a tmpfs onto /a/b/c. In that case, an unpriv user could clone(CLONE_NEWUSER), then clone(CLONE_NEWNS), then umount /a/b/c and see /a/b/c/d. This patch was to try and prevent that.

To all: sorry for noise, I can't comment this patch.

But Eric, could you please help me to understand? I am totally confused.

So, afaics, initially (even after MS_KERNMOUNT) fs_fully_visible("proc") should return false.

After the normal "mout -t proc none /proc/" it becomes true.

And it is still true after, say, "mount -t ramfs none /proc/sys" because "ls -ld /proc/sys" shows ->i_nlink == 1.

Quoted 8 lines · Oleg Nesterov

To all: sorry for noise, I can't comment this patch.

But Eric, could you please help me to understand? I am totally confused.

So, afaics, initially (even after MS_KERNMOUNT) fs_fully_visible("proc") should return false.

Quoted 4 lines

After the normal "mout -t proc none /proc/" it becomes true.

And it is still true after, say, "mount -t ramfs none /proc/sys" because "ls -ld /proc/sys" shows ->i_nlink == 1.

Grr. mount -t ramfs none /proc/sys should make proc be not "fully visible".

To all: sorry for noise, I can't comment this patch.

But Eric, could you please help me to understand? I am totally confused.

So, afaics, initially (even after MS_KERNMOUNT) fs_fully_visible("proc") should return false.

After the normal "mout -t proc none /proc/" it becomes true.

And it is still true after, say, "mount -t ramfs none /proc/sys" because "ls -ld /proc/sys" shows ->i_nlink == 1.

However, say, "mount -t ramfs none /proc/tty/" should make fs_fully_visible() == F, because in this case ->i_nlink == 4.

Correct?

If yes, could you explain what this "!CAP_SYS_ADMIN && !fs_fully_visible" check actually tries to prevent and why?

[... snip ...]

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

Just to avoid the possible confusion, let me repeat that the fix itsef looks "obviously fine" to me, "i_nlink != 2" looks obviously wrong.

I am not arguing with this patch, I am just trying to understand this logic.

Quoted 2 lines · Eric W. Biederman

[... snip ...]

Thanks a lot.

Quoted 2 lines

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

this is what I can't understand...

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Just to avoid the possible confusion, let me repeat that the fix itsef looks "obviously fine" to me, "i_nlink != 2" looks obviously wrong.

I am not arguing with this patch, I am just trying to understand this logic.

Quoted 2 lines · Eric W. Biederman

[... snip ...]

Thanks a lot.

Quoted 2 lines

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

this is what I can't understand...

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Quoted 15 lines · Oleg Nesterov

Just to avoid the possible confusion, let me repeat that the fix itsef looks "obviously fine" to me, "i_nlink != 2" looks obviously wrong.

I am not arguing with this patch, I am just trying to understand this logic.

Quoted 2 lines · Eric W. Biederman

[... snip ...]

Thanks a lot.

Quoted 2 lines

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

Quoted 4 lines

this is what I can't understand...

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Quoted 15 lines · Oleg Nesterov

Just to avoid the possible confusion, let me repeat that the fix itsef looks "obviously fine" to me, "i_nlink != 2" looks obviously wrong.

I am not arguing with this patch, I am just trying to understand this logic.

Quoted 2 lines · Eric W. Biederman

[... snip ...]

Thanks a lot.

Quoted 2 lines

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

Quoted 4 lines

this is what I can't understand...

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Quoted 17 lines · ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected] (Eric W. Biederman) writes:

Quoted 15 lines · Oleg Nesterov

Just to avoid the possible confusion, let me repeat that the fix itsef looks "obviously fine" to me, "i_nlink != 2" looks obviously wrong.

I am not arguing with this patch, I am just trying to understand this logic.

Quoted 2 lines · Eric W. Biederman

[... snip ...]

Thanks a lot.

Quoted 2 lines

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

Quoted 6 lines

Quoted 4 lines

this is what I can't understand...

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Well strictly speaking it should fail after CLONE_NEWUSER | NEWNS | NEWPID. If proc was never mounted.

Fresh mounts of proc are not allowed unless you have also created the pid namespace. With just CLONE_NEWUSER | NEWNS you are limited to bind mounts.

Has this cleared up the confusion?

Eric

Quoted 6 lines · ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected] (Eric W. Biederman) writes:

Quoted 3 lines · Oleg Nesterov

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Well strictly speaking it should fail after CLONE_NEWUSER | NEWNS | NEWPID.

Has this cleared up the confusion?

Quoted 11 lines · Eric W. Biederman

Quoted 6 lines · ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected] (Eric W. Biederman) writes:

Quoted 3 lines · Oleg Nesterov

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Well strictly speaking it should fail after CLONE_NEWUSER | NEWNS | NEWPID.

Yes, yes, I understand, the mounter should be CAP_SYS_ADMIN in task_active_pid_ns().

Quoted 1 line

Has this cleared up the confusion?

Yes. Thanks.

Oleg.

Quoted 11 lines · Eric W. Biederman

Quoted 6 lines · [email protected] (Eric W. Biederman) writes:

Quoted 3 lines · Oleg Nesterov

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Well strictly speaking it should fail after CLONE_NEWUSER | NEWNS | NEWPID.

Yes, yes, I understand, the mounter should be CAP_SYS_ADMIN in task_active_pid_ns().

Quoted 1 line

Has this cleared up the confusion?

Yes. Thanks.

Oleg.

Quoted 17 lines · [email protected] (Eric W. Biederman) writes:

Quoted 15 lines · Oleg Nesterov

Just to avoid the possible confusion, let me repeat that the fix itsef looks "obviously fine" to me, "i_nlink != 2" looks obviously wrong.

I am not arguing with this patch, I am just trying to understand this logic.

Quoted 2 lines · Eric W. Biederman

[... snip ...]

Thanks a lot.

Quoted 2 lines

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

Quoted 6 lines

Quoted 4 lines

this is what I can't understand...

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Well strictly speaking it should fail after CLONE_NEWUSER | NEWNS | NEWPID. If proc was never mounted.

Fresh mounts of proc are not allowed unless you have also created the pid namespace. With just CLONE_NEWUSER | NEWNS you are limited to bind mounts.

Has this cleared up the confusion?

Eric

Quoted 29 lines · Eric W. Biederman

Quoted 17 lines · ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected] (Eric W. Biederman) writes:

Quoted 15 lines · Oleg Nesterov

Just to avoid the possible confusion, let me repeat that the fix itsef looks "obviously fine" to me, "i_nlink != 2" looks obviously wrong.

I am not arguing with this patch, I am just trying to understand this logic.

Quoted 2 lines · Eric W. Biederman

[... snip ...]

Thanks a lot.

Quoted 2 lines

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,

Quoted 6 lines

Quoted 4 lines

this is what I can't understand...

Lets ignore the implementation details. Suppose that proc was never mounted. Then "mount -t proc" should fail after CLONE_NEWUSER | NEWNS?

Yes.

Well strictly speaking it should fail after CLONE_NEWUSER | NEWNS | NEWPID. If proc was never mounted.

Quoted 8 lines

Fresh mounts of proc are not allowed unless you have also created the pid namespace. With just CLONE_NEWUSER | NEWNS you are limited to bind mounts.

Has this cleared up the confusion?

Eric

This is all obnoxiously complicated. I wonder if we can do (a lot) better by allowing a "pid-only" variant of proc to be mounted. It should contain:

- All the pid directories
- /proc/self, /proc/net, and /proc/mounts (but possibly not
/proc/PID/net -- that's a weird interface IMO and isn't really related to the pid)
- keys key-users (wtf is up with that interface, though -- those
files are way too magical)
- cpuinfo, version, and maybe other informational things (crypto?)
- loadavg, perhaps

I wonder it would be possible to boot a reasonable container with a heavily limited /proc like that.

[... snip ...]

For the real concern about jail environments where proc and sysfs are not mounted at all a fs_visible check is all that is really required,