umount -r broken due to "mountinfo unnecessary"

umount -r broken due to "mountinfo unnecessary"

[Krzysztof Olędzki]

Hi Karel,

I noticed that "umount -r" does not work on my system for filesystems other than root:

# umount -r /usr
umount: /usr: target is busy.

Yes, umount first tries the umount2 syscall, so "target is busy" is very expected, but there is no follow-up attempt to remount fs as read-only:

umount2("/usr", 0)                      = -1 EBUSY (Device or resource busy)
openat(AT_FDCWD, "/usr/share/locale/locale.alias", O_RDONLY|O_CLOEXEC) = 3
newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=2998, ...}, AT_EMPTY_PATH) = 0
read(3, "# Locale name alias data base.\n#"..., 4096) = 2998
read(3, "", 4096)                       = 0
close(3)                                = 0
openat(AT_FDCWD, "/usr/share/locale/en_US/LC_MESSAGES/util-linux.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share/locale/en/LC_MESSAGES/util-linux.mo", O_RDONLY) = -1 ENOENT (No such file or directory)
write(2, "umount: ", 8umount: )                 = 8
write(2, "/usr: target is busy.", 21/usr: target is busy.)   = 21
write(2, "\n", 1
)                       = 1
dup(1)                                  = 3
close(3)                                = 0
dup(2)                                  = 3
close(3)                                = 0
exit_group(32)                          = ?
+++ exited with 32 +++

Looking at the code, "try remount read-only" section in do_umount() from libmount/src/context_umount.c seems to only be called if all these conditions are met:
 - rc < 0
 - cxt->syscall_status == -EBUSY
 - mnt_context_is_rdonly_umount(cxt)
 - src is not NULL

I added some debug code to do_umount() to see why it fails:
        DBG(CXT, ul_debugobj(cxt, "rc=%d, cxt->syscall_status=%d, mnt_context_is_rdonly_umount=%d, src=%s",
        rc, cxt->syscall_status, mnt_context_is_rdonly_umount(cxt), src));

Result:
17555: libmount:      CXT: [0x5638caf185b0]: rc=-1, cxt->syscall_status=-16, mnt_context_is_rdonly_umount=1, src=(null)

With this, I noticed the additional hint in the log coming from lookup_umount_fs_by_statfs():

17555: libmount:      CXT: [0x5638caf185b0]: umount: lookup FS
17555: libmount:      CXT: [0x5638caf185b0]:  lookup by statfs
17555: libmount:      CXT: [0x5638caf185b0]:   trying fstatfs()
17555: libmount:      CXT: [0x5638caf185b0]:   umount: disabling mountinfo

Adding "DBG(CXT, mnt_fs_print_debug(cxt->fs, stderr));" in do_umount() confirmed my assumptions:

19114: libmount:      CXT: ------ fs:
source: (null)
target: /usr
fstype: ext4

The problem seems to be that lookup_umount_fs() first calls lookup_umount_fs_by_statfs() and when it succeeds, we only get partial information - without the source.

Commenting the following section in lookup_umount_fs():

//      rc = lookup_umount_fs_by_statfs(cxt, tgt);
//      if (rc <= 0)
//              goto done;

... allows this to work and /usr gets re-mounted ro:

23821: libmount:   UPDATE: ------ fs:
source: /dev/mapper/VG0-usr
target: /usr
fstype: ext3
optstr: rw,defaults,data=journal,nodev,remount
VFS-optstr: rw,nodev,remount
FS-opstr: data=journal
user-optstr: defaults

umount2("/usr", 0)                      = -1 EBUSY (Device or resource busy)
mount("/dev/mapper/VG0-usr", "/usr", NULL, MS_RDONLY|MS_REMOUNT, NULL) = 0

Clearly this is not the right fix, but perhaps something like this would be correct:

@@ -275,6 +275,7 @@
      || mnt_context_is_lazy(cxt)
      || mnt_context_is_nocanonicalize(cxt)
      || mnt_context_is_loopdel(cxt)
+     || mnt_context_is_rdonly_umount(cxt)
      || mnt_safe_stat(tgt, &st) != 0 || !S_ISDIR(st.st_mode)
      || has_utab_entry(cxt, tgt))
       return 1; /* not found */

I wonder if we just missed the mnt_context_is_rdonly_umount case in https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=6a52473ecd877227f6f7da2b95da0b51593ffec1?

Thanks,
 Krzysztof

Re: umount -r broken due to "mountinfo unnecessary"

[Karel Zak]

On Thu, Apr 18, 2024 at 01:00:56AM -0700, Krzysztof Olędzki wrote:
> Clearly this is not the right fix, but perhaps something like this would be correct:
> 
> @@ -275,6 +275,7 @@
>       || mnt_context_is_lazy(cxt)
>       || mnt_context_is_nocanonicalize(cxt)
>       || mnt_context_is_loopdel(cxt)
> +     || mnt_context_is_rdonly_umount(cxt)
>       || mnt_safe_stat(tgt, &st) != 0 || !S_ISDIR(st.st_mode)
>       || has_utab_entry(cxt, tgt))
>        return 1; /* not found */
> 
> I wonder if we just missed the mnt_context_is_rdonly_umount case in https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=6a52473ecd877227f6f7da2b95da0b51593ffec1?

Yes :-)

Nice debugging work, thanks!

