[v2] linux: Split tst-ttyname

  The tst-ttyname may fail in container environments while trying to
mount the procfs after the unshare (test do_in_chroot_2).  It is not
clear why exaclty kernel returns EPERM in this case, the container
does have CAP_SYS_CHROOT, SELinux/AppArmor is disabled, and there is
no seccomp filter.

To avoid always reporting the test as FAIL in such scenario, the
test that uses new namespaces is moved to a new one and the failure
on the mount command is now report as UNSUPPORTED.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 sysdeps/unix/sysv/linux/Makefile              |   3 +-
 .../{tst-ttyname.c => tst-ttyname-common.c}   | 208 +-----------------
 sysdeps/unix/sysv/linux/tst-ttyname-direct.c  | 110 +++++++++
 .../unix/sysv/linux/tst-ttyname-namespace.c   | 147 +++++++++++++
 4 files changed, 263 insertions(+), 205 deletions(-)
 rename sysdeps/unix/sysv/linux/{tst-ttyname.c => tst-ttyname-common.c} (68%)
 create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname-direct.c
 create mode 100644 sysdeps/unix/sysv/linux/tst-ttyname-namespace.c
  

* Adhemerval Zanella:

> The tst-ttyname may fail in container environments while trying to
> mount the procfs after the unshare (test do_in_chroot_2).  It is not
> clear why exaclty kernel returns EPERM in this case, the container
> does have CAP_SYS_CHROOT, SELinux/AppArmor is disabled, and there is
> no seccomp filter.
>
> To avoid always reporting the test as FAIL in such scenario, the
> test that uses new namespaces is moved to a new one and the failure
> on the mount command is now report as UNSUPPORTED.

Sorry, there's this change a well:

