Re: Read-only `slaves` with shared subtrees?

[Eric W. Biederman]

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.

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.

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