On Thu, Jul 29, 2021 at 10:43:23AM +1000, NeilBrown wrote:

On Wed, 28 Jul 2021, Christian Brauner wrote:

quoted

Hey Neil,

Sorry if this is a stupid question but wouldn't you want to copy the
mount properties from path->mnt here? Couldn't you otherwise use this to
e.g. suddenly expose a dentry on a read-only mount as read-write?

There are no stupid questions, and this is a particularly non-stupid
one!

I hadn't considered that, but having examined the code I see that it
is already handled.
The vfsmount that d_automount returns is passed to finish_automount(),
which hands it to do_add_mount() together with the mnt_flags for the
parent vfsmount (plus MNT_SHRINKABLE).
do_add_mount() sets up the mnt_flags of the new vfsmount.
In fact, the d_automount interface has no control of these flags at all.
Whatever it sets will be over-written by do_add_mount.

Ah, interesting thank you very much, Neil. I seemed to have overlooked
this yesterday.

If btrfs makes use of automounts the way you envisioned to expose
subvolumes and also will support idmapped mounts (see [1]) we need to
teach do_add_mount() to also take the idmapped mount into account. So
you'd need something like (entirely untested):

diff --git a/fs/namespace.c b/fs/namespace.c
index ab4174a3c802..921f6396c36d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c

@@ -2811,6 +2811,11 @@ static int do_add_mount(struct mount *newmnt, struct mountpoint *mp,
                return -EINVAL;

        newmnt->mnt.mnt_flags = mnt_flags;
+
+       newmnt->mnt.mnt_userns = path->mnt;
+       if (newmnt->mnt.mnt_userns != &init_user_ns)
+               newmnt->mnt.mnt_userns = get_user_ns(newmnt->mnt.mnt_userns);
+
        return graft_tree(newmnt, parent, mp);
 }

[1]: https://lore.kernel.org/linux-btrfs/20210727104900.829215-1-brauner@kernel.org/T/#mca601363b435e81c89d8ca4f09134faa5c227e6d (local)