[Toybox] [PATCH] Support mount -o private mountpoint

[Yi-yo Chiang]

On Fri, Jun 24, 2022 at 3:07 PM Rob Landley <rob at landley.net> wrote:

> On 6/21/22 11:30, Yi-yo Chiang via Toybox wrote:
> > `mount -o private mountpoint` should change the mount propagation type
> > of mountpoint to MS_PRIVATE. This didn't work because it falls into
> > the "read /etc/fstab" branch (one argument), and fails when mountpoint
> > is not found in the fstab, or fstab is not found (Android case).
>
> I think you want to do "mount -o remount,private mountpoint"?
>
>
No i really just want `-o private`, in fact I want
  mount("", <mountpoint>, "", MS_PRIVATE, "");

MS_PRIVATE, MS_SHARED, MS_SLAVE, MS_UNBINDABLE are special semantics that
*changes the subtree propagation type* of an existing mount point. It
doesn't alter or remount the underlying filesystem.

If MS_BIND or MS_REMOUNT is given, then MS_PRIVATE, MS_SHARD... would be
ignored. (man 2 mount)



> > `mount -o private blah mountpoint` kind of works, as long as `blah`
> > doesn't look like a directory, otherwise toybox would assume bind mount
> > and MS_BIND would win over MS_PRIVATE.
>
> Is mountpoint an existing mountpoint? Because you should either get an
> overmount
> or an "is already mounted" error, unless MS_PRIVATE is being _REALLY_
> magic in
> the kernel behind the scenes and doing an implicit -o remount?
>

Yes mountpoint must be an already mounted mountpoint, so I can do this
right now
$ mount --bind mnt mnt
$ mount -o private xxxxxx mnt
which bind mounts /mnt to itself and changes its propagation type to
MS_PRIVATE.

If I do this instead
$ mount --bind mnt mnt
$ mount -o private mnt mnt
then toybox thinks the second command is a bind mount, and calls
`mount("mnt", "mnt", fs_type, MS_PRIVATE|MS_BIND, "")`, and the kernel
would ignore the MS_PRIVATE flag as MS_BIND is also given.



> > Even if `blah mountpoint` looks sufficient unlike a bind mount, the
> > underlying mount() syscall would be a bit off,
> >   mount(blah, mountpoint, "ext3", MS_SILENT|MS_PRIVATE, "")
>
> Yes, it would. Among other things, no MS_REMOUNT flag...
>

No MS_REMOUNT is the desired behavior here.


>
> > The "ext3" (or whatever comes first in /proc/filesystems) is because
> > unspecified `-t` defaults to `-t auto`, which means "try mount FS in
> > /proc/filesystems one-by-one", which is not what we want here. We don't
> > want to mount anything, but change an existing mountpoint's propagation
> > type.
>
> That's what -o remount does. Are you saying you want -o remount to be
> implicit here?
>

No we really don't want MS_REMOUNT if we are changing the propagation type
flags.

mount(<ignored>, mountpoint, <ignored>, MS_REMOUNT|flags, fs_flags);
=> change mount flags, if MS_PRIVATE (& friends) is specified in |flags|,
it is ignored by kernel.

mount(<ignored>, mountpoint, <ignored>, MS_PRIVATE, <ignored>); (or
MS_SHARED...)
=> change the propagation type of mountpoint, that's it.


>
> > This patch adds "mount -o private mountpoint" machinery and unit test.
>
> Not looking like it makes MS_REMOUNT implicit when
> MS_PRIVATE/SHARED/SLAVE/UNBINDABLE is giving with one argument...
>
> +testing "-o private not_a_mountpoint" \
> +  "mount -o private '${mountpoint}' 2>/dev/null || echo error" "error\n"
> "" ""
>
> Because you didn't tell it WHAT to mount, so yes it should error if this
> isn't a
> remount. (Did your patch change this behavior?)
>

Yes my patch added this behavior. It changes so that if one of the
propagation type flags is given, and only one argument is given, and no '-o
remount' is given, then perform `mount("", mountpoint, "", MS_PRIVATE, "")`.
This test case is expected to fail because ${mountpoint} was not a mounted
mountpoint, so mount(2) could not chnage its propagation type.


>
> +testing "-o bind,private dir dir (MS_BIND wins MS_PRIVATE)" \
> +  "mount -o bind,private '${mountpoint}' '${mountpoint}' 2>&1 &&
> +   umount '${mountpoint}' && echo yes" "yes\n" "" ""
>
> You're bind mounting a directory on itself and adding the private flag:
> did that
> not already work? (If it wasn't _already_ a mount point, I mean.)
>

yeah it works already right now. I added this case to check my changes
didn't break any existing behavior.
$ mount -o bind,private dir dir  #=>  still a bind mount
$ mount -o private dir  #=> was *mount by fstab*, now changed to *change
mount propagation type*


>
> I admit I haven't tested the private/shared/slave stuff much because it
> ties
> into namespaces/containers which I have a big todo hairball for. but I
> think I
> tested this at one point? I note that mount.tests is one of the big things
> I
> want to get tests running under mkroot for, because "run as root, leave the
> system in a bad state if it didn't finish cleanly" is not something I want
> to do
> on my development laptop a lot. Getting that to work soonish has been the
> focus
> of my recent sh.c commits...
>
>
100% agree. I once naively run `make tests` with root just to see if it
works, it end up leaving my system in a weird state and I had to reboot and
fix my rootfs... After that I always run tests on an android device.


> Rob
>


-- 

Yi-yo Chiang
Software Engineer
yochiang at google.com

I support flexible work schedules, and I’m sending this email now because
it is within the hours I’m working today. Please do not feel obliged to
reply straight away - I understand that you will reply during the hours you
work, which may not match mine.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20220624/ee6f0815/attachment.htm>