bug#47007: dcb640f02b broke guix environment --container

bug#47007: dcb640f02b broke guix environment --container

[Jelle Licht]

Hello guix,

I only tried this on x86_64 guix systems, one with Linux kernel 5.11.2,
and also on a Linux-libre kernel 4.14.223.

Running the equivalent of a `git bisect' starting some months back to
today's master, and with the following test to select bad/good bisect revisions:

`./pre-inst-env guix environment --ad-hoc --container --no-cwd --network hello'

leads me to believe that the following commit broke something.

dcb640f02b * file-systems: 'mount-file-system' preserves source flags for bind mounts.

Did this commit expose an existing (but silent) issue, or did it
introduce something new?

For completeness' sake, the output of the guix environment invocation on
'bad' revisions:

--8<---------------cut here---------------start------------->8---
guix environment: error: mount: mount "/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16" on "/tmp/guix-directory.Ji7KNW//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16": Operation not permitted
--8<---------------cut here---------------end--------------->8---

I think kozo[m] on irc was hunting the very same issue as well:
http://logs.guix.gnu.org/guix/2021-03-08.log#035557

Thanks,
 - Jelle

bug#47007: dcb640f02b broke guix environment --container

[Ludovic Courtès]

Hi,

Jelle Licht <jlicht@fsfe.org> skribis:

> I only tried this on x86_64 guix systems, one with Linux kernel 5.11.2,
> and also on a Linux-libre kernel 4.14.223.
>
> Running the equivalent of a `git bisect' starting some months back to
> today's master, and with the following test to select bad/good bisect revisions:
>
> `./pre-inst-env guix environment --ad-hoc --container --no-cwd --network hello'
>
> leads me to believe that the following commit broke something.
>
> dcb640f02b * file-systems: 'mount-file-system' preserves source flags for bind mounts.
>
> Did this commit expose an existing (but silent) issue, or did it
> introduce something new?
>
> For completeness' sake, the output of the guix environment invocation on
> 'bad' revisions:
>
> guix environment: error: mount: mount "/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16" on "/tmp/guix-directory.Ji7KNW//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16": Operation not permitted

Could you run:

  ./pre-inst-env strace -f -o log guix environment --container

and send the ‘log’ file (or the bits around the mount(2) calls)?

Could you also show /proc/mounts?

TIA!

Ludo’.

bug#47007: dcb640f02b broke guix environment --container

[Jelle Licht]

Ludovic Courtès <ludovic.courtes@inria.fr> writes:

> Hi,
>
> Jelle Licht <jlicht@fsfe.org> skribis:
>
>> I only tried this on x86_64 guix systems, one with Linux kernel 5.11.2,
>> and also on a Linux-libre kernel 4.14.223.
>>
>> Running the equivalent of a `git bisect' starting some months back to
>> today's master, and with the following test to select bad/good bisect revisions:
>>
>> `./pre-inst-env guix environment --ad-hoc --container --no-cwd --network hello'

This was intended to be, in case that was unclear:
`./pre-inst-env guix environment --ad-hoc --container --no-cwd --network hello -- hello'

>> [snip]
>> guix environment: error: mount: mount "/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16" on "/tmp/guix-directory.Ji7KNW//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16": Operation not permitted
>
> Could you run:
>
>   ./pre-inst-env strace -f -o log guix environment --container
>
> and send the ‘log’ file (or the bits around the mount(2) calls)?

I ran it using a `guix pull'ed guix on the master branch, commit
c4195a10783. I hope that's fine.

