Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1732993 > unrolled thread

Read-only `slaves` with shared subtrees?

Back to article view | Back to linux.kernel

Contents

  Read-only `slaves` with shared subtrees? Dawid Ciezarkiewicz <dawid.ciezarkiewicz@rubrik.com> - 2017-09-15 20:00 +0200
    Re: Read-only `slaves` with shared subtrees? Ram Pai <linuxram@us.ibm.com> - 2017-09-18 22:50 +0200
      Re: Read-only `slaves` with shared subtrees? Dawid Ciezarkiewicz <dawid.ciezarkiewicz@rubrik.com> - 2017-09-20 01:20 +0200
        Re: Read-only `slaves` with shared subtrees? Ram Pai <linuxram@us.ibm.com> - 2017-09-20 21:50 +0200
          Re: Read-only `slaves` with shared subtrees? ebiederm@xmission.com (Eric W. Biederman) - 2017-09-21 01:00 +0200
            Re: Read-only `slaves` with shared subtrees? ebiederm@xmission.com (Eric W. Biederman) - 2017-09-21 01:10 +0200
              Re: Read-only `slaves` with shared subtrees? Ram Pai <linuxram@us.ibm.com> - 2017-09-21 02:40 +0200
                Re: Read-only `slaves` with shared subtrees? Dawid Ciezarkiewicz <dawid.ciezarkiewicz@rubrik.com> - 2017-09-21 05:10 +0200
                  Re: Read-only `slaves` with shared subtrees? Ram Pai <linuxram@us.ibm.com> - 2017-09-21 21:20 +0200
                    Re: Read-only `slaves` with shared subtrees? Dawid Ciezarkiewicz <dawid.ciezarkiewicz@rubrik.com> - 2017-09-22 20:50 +0200
        Re: Read-only `slaves` with shared subtrees? Ram Pai <linuxram@us.ibm.com> - 2017-09-20 21:50 +0200

#1732993 — Read-only `slaves` with shared subtrees?

Hi,

(Please keep me in CC me when responding.)

I have an use-case for shared subtrees that is not covered by:

https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt

and I wasn't able to figure out any working solution - it might not be possible
ATM.

Long story short:
I'd like the `slave` mount (service in a container) to mount propagated events
as RO, no matter how did `master` (host) mount them. Host might need that data
RW, but slave must have it RO only.

I'm using Linux containers to isolate processes. I need the container
to follow part of the host system mount tree, but not have a write-access to it
(for security reasons). It's a trivial setup as long
as everything is static, but as soon as a part of what the container needs
to access is mounted/unmounted at runtime (and thus shared subtrees
are involved),
there seems to be no way to control the flags of the propagated mount events.

I might be able to write a patch implementing this, but before attempting that,
I'd like to confirm:

* Is it even a good idea?
* Is it maybe already possible by some other means?
* Is it an use-case that might potentially be worth supporting in the mainline?
   If so: any hints/ideas about the design and API?

Best Regards,
--
Dawid Ciezarkiewicz
Software Engineer at Rubrik

[toc] | [next] | [standalone]

#1734468

On Fri, Sep 15, 2017 at 10:57:30AM -0700, Dawid Ciezarkiewicz wrote:
> Hi,
> 
> (Please keep me in CC me when responding.)
> 
> I have an use-case for shared subtrees that is not covered by:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.kernel.org_doc_Documentation_filesystems_sharedsubtree.txt&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=m-UrKChQVkZtnPpjbF6YY99NbT8FBByQ-E-ygV8luxw&m=l46zr30OWtcm54Kc2X1KfLkB11GtFf2YLA0WcpI6Tuo&s=L-i2sXNn5dHjJfzl_lCW-JvlZnGf8NdOB7ZktFGTUdY&e= 
> 
> and I wasn't able to figure out any working solution - it might not be possible
> ATM.
> 
> Long story short:
> I'd like the `slave` mount (service in a container) to mount propagated events
> as RO, no matter how did `master` (host) mount them. Host might need that data
> RW, but slave must have it RO only.
> 
> I'm using Linux containers to isolate processes. I need the container
> to follow part of the host system mount tree, but not have a write-access to it
> (for security reasons). It's a trivial setup as long
> as everything is static, but as soon as a part of what the container needs
> to access is mounted/unmounted at runtime (and thus shared subtrees
> are involved),
> there seems to be no way to control the flags of the propagated mount events.

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.

Should it inherit? or should it not? -- that has not been thought
off AFAICT. it think we should let it inherit.

RP

[toc] | [prev] | [next] | [standalone]

#1735344

On Mon, Sep 18, 2017 at 1:47 PM, Ram Pai <linuxram@us.ibm.com> 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.

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:

http://elixir.free-electrons.com/linux/latest/source/fs/pnode.c#L294

Regards,
Dawid Ciezarkiewicz

[toc] | [prev] | [next] | [standalone]

#1736047

sorry forgot to copy Eric.


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@us.ibm.com> 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.
> 
> 
> > 
> > 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

-- 
Ram Pai

