Hello Christian,

On 8/10/21 4:32 PM, Christian Brauner wrote:

On Tue, Aug 10, 2021 at 03:38:00AM +0200, Michael Kerrisk (man-pages) wrote:

quoted

Hi Christian,

Thanks for the very nice manual page that you wrote. I have

Thank you!

quoted

made a large number of (mostly trivial) edits. If you could
read the page closely, to check that I introduced no errors,
I would appreciate it.

Happy to!

Thanks for the feedback. I've made some changes, and pushed to Git.

There's still a few open questions. Please see "????" below.

quoted

I have various questions below, marked ???. Could you please take
a look at these, and I will then make further edits based on your
answers.

I've answered all questions, I think. Feel free to just reformulate
where my suggestions weren't adequate. Since most things you ask about
are minor adaptions there's no need from my end for you to resend with
those reformulations. You can just make them directly. :) I'll peruse
the man-pages git repo anyway after you apply them and will send changes
if I spot issues.

Thank you for the review!
Christian

quoted

The current version of the page is already pushed to the man-pages
Git repo.

quoted

  MOUNT_SETATTR(2)      Linux Programmer's Manual     MOUNT_SETATTR(2)

  NAME
      mount_setattr - change mount properties of a mount or mount

???
s/mount properties/properties ?

(Just bcause more concise.)

Sounds good.

Done.

quoted

quoted

      tree

  SYNOPSIS
      #include <linux/fcntl.h> /* Definition of AT_* constants */
      #include <linux/mount.h> /* Definition of MOUNT_ATTR_* constants */
      #include <sys/syscall.h> /* Definition of SYS_* constants */
      #include <unistd.h>

      int syscall(SYS_mount_setattr, int dirfd, const char *path,
              unsigned int flags, struct mount_attr *attr, size_t size);

      Note: glibc provides no wrapper for mount_setattr(),
      necessitating the use of syscall(2).

  DESCRIPTION

[...]

quoted

quoted

      The size argument should usually be specified as
      sizeof(struct mount_attr).  However, if the caller does not
      intend to make use of features that got introduced after the
      initial version of struct mount_attr, it is possible to pass
      the size of the initial struct together with the larger
      struct.  This allows the kernel to not copy later parts of
      the struct that aren't used anyway.  With each extension that
      changes the size of struct mount_attr, the kernel will expose
      a definition of the form MOUNT_ATTR_SIZE_VERnumber.  For
      example, the macro for the size of the initial version of
      struct mount_attr is MOUNT_ATTR_SIZE_VER0.

???
I think I understand the above paragraph, but I wonder if it could
be improved a little. The general principle is that one can always
pass the size of an earlier, smaller structure to the kernel, right?

Yes.

quoted

My point is that it need not be the size of the initial structure,
right? So, I wonder whether a little rewording might be need above.

Yes, the initial structure size is just an example because that will be
very common.

quoted

What do you think?

Sure, I'm proposing something here but please, fell free to reformulate
or come up with something completely new:

	[...]
	However, if the caller is using a kernel that supports an
	extended struct mount_attr but the caller does not intend to
	make use of these features they can pass the size of an earlier
	version of the struct together with the extended structure.
	[...]

Perfect! I took that text pretty much exactly as you gave it.

[...]

quoted

quoted

      The attr_set and attr_clr members are used to specify the
      mount properties that are supposed to be set or cleared for a
      mount or mount tree.  Flags set in attr_set enable a property
      on a mount or mount tree, and flags set in attr_clr remove a
      property from a mount or mount tree.

      When changing mount properties, the kernel will first clear
      the flags specified in the attr_clr field, and then set the
      flags specified in the attr_set field:

???
I find the following example a bit confusing. See below.

quoted

          struct mount_attr attr = {
              .attr_clr = MOUNT_ATTR_NOEXEC | MOUNT_ATTR_NODEV,
              .attr_set = MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID,
          };

???
I *think* that what you are trying to show is that the above initializer
resuts in the equivalent of the following code. Is that correct? If so, 
I think the text needs some work to make this clearer. Let me know.

Yes, exactly. Feel free to remove that code and just explain it in text
if that's better.

I've done some rewording to say that the code snippet shows
the effect of the initializer.

[...]

quoted

quoted

  RETURN VALUE
      On success, mount_setattr() returns zero.  On error, -1 is
      returned and errno is set to indicate the cause of the error.

  ERRORS

[...]

quoted

quoted

      EINVAL A valid file descriptor value was specified in
             userns_fd, but the file descriptor wasn't a namespace
             file descriptor or did not refer to a user namespace.

