From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751976AbdITXHK (ORCPT ); Wed, 20 Sep 2017 19:07:10 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:46883 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751584AbdITXHI (ORCPT ); Wed, 20 Sep 2017 19:07:08 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Ram Pai Cc: Dawid Ciezarkiewicz , linux-kernel@vger.kernel.org, References: <20170918204710.GI5698@ram.oc3035372033.ibm.com> <20170920193954.GK5698@ram.oc3035372033.ibm.com> <20170920194108.GB5721@ram.oc3035372033.ibm.com> <87377h2d4a.fsf@xmission.com> Date: Wed, 20 Sep 2017 18:06:55 -0500 In-Reply-To: <87377h2d4a.fsf@xmission.com> (Eric W. Biederman's message of "Wed, 20 Sep 2017 17:56:21 -0500") Message-ID: <87shfh0y28.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1duo57-0000SM-4S;;;mid=<87shfh0y28.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.200.44;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18g5P31jLhIgBuE9ERU2JnMKkDp7SLJOso= X-SA-Exim-Connect-IP: 67.3.200.44 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Ram Pai X-Spam-Relay-Country: X-Spam-Timing: total 5543 ms - load_scoreonly_sql: 0.04 (0.0%), signal_user_changed: 3.2 (0.1%), b_tie_ro: 2.3 (0.0%), parse: 1.19 (0.0%), extract_message_metadata: 15 (0.3%), get_uri_detail_list: 3.7 (0.1%), tests_pri_-1000: 6 (0.1%), tests_pri_-950: 1.19 (0.0%), tests_pri_-900: 0.99 (0.0%), tests_pri_-400: 28 (0.5%), check_bayes: 27 (0.5%), b_tokenize: 10 (0.2%), b_tok_get_all: 9 (0.2%), b_comp_prob: 2.8 (0.1%), b_tok_touch_all: 3.5 (0.1%), b_finish: 0.57 (0.0%), tests_pri_0: 325 (5.9%), check_dkim_signature: 0.52 (0.0%), check_dkim_adsp: 3.4 (0.1%), tests_pri_500: 5159 (93.1%), poll_dns_idle: 5154 (93.0%), rewrite_mail: 0.00 (0.0%) Subject: Re: Read-only `slaves` with shared subtrees? X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ebiederm@xmission.com (Eric W. Biederman) writes: > Ram Pai 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 wrote: >>> > > It is possible to make a slave mount readonly, by remounting it with >>> > > 'ro' flags. >>> > > >>> > > something like >>> > > >>> > > mount -o bind,remount,ro >>> > > >>> > > 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