[toc] | [prev] | [next] | [standalone]

#1736223

Ram Pai <linuxram@us.ibm.com> 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@us.ibm.com> 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

[toc] | [prev] | [next] | [standalone]

#1736228

ebiederm@xmission.com (Eric W. Biederman) writes:

> Ram Pai <linuxram@us.ibm.com> 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@us.ibm.com> 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

[toc] | [prev] | [next] | [standalone]

#1736285

On Wed, Sep 20, 2017 at 06:06:55PM -0500, Eric W. Biederman wrote:
> ebiederm@xmission.com (Eric W. Biederman) writes:
> 
> > Ram Pai <linuxram@us.ibm.com> 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@us.ibm.com> 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.

Yes. This can be made generic, independent of
propagation/shared-subtree semantics.

"Any mount that has been marked as 'propagate-access' will pass-on
its read-write attribute to its children."

'propagate-*' may confuse the reader
into thinking shared-subtree.  May be 'pass-on-access' or 'endow-access'
or 'inherit-to-access' :-).

Anyway; so something like this should be possible without breaking
existing semantics.

mount -o bind,remount,ro /mnt
mount --make-pass-on-access  /mnt

anything that gets mounted under /mnt will inherit the
'ro' attribute from its parent.  And when a mount-event propagates
to a read-only-slave-mount, that new mount will automatically 
inherit the read-only attribute from its slave-parent.

Dawid: will that work for you?
RP

[toc] | [prev] | [next] | [standalone]

#1736341

On Wed, Sep 20, 2017 at 5:39 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> Anyway; so something like this should be possible without breaking
> existing semantics.
>
> mount -o bind,remount,ro /mnt
> mount --make-pass-on-access  /mnt
>
> anything that gets mounted under /mnt will inherit the
> 'ro' attribute from its parent.  And when a mount-event propagates
> to a read-only-slave-mount, that new mount will automatically
> inherit the read-only attribute from its slave-parent.
>
> Dawid: will that work for you?


Yes. It is even more universal.

Regards,
Dawid Ciezarkiewicz

[toc] | [prev] | [next] | [standalone]

#1737021

On Wed, Sep 20, 2017 at 08:00:57PM -0700, Dawid Ciezarkiewicz wrote:
> On Wed, Sep 20, 2017 at 5:39 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> > Anyway; so something like this should be possible without breaking
> > existing semantics.
> >
> > mount -o bind,remount,ro /mnt
> > mount --make-pass-on-access  /mnt
> >
> > anything that gets mounted under /mnt will inherit the
> > 'ro' attribute from its parent.  And when a mount-event propagates
> > to a read-only-slave-mount, that new mount will automatically
> > inherit the read-only attribute from its slave-parent.
> >
> > Dawid: will that work for you?
> 
> 
> Yes. It is even more universal.


Here is a patch that accomplishes the job. tested to work with
some simple use cases.  check if this works for you. If it does
than we will have to think through all the edge cases and make it
acceptable.


------------------------------------------------------
Signed-off-by: Ram Pai <linuxram@us.ibm.com>

diff --git a/fs/namespace.c b/fs/namespace.c
index f8893dc..08f63b6 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -854,6 +854,8 @@ void mnt_set_mountpoint(struct mount *mnt,
 	child_mnt->mnt_mountpoint = dget(mp->m_dentry);
 	child_mnt->mnt_parent = mnt;
 	child_mnt->mnt_mp = mp;
+	if (mnt->mnt.mnt_flags & MNT_STICKY_RW)
+		child_mnt->mnt.mnt_flags |= (mnt->mnt.mnt_flags & (MNT_READONLY | MNT_STICKY_RW));
 	hlist_add_head(&child_mnt->mnt_mp_list, &mp->m_list);
 }
 
@@ -1052,6 +1054,12 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	    (!(flag & CL_EXPIRE) || list_empty(&old->mnt_expire)))
 		mnt->mnt.mnt_flags |= MNT_LOCKED;
 
+	if (flag & CL_STICKY_RW) {
+		mnt->mnt.mnt_flags |= MNT_STICKY_RW;
+		if (flag & CL_READONLY)
+			mnt->mnt.mnt_flags |= MNT_READONLY;
+	}
+
 	atomic_inc(&sb->s_active);
 	mnt->mnt.mnt_sb = sb;
 	mnt->mnt.mnt_root = dget(root);
@@ -2078,7 +2086,7 @@ static int flags_to_propagation_type(int flags)
 	int type = flags & ~(MS_REC | MS_SILENT);
 
 	/* Fail if any non-propagation flags are set */
-	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
+	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE | MS_STICKY_RW))
 		return 0;
 	/* Only one propagation flag should be set */
 	if (!is_power_of_2(type))
@@ -2113,7 +2121,10 @@ static int do_change_type(struct path *path, int flag)
 
 	lock_mount_hash();
 	for (m = mnt; m; m = (recurse ? next_mnt(m, mnt) : NULL))