???
Could the above not be simplified to

      EINVAL A valid file descriptor value was specified in
             userns_fd, but the file descriptor did not refer
             to a user namespace.

Sounds good.

quoted

?

Done.

quoted

quoted

      EINVAL The underlying filesystem does not support ID-mapped
             mounts.

      EINVAL The mount that is to be ID mapped is not a
             detached/anonymous mount; that is, the mount is

???
What is a the distinction between "detached" and "anonymous"?
Or do you mean them to be synonymous? If so, then let's use
just one term, and I think "detached" is preferable.

Yes, they are synonymous here. I list both because detached can
potentially be confusing. A detached mount is a mount that has not been
visible in the filesystem. But if you attached it an then unmount it
right after and keep the fd for the mountpoint open it's a detached
mount purely on a natural language level, I'd argue. But it's not a
detached mount from the kernel's view anymore because it has been
exposed in the filesystem and is thus not detached anymore.
But I do prefer "detached" to "anonymous" and that confusion is very
unlikely to occur.

Thanks. I made it "detached". Elsewhere, the page already explains
that a detached mount is one that:

          must have been created by calling open_tree(2) with the
          OPEN_TREE_CLONE flag and it must not already have been
          visible in the filesystem.

Which seems a fine explanation. 

????
But, just a thought... "visible in the filesystem" seems not quite accurate. 
What you really mean I guess is that it must not already have been
/visible in the filesystem hierarchy/previously mounted/something else/,
right?

quoted

quoted

             already visible in the filesystem.

[...]

quoted

quoted

      EPERM  An already ID-mapped mount was supposed to be ID
             mapped.

???
Better:
    An attempt was made to add an ID mapping to a mount that is already
    ID mapped.

Sounds good.

quoted

?

Done.

[...]

quoted

quoted

  NOTES
  ID-mapped mounts
      Creating an ID-mapped mount makes it possible to change the
      ownership of all files located under a mount.  Thus, ID-
      mapped mounts make it possible to change ownership in a
      temporary and localized way.  It is a localized change
      because ownership changes are restricted to a specific mount.

???
Would it be clearer to say something like:

    It is a localized change because ownership changes are
    visible only via a specific mount.
?

Sounds good.

Done.

[...]

quoted

quoted

      The following conditions must be met in order to create an
      ID-mapped mount:

      •  The caller must have the CAP_SYS_ADMIN capability in the
         initial user namespace.

      •  The filesystem must be mounted in the initial user
         namespace.

???
Should this rather be written as:
 
     The filesystem must be mounted in a mount namespace 
     that is owned by the initial user namespace.

Sounds good.

Done.

quoted

quoted

      •  The underlying filesystem must support ID-mapped mounts.
         Currently, the xfs(5), ext4(5), and FAT filesystems
         support ID-mapped mounts with more filesystems being
         actively worked on.

      •  The mount must not already be ID-mapped.  This also
         implies that the ID mapping of a mount cannot be altered.

      •  The mount must be a detached/anonymous mount; that is, it

???
See the above questionon "detached" vs "anonymous"

Yes, please use "detached" only.

Done.

quoted

quoted

         must have been created by calling open_tree(2) with the
         OPEN_TREE_CLONE flag and it must not already have been
         visible in the filesystem.

      ID mappings can be created for user IDs, group IDs, and
      project IDs.  An ID mapping is essentially a mapping of a
      range of user or group IDs into another or the same range of
      user or group IDs.  ID mappings are usually written as three
      numbers either separated by white space or a full stop.  The
      first two numbers specify the starting user or group ID in
      each of the two user namespaces.  The third number specifies
      the range of the ID mapping.  For example, a mapping for user
      IDs such as 1000:1001:1 would indicate that user ID 1000 in
      the caller's user namespace is mapped to user ID 1001 in its
      ancestor user namespace.  Since the map range is 1, only user
      ID 1000 is mapped.

???
The details above seem wrong. When writing to map files, the
fields must be white-space separated, AFAIK. But above you mention
"full stops" and also show an example using colons (:). Those
both seem wrong and confusing. Am I missing something?

This is more about notational conventions that exist and not about how
they are actually written. That's something I'm not touching on here as
it doesn't belong on this manpage. But feel free to only mention spaces.

Thanks for the explanation. In this context though, this could mislead
the reader, so I've removed mention of "full stop" and ":".

quoted

