diff mbox series

[v2] linux: return UNSUPPORTED in tst-mount if !support_can_chroot

Message ID 20220715000657.1712606-1-michael.hudson@canonical.com
State Superseded
Headers
Series [v2] linux: return UNSUPPORTED in tst-mount if !support_can_chroot |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Michael Hudson-Doyle July 15, 2022, 12:06 a.m. UTC 
  Otherwise the test fails if run in a chroot by a non-root user:

warning: could not become root outside namespace (Operation not permitted)
../sysdeps/unix/sysv/linux/tst-mount.c:36: numeric comparison failure
   left: 1 (0x1); from: errno
  right: 19 (0x13); from: ENODEV
error: ../sysdeps/unix/sysv/linux/tst-mount.c:39: not true: fd != -1
error: ../sysdeps/unix/sysv/linux/tst-mount.c:46: not true: r != -1
error: ../sysdeps/unix/sysv/linux/tst-mount.c:48: not true: r != -1
../sysdeps/unix/sysv/linux/tst-mount.c:52: numeric comparison failure
   left: 1 (0x1); from: errno
  right: 9 (0x9); from: EBADF
error: ../sysdeps/unix/sysv/linux/tst-mount.c:55: not true: mfd != -1
../sysdeps/unix/sysv/linux/tst-mount.c:58: numeric comparison failure
   left: 1 (0x1); from: errno
  right: 2 (0x2); from: ENOENT
error: ../sysdeps/unix/sysv/linux/tst-mount.c:61: not true: r != -1
../sysdeps/unix/sysv/linux/tst-mount.c:65: numeric comparison failure
   left: 1 (0x1); from: errno
  right: 2 (0x2); from: ENOENT
error: ../sysdeps/unix/sysv/linux/tst-mount.c:68: not true: pfd != -1
error: ../sysdeps/unix/sysv/linux/tst-mount.c:75: not true: fd_tree != -1
../sysdeps/unix/sysv/linux/tst-mount.c:88: numeric comparison failure
   left: 1 (0x1); from: errno
  right: 38 (0x26); from: ENOSYS
error: 12 test failures
---
v2: check support_can_chroot() rather than support_become_root return
value
---
 sysdeps/unix/sysv/linux/tst-mount.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Florian Weimer July 15, 2022, 7:07 a.m. UTC | #1 
* Michael Hudson-Doyle via Libc-alpha:

> Otherwise the test fails if run in a chroot by a non-root user:
>
> warning: could not become root outside namespace (Operation not permitted)
> ../sysdeps/unix/sysv/linux/tst-mount.c:36: numeric comparison failure
>    left: 1 (0x1); from: errno
>   right: 19 (0x13); from: ENODEV
> error: ../sysdeps/unix/sysv/linux/tst-mount.c:39: not true: fd != -1
> error: ../sysdeps/unix/sysv/linux/tst-mount.c:46: not true: r != -1
> error: ../sysdeps/unix/sysv/linux/tst-mount.c:48: not true: r != -1
> ../sysdeps/unix/sysv/linux/tst-mount.c:52: numeric comparison failure
>    left: 1 (0x1); from: errno
>   right: 9 (0x9); from: EBADF
> error: ../sysdeps/unix/sysv/linux/tst-mount.c:55: not true: mfd != -1
> ../sysdeps/unix/sysv/linux/tst-mount.c:58: numeric comparison failure
>    left: 1 (0x1); from: errno
>   right: 2 (0x2); from: ENOENT
> error: ../sysdeps/unix/sysv/linux/tst-mount.c:61: not true: r != -1
> ../sysdeps/unix/sysv/linux/tst-mount.c:65: numeric comparison failure
>    left: 1 (0x1); from: errno
>   right: 2 (0x2); from: ENOENT
> error: ../sysdeps/unix/sysv/linux/tst-mount.c:68: not true: pfd != -1
> error: ../sysdeps/unix/sysv/linux/tst-mount.c:75: not true: fd_tree != -1
> ../sysdeps/unix/sysv/linux/tst-mount.c:88: numeric comparison failure
>    left: 1 (0x1); from: errno
>   right: 38 (0x26); from: ENOSYS
> error: 12 test failures
> ---
> v2: check support_can_chroot() rather than support_become_root return
> value
> ---
>  sysdeps/unix/sysv/linux/tst-mount.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/tst-mount.c b/sysdeps/unix/sysv/linux/tst-mount.c
> index 502d7e3433..a0b367d9df 100644
> --- a/sysdeps/unix/sysv/linux/tst-mount.c
> +++ b/sysdeps/unix/sysv/linux/tst-mount.c
> @@ -20,6 +20,7 @@
>  #include <support/check.h>
>  #include <support/xunistd.h>
>  #include <support/namespace.h>
> +#include <support/test-driver.h>
>  #include <sys/wait.h>
>  #include <sys/mount.h>
>  
> @@ -104,6 +105,8 @@ static int
>  do_test (void)
>  {
>    support_become_root ();
> +  if (!support_can_chroot ())
> +    return EXIT_UNSUPPORTED;
>  
>    pid_t pid = xfork ();
>    if (pid == 0)

This test still hoses the system if run as root on a system without user
namespaces.

I think you should call and check support_enter_mount_namespace instead,
to make sure that the test does not modify the original mount namespace.

Thanks,
Florian
Carlos O'Donell July 15, 2022, 3:35 p.m. UTC | #2 
On 7/15/22 03:07, Florian Weimer via Libc-alpha wrote:
> * Michael Hudson-Doyle via Libc-alpha:
> 
>> Otherwise the test fails if run in a chroot by a non-root user:
>>
>> warning: could not become root outside namespace (Operation not permitted)
>> ../sysdeps/unix/sysv/linux/tst-mount.c:36: numeric comparison failure
>>    left: 1 (0x1); from: errno
>>   right: 19 (0x13); from: ENODEV
>> error: ../sysdeps/unix/sysv/linux/tst-mount.c:39: not true: fd != -1
>> error: ../sysdeps/unix/sysv/linux/tst-mount.c:46: not true: r != -1
>> error: ../sysdeps/unix/sysv/linux/tst-mount.c:48: not true: r != -1
>> ../sysdeps/unix/sysv/linux/tst-mount.c:52: numeric comparison failure
>>    left: 1 (0x1); from: errno
>>   right: 9 (0x9); from: EBADF
>> error: ../sysdeps/unix/sysv/linux/tst-mount.c:55: not true: mfd != -1
>> ../sysdeps/unix/sysv/linux/tst-mount.c:58: numeric comparison failure
>>    left: 1 (0x1); from: errno
>>   right: 2 (0x2); from: ENOENT
>> error: ../sysdeps/unix/sysv/linux/tst-mount.c:61: not true: r != -1
>> ../sysdeps/unix/sysv/linux/tst-mount.c:65: numeric comparison failure
>>    left: 1 (0x1); from: errno
>>   right: 2 (0x2); from: ENOENT
>> error: ../sysdeps/unix/sysv/linux/tst-mount.c:68: not true: pfd != -1
>> error: ../sysdeps/unix/sysv/linux/tst-mount.c:75: not true: fd_tree != -1
>> ../sysdeps/unix/sysv/linux/tst-mount.c:88: numeric comparison failure
>>    left: 1 (0x1); from: errno
>>   right: 38 (0x26); from: ENOSYS
>> error: 12 test failures
>> ---
>> v2: check support_can_chroot() rather than support_become_root return
>> value
>> ---
>>  sysdeps/unix/sysv/linux/tst-mount.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/sysdeps/unix/sysv/linux/tst-mount.c b/sysdeps/unix/sysv/linux/tst-mount.c
>> index 502d7e3433..a0b367d9df 100644
>> --- a/sysdeps/unix/sysv/linux/tst-mount.c
>> +++ b/sysdeps/unix/sysv/linux/tst-mount.c
>> @@ -20,6 +20,7 @@
>>  #include <support/check.h>
>>  #include <support/xunistd.h>
>>  #include <support/namespace.h>
>> +#include <support/test-driver.h>
>>  #include <sys/wait.h>
>>  #include <sys/mount.h>
>>  
>> @@ -104,6 +105,8 @@ static int
>>  do_test (void)
>>  {
>>    support_become_root ();
>> +  if (!support_can_chroot ())
>> +    return EXIT_UNSUPPORTED;
>>  
>>    pid_t pid = xfork ();
>>    if (pid == 0)
> 
> This test still hoses the system if run as root on a system without user
> namespaces.

I had not considered that configuration when I reviewed the tests.

The subprocess use of fsopen, fsconfig, and fsmount could indeed leave the original
mount namespace in a potentially different state than when it started.

> I think you should call and check support_enter_mount_namespace instead,
> to make sure that the test does not modify the original mount namespace.

Like this in the child?

diff --git a/sysdeps/unix/sysv/linux/tst-mount.c b/sysdeps/unix/sysv/linux/tst-mount.c
index 502d7e3433..d19d70d42d 100644
--- a/sysdeps/unix/sysv/linux/tst-mount.c
+++ b/sysdeps/unix/sysv/linux/tst-mount.c
@@ -107,7 +107,11 @@ do_test (void)
 
   pid_t pid = xfork ();
   if (pid == 0)
-    subprocess ();
+    {
+      if (!support_enter_mount_namespace ())
+        FAIL_UNSUPPORTED ("could not enter new mount namespace");
+      subprocess ();
+    }
 
   int status;
   xwaitpid (pid, &status, 0);
Florian Weimer July 15, 2022, 3:44 p.m. UTC | #3 
* Carlos O'Donell:

>> I think you should call and check support_enter_mount_namespace instead,
>> to make sure that the test does not modify the original mount namespace.
>
> Like this in the child?
>
> diff --git a/sysdeps/unix/sysv/linux/tst-mount.c b/sysdeps/unix/sysv/linux/tst-mount.c
> index 502d7e3433..d19d70d42d 100644
> --- a/sysdeps/unix/sysv/linux/tst-mount.c
> +++ b/sysdeps/unix/sysv/linux/tst-mount.c
> @@ -107,7 +107,11 @@ do_test (void)
>  
>    pid_t pid = xfork ();
>    if (pid == 0)
> -    subprocess ();
> +    {
> +      if (!support_enter_mount_namespace ())
> +        FAIL_UNSUPPORTED ("could not enter new mount namespace");
> +      subprocess ();
> +    }

Yes, except that you need to change

  xwaitpid (pid, &status, 0);
  TEST_VERIFY (WIFEXITED (status));

as well, to handle status 77.

I'm not entirely sure the fork is necessary, though.

Thanks,
Florian
Michael Hudson-Doyle July 15, 2022, 9:01 p.m. UTC | #4 
On Sat, 16 Jul 2022, 03:36 Carlos O'Donell via Libc-alpha, <
libc-alpha@sourceware.org> wrote:

> On 7/15/22 03:07, Florian Weimer via Libc-alpha wrote:
> > * Michael Hudson-Doyle via Libc-alpha:
> >
> >> Otherwise the test fails if run in a chroot by a non-root user:
> >>
> >> warning: could not become root outside namespace (Operation not
> permitted)
> >> ../sysdeps/unix/sysv/linux/tst-mount.c:36: numeric comparison failure
> >>    left: 1 (0x1); from: errno
> >>   right: 19 (0x13); from: ENODEV
> >> error: ../sysdeps/unix/sysv/linux/tst-mount.c:39: not true: fd != -1
> >> error: ../sysdeps/unix/sysv/linux/tst-mount.c:46: not true: r != -1
> >> error: ../sysdeps/unix/sysv/linux/tst-mount.c:48: not true: r != -1
> >> ../sysdeps/unix/sysv/linux/tst-mount.c:52: numeric comparison failure
> >>    left: 1 (0x1); from: errno
> >>   right: 9 (0x9); from: EBADF
> >> error: ../sysdeps/unix/sysv/linux/tst-mount.c:55: not true: mfd != -1
> >> ../sysdeps/unix/sysv/linux/tst-mount.c:58: numeric comparison failure
> >>    left: 1 (0x1); from: errno
> >>   right: 2 (0x2); from: ENOENT
> >> error: ../sysdeps/unix/sysv/linux/tst-mount.c:61: not true: r != -1
> >> ../sysdeps/unix/sysv/linux/tst-mount.c:65: numeric comparison failure
> >>    left: 1 (0x1); from: errno
> >>   right: 2 (0x2); from: ENOENT
> >> error: ../sysdeps/unix/sysv/linux/tst-mount.c:68: not true: pfd != -1
> >> error: ../sysdeps/unix/sysv/linux/tst-mount.c:75: not true: fd_tree !=
> -1
> >> ../sysdeps/unix/sysv/linux/tst-mount.c:88: numeric comparison failure
> >>    left: 1 (0x1); from: errno
> >>   right: 38 (0x26); from: ENOSYS
> >> error: 12 test failures
> >> ---
> >> v2: check support_can_chroot() rather than support_become_root return
> >> value
> >> ---
> >>  sysdeps/unix/sysv/linux/tst-mount.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/sysdeps/unix/sysv/linux/tst-mount.c
> b/sysdeps/unix/sysv/linux/tst-mount.c
> >> index 502d7e3433..a0b367d9df 100644
> >> --- a/sysdeps/unix/sysv/linux/tst-mount.c
> >> +++ b/sysdeps/unix/sysv/linux/tst-mount.c
> >> @@ -20,6 +20,7 @@
> >>  #include <support/check.h>
> >>  #include <support/xunistd.h>
> >>  #include <support/namespace.h>
> >> +#include <support/test-driver.h>
> >>  #include <sys/wait.h>
> >>  #include <sys/mount.h>
> >>
> >> @@ -104,6 +105,8 @@ static int
> >>  do_test (void)
> >>  {
> >>    support_become_root ();
> >> +  if (!support_can_chroot ())
> >> +    return EXIT_UNSUPPORTED;
> >>
> >>    pid_t pid = xfork ();
> >>    if (pid == 0)
> >
> > This test still hoses the system if run as root on a system without user
> > namespaces.
>
> I had not considered that configuration when I reviewed the tests.
>
> The subprocess use of fsopen, fsconfig, and fsmount could indeed leave the
> original
> mount namespace in a potentially different state than when it started.
>

This reminds me of one of my favourite bugs ever where gccgo ignored
CloneFlags in some situations and so when you built docker with it,
pivot_root was called on the default mount namespace....

Anyway, I am afk for the weekend now and have no attachment to being the
one to push the fix for this, if someone who is still at a computer can
write up a better fix, please go ahead!

Cheers,
mwh

> I think you should call and check support_enter_mount_namespace instead,
> > to make sure that the test does not modify the original mount namespace.
>
> Like this in the child?
>
> diff --git a/sysdeps/unix/sysv/linux/tst-mount.c
> b/sysdeps/unix/sysv/linux/tst-mount.c
> index 502d7e3433..d19d70d42d 100644
> --- a/sysdeps/unix/sysv/linux/tst-mount.c
> +++ b/sysdeps/unix/sysv/linux/tst-mount.c
> @@ -107,7 +107,11 @@ do_test (void)
>
>    pid_t pid = xfork ();
>    if (pid == 0)
> -    subprocess ();
> +    {
> +      if (!support_enter_mount_namespace ())
> +        FAIL_UNSUPPORTED ("could not enter new mount namespace");
> +      subprocess ();
> +    }
>
>    int status;
>    xwaitpid (pid, &status, 0);
>
> --
> Cheers,
> Carlos.
>
>
Carlos O'Donell July 16, 2022, 12:26 a.m. UTC | #5 
On 7/15/22 17:01, Michael Hudson-Doyle wrote:
> Anyway, I am afk for the weekend now and have no attachment to being the
> one to push the fix for this, if someone who is still at a computer can
> write up a better fix, please go ahead!

Thanks for the notice.

We can review again next week and fix the issue before 2.36 releases.
Michael Hudson-Doyle July 17, 2022, 9:44 p.m. UTC | #6 
On Sat, 16 Jul 2022 at 03:45, Florian Weimer via Libc-alpha <
libc-alpha@sourceware.org> wrote:

> * Carlos O'Donell:
>
> >> I think you should call and check support_enter_mount_namespace instead,
> >> to make sure that the test does not modify the original mount namespace.
> >
> > Like this in the child?
> >
> > diff --git a/sysdeps/unix/sysv/linux/tst-mount.c
> b/sysdeps/unix/sysv/linux/tst-mount.c
> > index 502d7e3433..d19d70d42d 100644
> > --- a/sysdeps/unix/sysv/linux/tst-mount.c
> > +++ b/sysdeps/unix/sysv/linux/tst-mount.c
> > @@ -107,7 +107,11 @@ do_test (void)
> >
> >    pid_t pid = xfork ();
> >    if (pid == 0)
> > -    subprocess ();
> > +    {
> > +      if (!support_enter_mount_namespace ())
> > +        FAIL_UNSUPPORTED ("could not enter new mount namespace");
> > +      subprocess ();
> > +    }
>
> Yes, except that you need to change
>
>   xwaitpid (pid, &status, 0);
>   TEST_VERIFY (WIFEXITED (status));
>
> as well, to handle status 77.
>

I was a bit confused by this, as subprocess() already calls
FAIL_UNSUPPORTED (if fsopen sets errno to ENOSYS). WIFEXITED (status)
doesn't distinguish between status 0 or 77 though... So I guess I don't
understand how the test machinery works here.


> I'm not entirely sure the fork is necessary, though.
>

Yes, I don't see why it's there either.

Cheers,
mwh
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/tst-mount.c b/sysdeps/unix/sysv/linux/tst-mount.c
index 502d7e3433..a0b367d9df 100644
--- a/sysdeps/unix/sysv/linux/tst-mount.c
+++ b/sysdeps/unix/sysv/linux/tst-mount.c
@@ -20,6 +20,7 @@ 
 #include <support/check.h>
 #include <support/xunistd.h>
 #include <support/namespace.h>
+#include <support/test-driver.h>
 #include <sys/wait.h>
 #include <sys/mount.h>
 
@@ -104,6 +105,8 @@  static int
 do_test (void)
 {
   support_become_root ();
+  if (!support_can_chroot ())
+    return EXIT_UNSUPPORTED;
 
   pid_t pid = xfork ();
   if (pid == 0)