-		change_mnt_propagation(m, type);
+		if (type == MS_STICKY_RW)
+			set_mnt_sticky(m);
+		else
+			change_mnt_propagation(m, type);
 	unlock_mount_hash();
 
  out_unlock:
@@ -2768,7 +2779,7 @@ long do_mount(const char *dev_name, const char __user *dir_name,
 				    data_page);
 	else if (flags & MS_BIND)
 		retval = do_loopback(&path, dev_name, flags & MS_REC);
-	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
+	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE | MS_STICKY_RW))
 		retval = do_change_type(&path, flags);
 	else if (flags & MS_MOVE)
 		retval = do_move_mount(&path, dev_name);
diff --git a/fs/pnode.c b/fs/pnode.c
index 53d411a..386105a 100644
--- a/fs/pnode.c
+++ b/fs/pnode.c
@@ -262,6 +262,13 @@ 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_STICKY_RW) {
+		type |= CL_STICKY_RW;
+		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..0a4f7c2 100644
--- a/fs/pnode.h
+++ b/fs/pnode.h
@@ -29,6 +29,8 @@
 #define CL_SHARED_TO_SLAVE	0x20
 #define CL_UNPRIVILEGED		0x40
 #define CL_COPY_MNT_NS_FILE	0x80
+#define CL_STICKY_RW		0x100
+#define CL_READONLY		0x200
 
 #define CL_COPY_ALL		(CL_COPY_UNBINDABLE | CL_COPY_MNT_NS_FILE)
 
@@ -38,6 +40,11 @@ static inline void set_mnt_shared(struct mount *mnt)
 	mnt->mnt.mnt_flags |= MNT_SHARED;
 }
 
+static inline void set_mnt_sticky(struct mount *mnt)
+{
+	mnt->mnt.mnt_flags |= MNT_STICKY_RW;
+}
+
 void change_mnt_propagation(struct mount *, int);
 int propagate_mnt(struct mount *, struct mountpoint *, struct mount *,
 		struct hlist_head *);
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 1ce85e6..85dc195 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -28,6 +28,7 @@
 #define MNT_NODIRATIME	0x10
 #define MNT_RELATIME	0x20
 #define MNT_READONLY	0x40	/* does the user want this to be r/o? */
+#define MNT_STICKY_RW	0x80	/* children inherit READONLY attr if set */
 
 #define MNT_SHRINKABLE	0x100
 #define MNT_WRITE_HOLD	0x200
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index b7495d0..b06b277 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -112,6 +112,7 @@ struct inodes_stat_t {
 #define MS_REMOUNT	32	/* Alter flags of a mounted FS */
 #define MS_MANDLOCK	64	/* Allow mandatory locks on an FS */
 #define MS_DIRSYNC	128	/* Directory modifications are synchronous */
+#define MS_STICKY_RW	(1<<8)	/* children inherit the RW flag */
 #define MS_NOATIME	1024	/* Do not update access times. */
 #define MS_NODIRATIME	2048	/* Do not update directory access times */
 #define MS_BIND		4096

------------------------------------------------------

Here is a small program that setsup a mount to enable inheritance
of the RW attribute of the mount.

/* pass_on_readonly.c */
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/mount.h>

#define MS_STICKY_RW (1<<8)

int main(int argc, char *argv[])
{
	unsigned long flags = MS_STICKY_RW;

	if (argc < 2) {
		printf("only argc=%d\n", argc);
		exit (0);
	}
	if (mount(argv[1], argv[1], NULL, flags, NULL) == -1)
		perror("failed ");
	exit (0);
}

-------------------------------------------------------



my testcase is

a) setup a shared mount
	mkdir shared
	mount --bind shared shared
	mount --make-shared shared

b) setup a slave mount
	mkdir slave
	mount --bind shared slave
	mount --make-slave slave

c) make the slave readonly
	mount -o bind,remount,ro slave

d) setup the slave to for passing one its readonly attribute
	gcc pass_on_readonly.c -o pass_on_readonly
	./pass_on_readonly slave


e) create a small mount tree to bind
	mkdir tmpbind
	mount --bind tmpbind tmpbind
	mkdir -p tmpbind/subtmpbind
	mount --bind tmpbind/subtmpbind tmpbind/subtmpbind

f) now mount this tree on the shared mount
	mkdir shared/sub
	mount --rbind tmpbind shared/sub

e) verify if the mounts under slave/sub are all readonly.
	> slave/sub/create_a_file
	slave/sub/create_a_file	: Read-only file system

	> slave/sub/subtmpbind/create_another_file
	slave/sub/subtmpbind/create_another_file : Read-only file system


RP

[toc] | [prev] | [next] | [standalone]

#1737766

On Thu, Sep 21, 2017 at 12:14 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> Here is a patch that accomplishes the job. tested to work with
> some simple use cases.  check if this works for you. If it does
> than we will have to think through all the edge cases and make it
> acceptable.

From your experiments, it looks exactly right.

I'll give it a try in the upcoming week. Thank you!

[toc] | [prev] | [next] | [standalone]

#1736050

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@us.ibm.com> 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.


> 
> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web