-          VERIFY (mount ("proc", "/proc", "proc",
-                         MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0);
+	  if (mount ("proc", "/proc", "proc",
+		     MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) != 0)
+	    {
+	      /* This happens if we're trying to create a nested container,
+		 like if the build is running under podman, and we lack
+		 priviledges.  */
+	      if (errno == EPERM)
+		_exit (EXIT_UNSUPPORTED);
+	      else
+		_exit (EXIT_FAILURE);
+	    }

I wouldn't expect this based on the commit message.

The systemd developers say that mounting /proc before the
unshare (CLONE_NEWNS | CLONE_NEWPID) call should avoid the failure.

I'd prefer to fix that in a separate change.

Thanks,
Florian
  

Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:
> The systemd developers say that mounting /proc before the
> unshare (CLONE_NEWNS | CLONE_NEWPID) call should avoid the failure.

Beware doing this - /proc's contents depend on which pid namespace it's
in, so that things like /proc/self and /proc/$pid work correctly.  If
you mount /proc and then change the pid namespace, /proc will be wrong.
  

On Mon, Jun 05, 2023 at 03:32:43PM -0300, Adhemerval Zanella via Libc-alpha wrote:
> The tst-ttyname may fail in container environments while trying to
> mount the procfs after the unshare (test do_in_chroot_2).  It is not
> clear why exaclty kernel returns EPERM in this case, the container
> does have CAP_SYS_CHROOT, SELinux/AppArmor is disabled, and there is
> no seccomp filter.

If it's not yet clear why exactly EPERM is returned in this case,
please re-read https://bugzilla.redhat.com/show_bug.cgi?id=2210335#c10
mentioned earlier in this thread.
  

On 07/06/23 18:24, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> The tst-ttyname may fail in container environments while trying to
>> mount the procfs after the unshare (test do_in_chroot_2).  It is not
>> clear why exaclty kernel returns EPERM in this case, the container
>> does have CAP_SYS_CHROOT, SELinux/AppArmor is disabled, and there is
>> no seccomp filter.
>>
>> To avoid always reporting the test as FAIL in such scenario, the
>> test that uses new namespaces is moved to a new one and the failure
>> on the mount command is now report as UNSUPPORTED.
> 
> Sorry, there's this change a well:
> 
> -          VERIFY (mount ("proc", "/proc", "proc",
> -                         MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0);
> +	  if (mount ("proc", "/proc", "proc",
> +		     MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) != 0)
> +	    {
> +	      /* This happens if we're trying to create a nested container,
> +		 like if the build is running under podman, and we lack
> +		 priviledges.  */
> +	      if (errno == EPERM)
> +		_exit (EXIT_UNSUPPORTED);
> +	      else
> +		_exit (EXIT_FAILURE);
> +	    }
> 
> I wouldn't expect this based on the commit message.
> 
> The systemd developers say that mounting /proc before the
> unshare (CLONE_NEWNS | CLONE_NEWPID) call should avoid the failure.
> 
> I'd prefer to fix that in a separate change.

Already, I can add this change in a subsequent patch.


> If it's not yet clear why exactly EPERM is returned in this case,
> please re-read https://bugzilla.redhat.com/show_bug.cgi?id=2210335#c10
> mentioned earlier in this thread.

I have read the thread, but what is not clear to me is how to proper fix
on tst-ttyname.  Running on my environment I do see that after 
support_become_root the process mount point is indeed 'polluted' with
extra procfs mount added by the container itself:

/proc/self/mountinfo: 1241 1240 0:184 / /proc rw,nosuid,nodev,noexec,relatime - proc proc rw
/proc/self/mountinfo: 1242 1241 0:184 /bus /proc/bus ro,nosuid,nodev,noexec,relatime - proc proc rw
/proc/self/mountinfo: 1243 1241 0:184 /fs /proc/fs ro,nosuid,nodev,noexec,relatime - proc proc rw
/proc/self/mountinfo: 1244 1241 0:184 /irq /proc/irq ro,nosuid,nodev,noexec,relatime - proc proc rw
/proc/self/mountinfo: 1249 1241 0:184 /sys /proc/sys ro,nosuid,nodev,noexec,relatime - proc proc rw
/proc/self/mountinfo: 1257 1241 0:184 /sysrq-trigger /proc/sysrq-trigger ro,nosuid,nodev,noexec,relatime - proc proc rw
/proc/self/mountinfo: 1258 1241 0:189 / /proc/acpi ro,relatime - tmpfs tmpfs ro,inode64
/proc/self/mountinfo: 1259 1241 0:185 /null /proc/kcore rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
/proc/self/mountinfo: 1260 1241 0:185 /null /proc/keys rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
/proc/self/mountinfo: 1375 1241 0:185 /null /proc/timer_list rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
/proc/self/mountinfo: 1410 1241 0:190 / /proc/scsi ro,relatime - tmpfs tmpfs ro,inode64

The 'fix' that Christian has suggested is out of the scope of the test
because afaiu it should be done by the container management (before
spawning the process that trigger the glibc testcase).

I have tried to move this test to test-container, but it circles back
to the same issue.  That's why I have added the UNSUPPORTED if the inner
mount fails (I think I can improve the commit message).
  

On Thu, Jun 08, 2023 at 08:53:42AM -0300, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 07/06/23 18:24, Florian Weimer wrote:
> > * Adhemerval Zanella:
> > 
> >> The tst-ttyname may fail in container environments while trying to
> >> mount the procfs after the unshare (test do_in_chroot_2).  It is not
> >> clear why exaclty kernel returns EPERM in this case, the container
> >> does have CAP_SYS_CHROOT, SELinux/AppArmor is disabled, and there is
> >> no seccomp filter.
> >>
> >> To avoid always reporting the test as FAIL in such scenario, the
> >> test that uses new namespaces is moved to a new one and the failure
> >> on the mount command is now report as UNSUPPORTED.
> > 
> > Sorry, there's this change a well:
> > 
> > -          VERIFY (mount ("proc", "/proc", "proc",
> > -                         MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0);
> > +	  if (mount ("proc", "/proc", "proc",
> > +		     MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) != 0)
> > +	    {
> > +	      /* This happens if we're trying to create a nested container,
> > +		 like if the build is running under podman, and we lack
> > +		 priviledges.  */
> > +	      if (errno == EPERM)
> > +		_exit (EXIT_UNSUPPORTED);
> > +	      else
> > +		_exit (EXIT_FAILURE);
> > +	    }
> > 
> > I wouldn't expect this based on the commit message.
> > 
> > The systemd developers say that mounting /proc before the
> > unshare (CLONE_NEWNS | CLONE_NEWPID) call should avoid the failure.
> > 
> > I'd prefer to fix that in a separate change.
> 
> Already, I can add this change in a subsequent patch.
> 
> 
> > If it's not yet clear why exactly EPERM is returned in this case,
> > please re-read https://bugzilla.redhat.com/show_bug.cgi?id=2210335#c10
> > mentioned earlier in this thread.
> 
> I have read the thread, but what is not clear to me is how to proper fix
> on tst-ttyname.  Running on my environment I do see that after 
> support_become_root the process mount point is indeed 'polluted' with
> extra procfs mount added by the container itself:
> 
> /proc/self/mountinfo: 1241 1240 0:184 / /proc rw,nosuid,nodev,noexec,relatime - proc proc rw
> /proc/self/mountinfo: 1242 1241 0:184 /bus /proc/bus ro,nosuid,nodev,noexec,relatime - proc proc rw
> /proc/self/mountinfo: 1243 1241 0:184 /fs /proc/fs ro,nosuid,nodev,noexec,relatime - proc proc rw
> /proc/self/mountinfo: 1244 1241 0:184 /irq /proc/irq ro,nosuid,nodev,noexec,relatime - proc proc rw
> /proc/self/mountinfo: 1249 1241 0:184 /sys /proc/sys ro,nosuid,nodev,noexec,relatime - proc proc rw
> /proc/self/mountinfo: 1257 1241 0:184 /sysrq-trigger /proc/sysrq-trigger ro,nosuid,nodev,noexec,relatime - proc proc rw
> /proc/self/mountinfo: 1258 1241 0:189 / /proc/acpi ro,relatime - tmpfs tmpfs ro,inode64
> /proc/self/mountinfo: 1259 1241 0:185 /null /proc/kcore rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
> /proc/self/mountinfo: 1260 1241 0:185 /null /proc/keys rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
> /proc/self/mountinfo: 1375 1241 0:185 /null /proc/timer_list rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
> /proc/self/mountinfo: 1410 1241 0:190 / /proc/scsi ro,relatime - tmpfs tmpfs ro,inode64
> 
> The 'fix' that Christian has suggested is out of the scope of the test
> because afaiu it should be done by the container management (before
> spawning the process that trigger the glibc testcase).

So there's a few things to consider. What I wrote in that bug report
applies to unprivileged containers only - which is what the
systemd-nspawn bug report was about. If you're using a privileged
container things are different. So you need to figure that out first.

(1) unprivileged container
    If this is an unprivileged container then you can test whether my
    fix works by trying:

    umount /proc/kcore

    If that works then the mount isn't locked. So that means you could
    reveal the underlying files. If that's the case then within the
    container manager you can do:

    mount -t proc proc /run/host/proc

(2) privileged container
     (i) If this is a privileged container your container manager might
	 simply have dropped CAP_SYS_ADMIN so you can neither mount or
	 umount.
    (ii) The files and directories covering various proc files and
         directories are protected by an LSM profile making it
	 impossible to unmount them. For this to be meaningful you would
	 need to be restricted either from creating namespaces - which
	 doesn't seem to be the case - or the LSM profile would have to
	 restrict you from mounting procfs.

         If you happen to have bpftrace you can test this quickly by:

	 # This will report the return value of security_sb_mount().
	 sudo bpftrace -e 'kretprobe:security_sb_mount { printf("returned: %d\n", retval); }'

	 and then try:

	 mount -t proc proc /mnt

	 and see whether you get an errno.
  

On 09/06/23 04:49, Christian Brauner wrote:
> On Thu, Jun 08, 2023 at 08:53:42AM -0300, Adhemerval Zanella Netto via Libc-alpha wrote:
>>
>>
>> On 07/06/23 18:24, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> The tst-ttyname may fail in container environments while trying to
>>>> mount the procfs after the unshare (test do_in_chroot_2).  It is not
>>>> clear why exaclty kernel returns EPERM in this case, the container
>>>> does have CAP_SYS_CHROOT, SELinux/AppArmor is disabled, and there is
>>>> no seccomp filter.
>>>>
>>>> To avoid always reporting the test as FAIL in such scenario, the
>>>> test that uses new namespaces is moved to a new one and the failure
>>>> on the mount command is now report as UNSUPPORTED.
>>>
>>> Sorry, there's this change a well:
>>>
>>> -          VERIFY (mount ("proc", "/proc", "proc",
>>> -                         MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0);
>>> +	  if (mount ("proc", "/proc", "proc",
>>> +		     MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) != 0)
>>> +	    {
>>> +	      /* This happens if we're trying to create a nested container,
>>> +		 like if the build is running under podman, and we lack
>>> +		 priviledges.  */
>>> +	      if (errno == EPERM)
>>> +		_exit (EXIT_UNSUPPORTED);
>>> +	      else
>>> +		_exit (EXIT_FAILURE);
>>> +	    }
>>>
>>> I wouldn't expect this based on the commit message.
>>>
>>> The systemd developers say that mounting /proc before the
>>> unshare (CLONE_NEWNS | CLONE_NEWPID) call should avoid the failure.
>>>
>>> I'd prefer to fix that in a separate change.
>>
>> Already, I can add this change in a subsequent patch.
>>
>>
>>> If it's not yet clear why exactly EPERM is returned in this case,
>>> please re-read https://bugzilla.redhat.com/show_bug.cgi?id=2210335#c10
>>> mentioned earlier in this thread.
>>
>> I have read the thread, but what is not clear to me is how to proper fix
>> on tst-ttyname.  Running on my environment I do see that after 
>> support_become_root the process mount point is indeed 'polluted' with
>> extra procfs mount added by the container itself:
>>
>> /proc/self/mountinfo: 1241 1240 0:184 / /proc rw,nosuid,nodev,noexec,relatime - proc proc rw
>> /proc/self/mountinfo: 1242 1241 0:184 /bus /proc/bus ro,nosuid,nodev,noexec,relatime - proc proc rw
>> /proc/self/mountinfo: 1243 1241 0:184 /fs /proc/fs ro,nosuid,nodev,noexec,relatime - proc proc rw
>> /proc/self/mountinfo: 1244 1241 0:184 /irq /proc/irq ro,nosuid,nodev,noexec,relatime - proc proc rw
>> /proc/self/mountinfo: 1249 1241 0:184 /sys /proc/sys ro,nosuid,nodev,noexec,relatime - proc proc rw
>> /proc/self/mountinfo: 1257 1241 0:184 /sysrq-trigger /proc/sysrq-trigger ro,nosuid,nodev,noexec,relatime - proc proc rw
>> /proc/self/mountinfo: 1258 1241 0:189 / /proc/acpi ro,relatime - tmpfs tmpfs ro,inode64
>> /proc/self/mountinfo: 1259 1241 0:185 /null /proc/kcore rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
>> /proc/self/mountinfo: 1260 1241 0:185 /null /proc/keys rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
>> /proc/self/mountinfo: 1375 1241 0:185 /null /proc/timer_list rw,nosuid - tmpfs tmpfs rw,size=65536k,mode=755,inode64
>> /proc/self/mountinfo: 1410 1241 0:190 / /proc/scsi ro,relatime - tmpfs tmpfs ro,inode64
>>
>> The 'fix' that Christian has suggested is out of the scope of the test
>> because afaiu it should be done by the container management (before
>> spawning the process that trigger the glibc testcase).
> 
> So there's a few things to consider. What I wrote in that bug report
> applies to unprivileged containers only - which is what the
> systemd-nspawn bug report was about. If you're using a privileged
> container things are different. So you need to figure that out first.
> 
> (1) unprivileged container
>     If this is an unprivileged container then you can test whether my
>     fix works by trying:
> 
>     umount /proc/kcore
> 
>     If that works then the mount isn't locked. So that means you could
>     reveal the underlying files. If that's the case then within the
>     container manager you can do:
> 
>     mount -t proc proc /run/host/proc

Thanks for the explanation, but I still not really sure what this test
still in my environment (basically a docker container with CAP_SYS_ADMIN
plus apparmor disabled).

It does not seem to be this case, the running within the container and
adding a explicit umount on the test:

$ strace -e umount2 -f misc/tst-ttyname-namespace --direct
umount2("/proc/kcore", 0)               = -1 EINVAL (Invalid argument)

> 
> (2) privileged container
>      (i) If this is a privileged container your container manager might
> 	 simply have dropped CAP_SYS_ADMIN so you can neither mount or
> 	 umount.
>     (ii) The files and directories covering various proc files and
>          directories are protected by an LSM profile making it
> 	 impossible to unmount them. For this to be meaningful you would
> 	 need to be restricted either from creating namespaces - which
> 	 doesn't seem to be the case - or the LSM profile would have to
> 	 restrict you from mounting procfs.
> 
>          If you happen to have bpftrace you can test this quickly by:
> 
> 	 # This will report the return value of security_sb_mount().
> 	 sudo bpftrace -e 'kretprobe:security_sb_mount { printf("returned: %d\n", retval); }'
> 
> 	 and then try:
> 
> 	 mount -t proc proc /mnt
> 
> 	 and see whether you get an errno.

I am not really seeing any issue here:

# bpftrace -e 'kretprobe:security_sb_mount { printf("returned: %d\n", retval); }'
Attaching 1 probe...
returned: 0
returned: 0
returned: 0
returned: 0
returned: 0

While running:

$ misc/tst-ttyname-namespace
info:  entering chroot 2
info:    testcase: basic smoketest
info:      ttyname: PASS {name="/dev/pts/2", errno=0}
info:      ttyname_r: PASS {name="/dev/pts/2", ret=0, errno=0}
error: ../sysdeps/unix/sysv/linux/tst-ttyname-namespace.c:97: mount ("proc", "/proc", "proc", MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL) == 0: Operation not permitted

This is being really time consuming to figure out the underlying issue, so
I really think we should move the test to make it as UNSUPPORTED if the procfs
mount fails; it really does not seem to be a test failure but rather system 
configuration issue (which fails on the UNSUPPORTED case).