quoted

      It is possible to specify up to 340 ID mappings for each ID
      mapping type.  If any user IDs or group IDs are not mapped,
      all files owned by that unmapped user or group ID will appear
      as being owned by the overflow user ID or overflow group ID
      respectively.

      Further details and instructions for setting up ID mappings
      can be found in the user_namespaces(7) man page.

      In the common case, the user namespace passed in userns_fd
      together with MOUNT_ATTR_IDMAP in attr_set to create an ID-
      mapped mount will be the user namespace of a container.  In
      other scenarios it will be a dedicated user namespace
      associated with a user's login session as is the case for
      portable home directories in systemd-homed.service(8)).  It
      is also perfectly fine to create a dedicated user namespace
      for the sake of ID mapping a mount.

I forgot to mention it earlier, but the following text on the
rationale for ID-mapped mounts is what turns this from a good 
manual page into a great manual page. Thank you for including it.

quoted

quoted

      ID-mapped mounts can be useful in the following and a variety
      of other scenarios:

      •  Sharing files between multiple users or multiple machines,

???
s/Sharing files/Sharing filesystems/ ?

[1]: But work. But feel free to use "sharing filesystems".

s/But/Both/

I made it "Sharing files or filesystsms"

quoted

quoted

         especially in complex scenarios.  For example, ID-mapped
         mounts are used to implement portable home directories in
         systemd-homed.service(8), where they allow users to move
         their home directory to an external storage device and use
         it on multiple computers where they are assigned different
         user IDs and group IDs.  This effectively makes it
         possible to assign random user IDs and group IDs at login
         time.

      •  Sharing files from the host with unprivileged containers.

???
s/Sharing files/Sharing filesystems/ ?

See [1].

Same.

quoted

quoted

         This allows a user to avoid having to change ownership
         permanently through chown(2).

      •  ID mapping a container's root filesystem.  Users don't
         need to change ownership permanently through chown(2).
         Especially for large root filesystems, using chown(2) can
         be prohibitively expensive.

      •  Sharing files between containers with non-overlapping ID

???
s/Sharing files/Sharing filesystems/ ?

See [1].

Same.

[...]

quoted

quoted

      •  Locally and temporarily restricted ownership changes.  ID-
         mapped mounts make it possible to change ownership
         locally, restricting it to specific mounts, and

???
The referent of "it" in the preceding line is not clear.
Should it be "the ownership changes"? Or something else?

It should refer to ownership changes. I'd appreciate it if you could
reformulate.

Done.

quoted

quoted

         temporarily as the ownership changes only apply as long as
         the mount exists.  By contrast, changing ownership via the
         chown(2) system call changes the ownership globally and
         permanently.

  Extensibility

[...]

quoted

quoted

  EXAMPLES

???
Do you have a (preferably simple) example piece of code
somewhere for setting up an ID mapped mount?

????
I guess the best example is this:
https://github.com/brauner/mount-idmapped/
right?

[...]

quoted

quoted

      int
      main(int argc, char *argv[])
      {
          struct mount_attr *attr = &(struct mount_attr){};
          int fd_userns = -EBADF;

???
Why this magic initializer here? Why not just "-1"?
Using -EBADF makes it look this is value specifically is
meaningful, although I don't think that's true.

[2]: I always use -EBADF to initialize fds in all my code. It makes it
pretty easy to grep for fd initialization etc. So it's pure visual
convenience. Freel free to just use -1.

Changed.

[...]

quoted

quoted

          int fd_tree = open_tree(-EBADF, source,
                       OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC |
                       AT_EMPTY_PATH | (recursive ? AT_RECURSIVE : 0));

???
What is the significance of -EBADF here? As far as I can tell, it
is not meaningful to open_tree()?

I always pass -EBADF for similar reasons to [2]. Feel free to just use -1.

????
But here, both -EBADF and -1 seem to be wrong. This argument 
is a dirfd, and so should either be a file descriptor or the
value AT_FDCWD, right?

quoted

quoted

          if (fd_tree == -1)
              exit_log("%m - Failed to open %s\n", source);

          if (fd_userns >= 0) {
              attr->attr_set  |= MOUNT_ATTR_IDMAP;
              attr->userns_fd = fd_userns;
          }

          ret = mount_setattr(fd_tree, "",
                      AT_EMPTY_PATH | (recursive ? AT_RECURSIVE : 0),
                      attr, sizeof(struct mount_attr));
          if (ret == -1)
              exit_log("%m - Failed to change mount attributes\n");

          close(fd_userns);

          ret = move_mount(fd_tree, "", -EBADF, target,
                           MOVE_MOUNT_F_EMPTY_PATH);

???
What is the significance of -EBADF here? As far as I can tell, it
is not meaningful to move_mount()?

See [2].

????
As above, both -EBADF and -1 seem to be wrong. This argument 
is a dirfd, and so should either be a file descriptor or the
value AT_FDCWD, right?

[...]

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/