From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754238AbdCWW5U (ORCPT ); Thu, 23 Mar 2017 18:57:20 -0400 Received: from mozilla-myt-1.haze.yandex.net ([77.88.23.115]:49424 "EHLO mail.unsafe.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751672AbdCWW5R (ORCPT ); Thu, 23 Mar 2017 18:57:17 -0400 Date: Thu, 23 Mar 2017 23:57:10 +0100 From: Alexey Gladkov To: Oleg Nesterov Cc: Linux Kernel Mailing List , Linux API , "Kirill A. Shutemov" , Vasiliy Kulikov , Al Viro , "Eric W. Biederman" , Pavel Emelyanov , James Bottomley , "Dmitry V. Levin" Subject: Re: [RFC] Add option to mount only a pids subset Message-ID: <20170323225710.GI4554@comp-core-i7-2640m-0182e6> References: <20170312021257.GP29622@ZenIV.linux.org.uk> <20170320125855.GG4554@comp-core-i7-2640m-0182e6> <20170323160507.GA23135@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170323160507.GA23135@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 23, 2017 at 05:05:07PM +0100, Oleg Nesterov wrote: > Again, I can't really review this, I know nothing about vfs, but since > nobody else replied... Thanks anyway :) > On 03/20, Alexey Gladkov wrote: > > > > @@ -97,7 +169,23 @@ static struct dentry *proc_mount(struct file_system_type *fs_type, > > ns = task_active_pid_ns(current); > > } > > > > - return mount_ns(fs_type, flags, data, ns, ns->user_ns, proc_fill_super); > > + root = mount_ns(fs_type, flags, data, ns, ns->user_ns, proc_fill_super); > > + > > + if (!IS_ERR(root)) { > > + if (!proc_fill_options(data, &opts)) > > + return ERR_PTR(-EINVAL); > > So we have to call proc_fill_options() twice, not good... Yes, I understand > why, but perhaps we factor it out somehow, we can pack options + pid_ns into > sb->s_fs_info. Nevermind, this is minor. It happens only once, when we don't have the s_root yet. > > + if (opts.pid_only) { > > + int ret; > > + > > + if (!ns->pidfs && (ret = fill_pidfs_root(root->d_sb))) > > + return ERR_PTR(ret); > > + > > + root = ns->pidfs; > > Afaics this lacks dget(ns->pidfs) which should pair with dput(mnt.mnt_root) > in cleanup_mnt(). IIUC otherwise ns->pidfs can go away after umount, OTOH, > if we return ns->pidfs then dget(sb->s_root) in mount_ns() is not balanced. > But this all is fixeable. > > So with this change "mount -opidonly" creates another IS_ROOT() dentry which > is not equal to sb->s_root. I simply do not know if this is technically > correct or not... but, say, the "Only bind mounts can have disconnected paths" > comment in path_connected() makes me worry ;) > > And this obviously means that /path-to-pidonly-mnt/ won't share dentries with > the normal /proc mount. Not really good imo even if not really wrong... Lets > look at proc_flush_task(). The exiting task will flush its $pid dentries in > /proc/ but not in /path-to-pidonly-mnt/ iiuc. Again, not really a bug, but > still... I know that I'm cheater, but I did not start first :) -- Rgrds, legion From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Gladkov Subject: Re: [RFC] Add option to mount only a pids subset Date: Thu, 23 Mar 2017 23:57:10 +0100 Message-ID: <20170323225710.GI4554@comp-core-i7-2640m-0182e6> References: <20170312021257.GP29622@ZenIV.linux.org.uk> <20170320125855.GG4554@comp-core-i7-2640m-0182e6> <20170323160507.GA23135@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20170323160507.GA23135-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Oleg Nesterov Cc: Linux Kernel Mailing List , Linux API , "Kirill A. Shutemov" , Vasiliy Kulikov , Al Viro , "Eric W. Biederman" , Pavel Emelyanov , James Bottomley , "Dmitry V. Levin" List-Id: linux-api@vger.kernel.org On Thu, Mar 23, 2017 at 05:05:07PM +0100, Oleg Nesterov wrote: > Again, I can't really review this, I know nothing about vfs, but since > nobody else replied... Thanks anyway :) > On 03/20, Alexey Gladkov wrote: > > > > @@ -97,7 +169,23 @@ static struct dentry *proc_mount(struct file_system_type *fs_type, > > ns = task_active_pid_ns(current); > > } > > > > - return mount_ns(fs_type, flags, data, ns, ns->user_ns, proc_fill_super); > > + root = mount_ns(fs_type, flags, data, ns, ns->user_ns, proc_fill_super); > > + > > + if (!IS_ERR(root)) { > > + if (!proc_fill_options(data, &opts)) > > + return ERR_PTR(-EINVAL); > > So we have to call proc_fill_options() twice, not good... Yes, I understand > why, but perhaps we factor it out somehow, we can pack options + pid_ns into > sb->s_fs_info. Nevermind, this is minor. It happens only once, when we don't have the s_root yet. > > + if (opts.pid_only) { > > + int ret; > > + > > + if (!ns->pidfs && (ret = fill_pidfs_root(root->d_sb))) > > + return ERR_PTR(ret); > > + > > + root = ns->pidfs; > > Afaics this lacks dget(ns->pidfs) which should pair with dput(mnt.mnt_root) > in cleanup_mnt(). IIUC otherwise ns->pidfs can go away after umount, OTOH, > if we return ns->pidfs then dget(sb->s_root) in mount_ns() is not balanced. > But this all is fixeable. > > So with this change "mount -opidonly" creates another IS_ROOT() dentry which > is not equal to sb->s_root. I simply do not know if this is technically > correct or not... but, say, the "Only bind mounts can have disconnected paths" > comment in path_connected() makes me worry ;) > > And this obviously means that /path-to-pidonly-mnt/ won't share dentries with > the normal /proc mount. Not really good imo even if not really wrong... Lets > look at proc_flush_task(). The exiting task will flush its $pid dentries in > /proc/ but not in /path-to-pidonly-mnt/ iiuc. Again, not really a bug, but > still... I know that I'm cheater, but I did not start first :) -- Rgrds, legion