I will most likely use the bugfix that you have suggested. The
long-term solution should be to use the new mount API for
context_umount.c as well, as it allows for reconfiguring the
mountpoint without having to worry about the source.

  attr.attr_set = MOUNT_ATTR_RDONLY;

  fd = open_tree(AT_FDCWD, target, 0);
  mount_setattr(fd, "", AT_EMPTY_PATH, &attr, sizeof(attr));
  close(fd);

However, we need to implement it in a backwardly compatible way for
cases where there is the new API unsupported.

 Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: umount -r broken due to "mountinfo unnecessary"

[Krzysztof Olędzki]

On 22.04.2024 at 04:43, Karel Zak wrote:
> On Thu, Apr 18, 2024 at 01:00:56AM -0700, Krzysztof Olędzki wrote:
>> Clearly this is not the right fix, but perhaps something like this would be correct:
>>
>> @@ -275,6 +275,7 @@
>>       || mnt_context_is_lazy(cxt)
>>       || mnt_context_is_nocanonicalize(cxt)
>>       || mnt_context_is_loopdel(cxt)
>> +     || mnt_context_is_rdonly_umount(cxt)
>>       || mnt_safe_stat(tgt, &st) != 0 || !S_ISDIR(st.st_mode)
>>       || has_utab_entry(cxt, tgt))
>>        return 1; /* not found */
>>
>> I wonder if we just missed the mnt_context_is_rdonly_umount case in https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=6a52473ecd877227f6f7da2b95da0b51593ffec1?
> 
> Yes :-)
> 
> Nice debugging work, thanks!

Thanks!

> I will most likely use the bugfix that you have suggested. The
> long-term solution should be to use the new mount API for
> context_umount.c as well, as it allows for reconfiguring the
> mountpoint without having to worry about the source.
> 
>   attr.attr_set = MOUNT_ATTR_RDONLY;
> 
>   fd = open_tree(AT_FDCWD, target, 0);
>   mount_setattr(fd, "", AT_EMPTY_PATH, &attr, sizeof(attr));
>   close(fd);
> 
> However, we need to implement it in a backwardly compatible way for
> cases where there is the new API unsupported.

Right, and it seems it will need to live for many years to come given mount_setattr got added in Linux 5.12 with xfs/ext4/FAT support only and several other filesystems added in 5.15/5.18/5.19.

BTW: we probably did not notice it for that long because Linux kernel has a special handling for root filesystem, where failed umount is internally handled as remount read-only, and it is not very common to have /usr on a separate filesystem.

Thanks,
 Krzysztof

Re: umount -r broken due to "mountinfo unnecessary"

[Karel Zak]

On Thu, Apr 18, 2024 at 01:00:56AM -0700, Krzysztof Olędzki wrote:
> I noticed that "umount -r" does not work on my system for filesystems other than root:

Fixed: https://github.com/util-linux/util-linux/pull/2989

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: umount -r broken due to "mountinfo unnecessary"

[Krzysztof Olędzki]

On 23.04.2024 at 01:33, Karel Zak wrote:
> On Thu, Apr 18, 2024 at 01:00:56AM -0700, Krzysztof Olędzki wrote:
>> I noticed that "umount -r" does not work on my system for filesystems other than root:
> 
> Fixed: https://github.com/util-linux/util-linux/pull/2989

Perfect, thanks!

When is the next release planned? Also, do you expect a backport to stable/v2.39 and v2.39.5?

Thanks,
 Krzysztof 

Re: umount -r broken due to "mountinfo unnecessary"

[Karel Zak]

On Tue, Apr 23, 2024 at 07:35:18AM -0700, Krzysztof Olędzki wrote:
> When is the next release planned?

I plan to release v2.40.1 next week.

> Also, do you expect a backport to stable/v2.39 and v2.39.5?

I have cherry-picked it to stable/v2.39. Currently, I have no plans
for v2.39.5.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: umount -r broken due to "mountinfo unnecessary"

[Krzysztof Olędzki]

On 24.04.2024 at 03:15, Karel Zak wrote:
> On Tue, Apr 23, 2024 at 07:35:18AM -0700, Krzysztof Olędzki wrote:
>> When is the next release planned?
> 
> I plan to release v2.40.1 next week.

Thanks you! However, the patch is still missing in the stable/v2.40 branch. Sorry for the noise if this WAI, just don't want it to be accidentally missed.

> 
>> Also, do you expect a backport to stable/v2.39 and v2.39.5?
> 
> I have cherry-picked it to stable/v2.39. Currently, I have no plans
> for v2.39.5.

Understood, thanks!

BTW: v2.39.4 tag seems to be missing? https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tag/?h=v2.39.4


Krzysztof

Re: umount -r broken due to "mountinfo unnecessary"

[Karel Zak]

On Tue, Apr 30, 2024 at 09:31:54PM -0700, Krzysztof Olędzki wrote:
> On 24.04.2024 at 03:15, Karel Zak wrote:
> > On Tue, Apr 23, 2024 at 07:35:18AM -0700, Krzysztof Olędzki wrote:
> >> When is the next release planned?
> > 
> > I plan to release v2.40.1 next week.
> 
> Thanks you! However, the patch is still missing in the stable/v2.40
> branch. Sorry for the noise if this WAI, just don't want it to be
> accidentally missed.

Cherry-picked to the branch now.

> BTW: v2.39.4 tag seems to be missing? https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/tag/?h=v2.39.4

Ah, fixed now.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com