There you go:
--8<---------------cut here---------------start------------->8---
26123 stat("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0
26123 mkdir("/tmp", 0777)               = -1 EEXIST (File exists)
26123 mkdir("/tmp/guix-directory.9IH6jJ", 0777) = -1 EEXIST (File exists)
26123 mkdir("/tmp/guix-directory.9IH6jJ/gnu", 0777) = 0
26123 mkdir("/tmp/guix-directory.9IH6jJ/gnu/store", 0777) = 0
26123 mkdir("/tmp/guix-directory.9IH6jJ/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0777) = 0
26123 mount("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", "/tmp/guix-directory.9IH6jJ//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0x15a2060, MS_RDONLY|MS_BIND|MS_RELATIME, NULL) = 0
26123 mount("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", "/tmp/guix-directory.9IH6jJ//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0x15a20f0, MS_RDONLY|MS_REMOUNT|MS_BIND|MS_RELATIME, NULL) = -1 EPERM (Operation not permitted)
--8<---------------cut here---------------end--------------->8---

> Could you also show /proc/mounts?

--8<---------------cut here---------------start------------->8---
none /proc proc rw,relatime 0 0
none /dev devtmpfs rw,relatime,size=3944948k,nr_inodes=986237,mode=755 0 0
none /sys sysfs rw,relatime 0 0
/dev/sda1 / ext4 rw,relatime,data=ordered 0 0
none /dev/pts devpts rw,relatime,gid=996,mode=620,ptmxmode=000 0 0
none /sys/kernel/debug debugfs rw,relatime 0 0
tmpfs /dev/shm tmpfs rw,nosuid,nodev,relatime 0 0
/dev/sda1 /gnu/store ext4 ro,noatime,data=ordered 0 0
none /run/systemd tmpfs rw,nosuid,nodev,noexec,relatime,mode=755 0 0
none /run/user tmpfs rw,nosuid,nodev,noexec,relatime,mode=755 0 0
cgroup /sys/fs/cgroup tmpfs rw,relatime 0 0
cgroup /sys/fs/cgroup/elogind cgroup rw,relatime,name=elogind 0 0
cgroup /sys/fs/cgroup/cpuset cgroup rw,relatime,cpuset 0 0
cgroup /sys/fs/cgroup/cpu cgroup rw,relatime,cpu 0 0
cgroup /sys/fs/cgroup/cpuacct cgroup rw,relatime,cpuacct 0 0
cgroup /sys/fs/cgroup/memory cgroup rw,relatime,memory 0 0
cgroup /sys/fs/cgroup/devices cgroup rw,relatime,devices 0 0
cgroup /sys/fs/cgroup/freezer cgroup rw,relatime,freezer 0 0
cgroup /sys/fs/cgroup/blkio cgroup rw,relatime,blkio 0 0
cgroup /sys/fs/cgroup/perf_event cgroup rw,relatime,perf_event 0 0
cgroup /sys/fs/cgroup/pids cgroup rw,relatime,pids 0 0
none /var/cache/fontconfig tmpfs ro,relatime 0 0
cgroup2 /sys/fs/cgroup/unified cgroup2 rw,nosuid,nodev,noexec,relatime,nsdelegate 0 0
tmpfs /run/user/1001 tmpfs rw,nosuid,nodev,relatime,size=790824k,mode=700,uid=1001,gid=997 0 0
--8<---------------cut here---------------end--------------->8---

FWIW, I only have this issue on Guix System: on my Ubuntu 20.04.2 things
JustWork(tm)

Thanks for taking a look,
 - Jelle

bug#47007: dcb640f02b broke guix environment --container

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 3226 bytes --]

Hi,

Jelle Licht <jlicht@fsfe.org> skribis:

> There you go:
>
> 26123 stat("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0
> 26123 mkdir("/tmp", 0777)               = -1 EEXIST (File exists)
> 26123 mkdir("/tmp/guix-directory.9IH6jJ", 0777) = -1 EEXIST (File exists)
> 26123 mkdir("/tmp/guix-directory.9IH6jJ/gnu", 0777) = 0
> 26123 mkdir("/tmp/guix-directory.9IH6jJ/gnu/store", 0777) = 0
> 26123 mkdir("/tmp/guix-directory.9IH6jJ/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0777) = 0
> 26123 mount("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", "/tmp/guix-directory.9IH6jJ//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0x15a2060, MS_RDONLY|MS_BIND|MS_RELATIME, NULL) = 0
> 26123 mount("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", "/tmp/guix-directory.9IH6jJ//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0x15a20f0, MS_RDONLY|MS_REMOUNT|MS_BIND|MS_RELATIME, NULL) = -1 EPERM (Operation not permitted)

On my machine (Guix System), the same thing works:

--8<---------------cut here---------------start------------->8---
19524 mkdir("/tmp/guix-directory.Js9BMV/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0777) = 0
19524 mount("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", "/tmp/guix-directory.Js9BMV//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0x153e200, MS_RDONLY|MS_BIND|MS_RELATIME, NULL) = 0
19524 mount("/gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", "/tmp/guix-directory.Js9BMV//gnu/store/mmhimfwmmidf09jw1plw3aw1g1zn2nkh-bash-static-5.0.16", 0x153e0f0, MS_RDONLY|MS_REMOUNT|MS_BIND|MS_RELATIME, NULL) = 0
--8<---------------cut here---------------end--------------->8---

> /dev/sda1 / ext4 rw,relatime,data=ordered 0 0
> none /dev/pts devpts rw,relatime,gid=996,mode=620,ptmxmode=000 0 0
> none /sys/kernel/debug debugfs rw,relatime 0 0
> tmpfs /dev/shm tmpfs rw,nosuid,nodev,relatime 0 0
> /dev/sda1 /gnu/store ext4 ro,noatime,data=ordered 0 0

Here you have “ro,noatime,data=ordered” and I have “ro,relatime”.

At any rate, it seems the bug is that in your case it should be picking
MS_NOATIME and not MS_RELATIME, which is must have copied from /.

Indeed, the trick to find the right device in that commit is wrong
because it finds two devices in this case: root and the /gnu/store bind
mount.

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,use(guix build syscalls)
scheme@(guile-user)> (filter (lambda (m)
			       (= (mount-device-number m) (stat:dev (lstat "/gnu/store"))))
			     (mounts))
$13 = (#<<mount> devno: 64768 source: "/dev/dm-0" point: "/" type: "ext4" options: "rw,relatime"> #<<mount> devno: 64768 source: "/dev/dm-0" point: "/gnu/store" type: "ext4" options: "ro,relatime">)
--8<---------------cut here---------------end--------------->8---

In my case it’s fine because they have the same options, but in your
case it breaks because they have different options and it picks the
wrong one.

Could you try the attached patch?

Thanks,
Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1157 bytes --]

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index aca4aad848..64c2557a43 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -920,11 +920,9 @@ corresponds to the symbols listed in FLAGS."
                           ;; MS_REMOUNT call below fails with EPERM.
                           ;; See <https://bugs.gnu.org/46292>
                           (if (memq 'bind-mount (file-system-flags fs))
-                              (or (and=> (find (let ((devno (stat:dev
-                                                             (lstat source))))
-                                                 (lambda (mount)
-                                                   (= (mount-device-number mount)
-                                                      devno)))
+                              (or (and=> (find (lambda (mount)
+                                                 (= (mount-point mount)
+                                                    source))
                                                (mounts))
                                          mount-flags)
                                   0)

bug#47007: dcb640f02b broke guix environment --container

[Martin via Bug reports for GNU Guix]

Hello,

On 3/9/21 12:18 PM, Jelle Licht wrote:
> FWIW, I only have this issue on Guix System: on my Ubuntu 20.04.2 things
> JustWork(tm)
>
> Thanks for taking a look,
>   - Jelle
>
I can confirm similar issues on my Guix Systems from below:

   canonical file name: /gnu/store/70rfnrwxxwmipvnfxg54bwyz5rkkl4ps-system
   label: GNU with Linux-Libre 5.11.4
   configuration file: 
/gnu/store/pl31mxphv6hx36ab07j7i1wnv0l4pqp5-configuration.scm

   canonical file name: /gnu/store/8cny9lkvclz1wb06rvz6gywlb9nqqb7r-system
   label: GNU with Linux-Libre 5.11.3
   configuration file: 
/gnu/store/pl31mxphv6hx36ab07j7i1wnv0l4pqp5-configuration.scm

However the Guix System on the same hardware with the same basic 
configuration but using linux-libre-LTS JustWork(tm) fine:

   canonical file name: /gnu/store/p18smv0h2yygdhnc4gxhzjkck9hasmf3-system
   label: GNU with Linux-Libre 5.10.13
   configuration file: 
/gnu/store/pl31mxphv6hx36ab07j7i1wnv0l4pqp5-configuration.scm


Kind regards!
Martin

bug#47007: dcb640f02b broke guix environment --container

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

Here’s a more sensible patch for you to try.  This time it should
correctly determine the necessary mount flags based on statfs(2) info.

Could you apply it and report back?

TIA!

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 4431 bytes --]

diff --git a/gnu/build/file-systems.scm b/gnu/build/file-systems.scm
index aca4aad848..304805db62 100644
--- a/gnu/build/file-systems.scm
+++ b/gnu/build/file-systems.scm
@@ -920,14 +920,8 @@ corresponds to the symbols listed in FLAGS."
                           ;; MS_REMOUNT call below fails with EPERM.
                           ;; See <https://bugs.gnu.org/46292>
                           (if (memq 'bind-mount (file-system-flags fs))
-                              (or (and=> (find (let ((devno (stat:dev
-                                                             (lstat source))))
-                                                 (lambda (mount)
-                                                   (= (mount-device-number mount)
-                                                      devno)))
-                                               (mounts))
-                                         mount-flags)
-                                  0)
+                              (statfs-flags->mount-flags
+                               (file-system-mount-flags (statfs source)))
                               0)))
          (options (file-system-options fs)))
     (when (file-system-check? fs)
diff --git a/guix/build/syscalls.scm b/guix/build/syscalls.scm
index 552343a481..6ed11a0d69 100644
--- a/guix/build/syscalls.scm
+++ b/guix/build/syscalls.scm
@@ -82,6 +82,21 @@
             file-system-fragment-size
             file-system-mount-flags
             statfs
+
+            ST_RDONLY
+            ST_NOSUID
+            ST_NODEV
+            ST_NOEXEC
+            ST_SYNCHRONOUS
+            ST_MANDLOCK
+            ST_WRITE
+            ST_APPEND
+            ST_IMMUTABLE
+            ST_NOATIME
+            ST_NODIRATIME
+            ST_RELATIME
+            statfs-flags->mount-flags
+
             free-disk-space
             device-in-use?
             add-to-entropy-count
@@ -754,6 +769,56 @@ fdatasync(2) on the underlying file descriptor."
 (define-syntax fsword                             ;fsword_t
   (identifier-syntax long))
 
+(define linux? (string-contains %host-type "linux-gnu"))
+
+(define-syntax define-statfs-flags
+  (syntax-rules (linux hurd)
+    "Define the statfs mount flags."
+    ((_ (name (linux linux-value) (hurd hurd-value)) rest ...)
+     (begin
+       (define name
+         (if linux? linux-value hurd-value))
+       (define-statfs-flags rest ...)))
+    ((_ (name value) rest ...)
+     (begin
+       (define name value)
+       (define-statfs-flags rest ...)))
+    ((_) #t)))
+
+(define-statfs-flags                              ;<bits/statfs.h>
+  (ST_RDONLY      1)
+  (ST_NOSUID      2)
+  (ST_NODEV       (linux 4) (hurd 0))
+  (ST_NOEXEC      8)
+  (ST_SYNCHRONOUS 16)
+  (ST_MANDLOCK    (linux 64) (hurd 0))
+  (ST_WRITE       (linux 128) (hurd 0))
+  (ST_APPEND      (linux 256) (hurd 0))
+  (ST_IMMUTABLE   (linux 512) (hurd 0))
+  (ST_NOATIME     (linux 1024) (hurd 32))
+  (ST_NODIRATIME  (linux 2048) (hurd 0))
+  (ST_RELATIME    (linux 4096) (hurd 64)))
+
+(define (statfs-flags->mount-flags flags)
+  "Convert FLAGS, a logical or of ST_* constants as returned by
+'file-system-mount-flags', to the corresponding logical or of MS_* constants."
+  (letrec-syntax ((match-flags (syntax-rules (=>)
+                                 ((_ (statfs => mount) rest ...)
+                                  (logior (if (zero? (logand flags statfs))
+                                              0
+                                              mount)
+                                          (match-flags rest ...)))
+                                 ((_)
+                                  0))))
+    (match-flags
+     (ST_RDONLY     => MS_RDONLY)
+     (ST_NOSUID     => MS_NOSUID)
+     (ST_NODEV      => MS_NODEV)
+     (ST_NOEXEC     => MS_NOEXEC)
+     (ST_NOATIME    => MS_NOATIME)
+     (ST_NODIRATIME => 0)                         ;FIXME
+     (ST_RELATIME   => MS_RELATIME))))
+
 (define-c-struct %statfs                          ;<bits/statfs.h>
   sizeof-statfs                                   ;slightly overestimated
   file-system
@@ -769,7 +834,7 @@ fdatasync(2) on the underlying file descriptor."
   (identifier       (array int 2))
   (name-length      fsword)
   (fragment-size    fsword)
-  (mount-flags      fsword)
+  (mount-flags      fsword)                       ;ST_*
   (spare            (array fsword 4)))
 
 (define statfs

bug#47007: dcb640f02b broke guix environment --container

[Andreas Enge]

Hello,

incidentally I stumbled upon the same problem as Jelle today.

Am Tue, Mar 09, 2021 at 05:17:10PM +0100 schrieb Ludovic Courtès:
> Could you try the attached patch?

It raises an error:
$ ./pre-inst-env guix environment -C --ad-hoc coreutils
Backtrace:
In ice-9/boot-9.scm:
  1736:10 17 (with-exception-handler _ _ #:unwind? _ # _)
In unknown file:
          16 (apply-smob/0 #<thunk 7f421c659520>)
In ice-9/boot-9.scm:
    718:2 15 (call-with-prompt _ _ #<procedure default-prompt-handle…>)
In ice-9/eval.scm:
    619:8 14 (_ #(#(#<directory (guile-user) 7f421c65cc80>)))
In guix/ui.scm:
  2164:12 13 (run-guix-command _ . _)
In ice-9/boot-9.scm:
  1736:10 12 (with-exception-handler _ _ #:unwind? _ # _)
  1731:15 11 (with-exception-handler #<procedure 7f420c8a2db0 at ic…> …)
  1736:10 10 (with-exception-handler _ _ #:unwind? _ # _)
In guix/store.scm:
   636:37  9 (thunk)
   1305:8  8 (call-with-build-handler _ _)
   1305:8  7 (call-with-build-handler #<procedure 7f420c8a26f0 at g…> …)
In guix/status.scm:
    780:4  6 (call-with-status-report _ _)
In guix/scripts/environment.scm:
   768:14  5 (_)
In guix/store.scm:
  2066:24  4 (run-with-store #<store-connection 256.99 7f420c7448c0> …)
In guix/scripts/environment.scm:
    523:8  3 (_ _)
In gnu/build/linux-container.scm:
   327:16  2 (call-with-container (#<<file-system> device: "none…> …) …)
In ice-9/boot-9.scm:
  1669:16  1 (raise-exception _ #:continuable? _)
  1669:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1669:16: In procedure raise-exception:
In procedure =: Wrong type argument in position 1: "/"

Andreas

bug#47007: dcb640f02b broke guix environment --container

[Andreas Enge]

Am Tue, Mar 09, 2021 at 10:00:30PM +0100 schrieb Ludovic Courtès:
> Here’s a more sensible patch for you to try.  This time it should
> correctly determine the necessary mount flags based on statfs(2) info.
> Could you apply it and report back?

This one works like a charm, thanks a lot!

Andreas

bug#47007: dcb640f02b broke guix environment --container

[Jelle Licht]

Ludovic Courtès <ludo@gnu.org> writes:

> Here’s a more sensible patch for you to try.  This time it should
> correctly determine the necessary mount flags based on statfs(2) info.
>
> Could you apply it and report back?

I can confirm that it does what it says on the tin :-).

Thanks again!
 - Jelle

bug#47007: dcb640f02b broke guix environment --container

[Ludovic Courtès]

Hi Andreas,

Andreas Enge <andreas@enge.fr> skribis:

> incidentally I stumbled upon the same problem as Jelle today.
>
> Am Tue, Mar 09, 2021 at 05:17:10PM +0100 schrieb Ludovic Courtès:
>> Could you try the attached patch?

Could you instead try the latest patch?

  https://issues.guix.gnu.org/47007#6

Thanks!

Ludo’.

bug#47007: dcb640f02b broke guix environment --container

[Andreas Enge]

Hello,

Am Wed, Mar 10, 2021 at 12:25:05PM +0100 schrieb Ludovic Courtès:
> Could you instead try the latest patch?
>   https://issues.guix.gnu.org/47007#6

it looks like my reply was missed, I tried this patch, and it solves the
problem for me, as for Jelle. So please push.

Thanks!

Andreas

bug#47007: dcb640f02b broke guix environment --container

[Ludovic Courtès]

Hi!

Andreas Enge <andreas@enge.fr> skribis:

> Am Wed, Mar 10, 2021 at 12:25:05PM +0100 schrieb Ludovic Court鑚:
>> Could you instead try the latest patch?
>>   https://issues.guix.gnu.org/47007#6
>
> it looks like my reply was missed, I tried this patch, and it solves the
> problem for me, as for Jelle. So please push.

Done as b665dd4a9902b5722b9e06fd89c203e2221b19e0, thank you!  :-)

Ludo’.