ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes:
> Ram Pai <linuxram@xxxxxxxxxx> writes:
>
>> sorry forgot to copy Eric.
>
> Adding fs-devel as well.
>
>> On Wed, Sep 20, 2017 at 12:39:54PM -0700, Ram Pai wrote:
>>> On Tue, Sep 19, 2017 at 04:18:02PM -0700, Dawid Ciezarkiewicz wrote:
>>> > On Mon, Sep 18, 2017 at 1:47 PM, Ram Pai <linuxram@xxxxxxxxxx> wrote:
>>> > > It is possible to make a slave mount readonly, by remounting it with
>>> > > 'ro' flags.
>>> > >
>>> > > something like
>>> > >
>>> > > mount -o bind,remount,ro <slave-mount-dir>
>>> > >
>>> > > Any mount-propagation events reaching a read-only-slave does
>>> > > inherit the slave attribute. However it does not inherit the
>>> > > read-only attribute.
>>> >
>>> > I did try manually remounting, and it worked for me. If this could be
>>> > done atomically
>>> > (which I assume can't be, in the userspace) it could even be a workaround.
>>> >
>>> > > Should it inherit? or should it not? -- that has not been thought
>>> > > off AFAICT. it think we should let it inherit.
>>> >
>>> > It makes sense, and it would work in my use-case. I wonder
>>> > if that would break any existing expectations though.
>>>
>>> It could break existing expectations, for mounts created by propagation.
>>> This needs to be thought through. Also Should the same semantics
>>> apply to MNT_NOSUID, MNT_NOEXEC etc etc?
>>>
>>> Copying Eric. he should be able to tell if any of the container
>>> infrastructure assumes anything about mounts propagated to read-only
>>> mounts.
>
> *Blink*
>
> Let me reiterate what I think I am seeing. The properties of a
> propogated mount taking on attributes from the propagation node, where
> the mount is propagated too.
>
> I honestly can't say if any code cares today, but this feels like it
> will break the principle of least surprise and break someone.
Thinking about this a little I am almost certain this will break
something.
A common pattern for containers is to have a read-only shared portion
typically the rootfs and then other mounts that are read-write. If all
of your propagation nodes hang off of a big read-only mount (and
therefore need to be read-only) forcing everything else to propagate
into the container as read-only is likely going to break something.
> We can safely add this extension by adding a new flag or flags that can
> be set on a pnode that will give the desired semantics. So I expect
> that is a better model then adding new semantics to MNT_RDONLY.
Which means I think to do this safely we really do need to add a new
flag.
Eric
>>> > I could at least test such a patch, it seems like a tiny change.
>>> > Should I give it a try and submit a patch? If you could PM me any pointers
>>> > it could help a lot since I'm not familiar with FS internals. So far I got here:
>>>
>>> Here is a rough patch which will accomplish what you want; not
>>> compile-tested nor tested.
>>>
>>>
>>> diff --git a/fs/namespace.c b/fs/namespace.c
>>> index f8893dc..3239adc 100644
>>> --- a/fs/namespace.c
>>> +++ b/fs/namespace.c
>>> @@ -1061,6 +1061,9 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
>>> list_add_tail(&mnt->mnt_instance, &sb->s_mounts);
>>> unlock_mount_hash();
>>>
>>> + if (flag & CL_READONLY)
>>> + mnt->mnt.mnt_flags |= MNT_READONLY;
>>> +
>>> if ((flag & CL_SLAVE) ||
>>> ((flag & CL_SHARED_TO_SLAVE) && IS_MNT_SHARED(old))) {
>>> list_add(&mnt->mnt_slave, &old->mnt_slave_list);
>>> diff --git a/fs/pnode.c b/fs/pnode.c
>>> index 53d411a..aeb5b47 100644
>>> --- a/fs/pnode.c
>>> +++ b/fs/pnode.c
>>> @@ -262,6 +262,8 @@ static int propagate_one(struct mount *m)
>>> /* Notice when we are propagating across user namespaces */
>>> if (m->mnt_ns->user_ns != user_ns)
>>> type |= CL_UNPRIVILEGED;
>>> + if (m->mnt.mnt_flags & MNT_READONLY)
>>> + type |= CL_READONLY;
>>> child = copy_tree(last_source, last_source->mnt.mnt_root, type);
>>> if (IS_ERR(child))
>>> return PTR_ERR(child);
>>> diff --git a/fs/pnode.h b/fs/pnode.h
>>> index dc87e65..7c59469 100644
>>> --- a/fs/pnode.h
>>> +++ b/fs/pnode.h
>>> @@ -29,6 +29,7 @@
>>> #define CL_SHARED_TO_SLAVE 0x20
>>> #define CL_UNPRIVILEGED 0x40
>>> #define CL_COPY_MNT_NS_FILE 0x80
>>> +#define CL_READONLY 0x100
>>>
>>> #define CL_COPY_ALL (CL_COPY_UNBINDABLE | CL_COPY_MNT_NS_FILE)
>>>
>>> RP
