From: Linus Torvalds <torvalds@linux-foundation.org> To: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Al Viro <viro@zeniv.linux.org.uk>, LKML <linux-kernel@vger.kernel.org>, Kernel Hardening <kernel-hardening@lists.openwall.com>, Linux API <linux-api@vger.kernel.org>, Linux FS Devel <linux-fsdevel@vger.kernel.org>, Linux Security Module <linux-security-module@vger.kernel.org>, Akinobu Mita <akinobu.mita@gmail.com>, Alexey Dobriyan <adobriyan@gmail.com>, Andrew Morton <akpm@linux-foundation.org>, Andy Lutomirski <luto@kernel.org>, Daniel Micay <danielmicay@gmail.com>, Djalal Harouni <tixxdz@gmail.com>, "Dmitry V . Levin" <ldv@altlinux.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Ingo Molnar <mingo@kernel.org>, "J . Bruce Fields" <bfields@fieldses.org>, Jeff Layton <jlayton@poochiereds.net>, Jonathan Corbet <corbet@lwn.net>, Kees Cook <keescook@chromium.org>, Oleg Nesterov <oleg@redhat.com>, Solar Designer <solar@openwall.com> Subject: Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances Date: Wed, 12 Feb 2020 16:48:14 -0800 [thread overview] Message-ID: <CAHk-=wgmn9Qds0VznyphouSZW6e42GWDT5H1dpZg8pyGDGN+=w@mail.gmail.com> (raw) In-Reply-To: <87lfp7h422.fsf@x220.int.ebiederm.org> On Wed, Feb 12, 2020 at 1:48 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > The good news is proc_flush_task isn't exactly called from process exit. > proc_flush_task is called during zombie clean up. AKA release_task. Yeah, that at least avoids some of the nasty locking while dying debug problems. But the one I was more worried about was actually the lock contention issue with lots of processes. The lock is basically a single global lock in many situations - yes, it's technically per-ns, but in a lot of cases you really only have one namespace anyway. And we've had problems with global locks in this area before, notably the one you call out: > Further after proc_flush_task does it's thing the code goes > and does "write_lock_irq(&task_list_lock);" Yeah, so it's not introducing a new issue, but it is potentially making something we already know is bad even worse. > What would be downside of having a mutex for a list of proc superblocks? > A mutex that is taken for both reading and writing the list. That's what the original patch actually was, and I was hoping we could avoid that thing. An rwsem would be possibly better, since most cases by far are likely about reading. And yes, I'm very aware of the task_list_lock, but it's literally why I don't want to make a new one. I'm _hoping_ we can some day come up with something better than task_list_lock. Linus
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> To: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Kernel Hardening <kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8@public.gmane.org>, Linux API <linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux FS Devel <linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux Security Module <linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Alexey Dobriyan <adobriyan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Daniel Micay <danielmicay-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Djalal Harouni <tixxdz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Dmitry V . Levin" <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, "J . Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>, Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>, Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>, Kees Subject: Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances Date: Wed, 12 Feb 2020 16:48:14 -0800 [thread overview] Message-ID: <CAHk-=wgmn9Qds0VznyphouSZW6e42GWDT5H1dpZg8pyGDGN+=w@mail.gmail.com> (raw) In-Reply-To: <87lfp7h422.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> On Wed, Feb 12, 2020 at 1:48 PM Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > The good news is proc_flush_task isn't exactly called from process exit. > proc_flush_task is called during zombie clean up. AKA release_task. Yeah, that at least avoids some of the nasty locking while dying debug problems. But the one I was more worried about was actually the lock contention issue with lots of processes. The lock is basically a single global lock in many situations - yes, it's technically per-ns, but in a lot of cases you really only have one namespace anyway. And we've had problems with global locks in this area before, notably the one you call out: > Further after proc_flush_task does it's thing the code goes > and does "write_lock_irq(&task_list_lock);" Yeah, so it's not introducing a new issue, but it is potentially making something we already know is bad even worse. > What would be downside of having a mutex for a list of proc superblocks? > A mutex that is taken for both reading and writing the list. That's what the original patch actually was, and I was hoping we could avoid that thing. An rwsem would be possibly better, since most cases by far are likely about reading. And yes, I'm very aware of the task_list_lock, but it's literally why I don't want to make a new one. I'm _hoping_ we can some day come up with something better than task_list_lock. Linus
next prev parent reply other threads:[~2020-02-13 0:54 UTC|newest] Thread overview: 176+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-02-10 15:05 [PATCH v8 00/11] proc: modernize proc to support multiple private instances Alexey Gladkov 2020-02-10 15:05 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 01/11] proc: Rename struct proc_fs_info to proc_fs_opts Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 02/11] proc: add proc_fs_info struct to store proc information Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 03/11] proc: move /proc/{self|thread-self} dentries to proc_fs_info Alexey Gladkov 2020-02-10 15:05 ` Alexey Gladkov 2020-02-10 18:23 ` Andy Lutomirski 2020-02-10 18:23 ` Andy Lutomirski 2020-02-10 18:23 ` Andy Lutomirski 2020-02-12 15:00 ` Alexey Gladkov 2020-02-12 15:00 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 04/11] proc: move hide_pid, pid_gid from pid_namespace " Alexey Gladkov 2020-02-10 15:05 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 05/11] proc: add helpers to set and get proc hidepid and gid mount options Alexey Gladkov 2020-02-10 18:30 ` Andy Lutomirski 2020-02-10 18:30 ` Andy Lutomirski 2020-02-10 18:30 ` Andy Lutomirski 2020-02-12 14:57 ` Alexey Gladkov 2020-02-12 14:57 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 06/11] proc: support mounting procfs instances inside same pid namespace Alexey Gladkov 2020-02-10 15:05 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances Alexey Gladkov 2020-02-10 17:46 ` Linus Torvalds 2020-02-10 17:46 ` Linus Torvalds 2020-02-10 17:46 ` Linus Torvalds 2020-02-10 19:23 ` Al Viro 2020-02-10 19:23 ` Al Viro 2020-02-11 1:36 ` Eric W. Biederman 2020-02-11 1:36 ` Eric W. Biederman 2020-02-11 1:36 ` Eric W. Biederman 2020-02-11 4:01 ` Eric W. Biederman 2020-02-11 4:01 ` Eric W. Biederman 2020-02-11 4:01 ` Eric W. Biederman 2020-02-12 14:49 ` Alexey Gladkov 2020-02-12 14:49 ` Alexey Gladkov 2020-02-12 14:59 ` Eric W. Biederman 2020-02-12 14:59 ` Eric W. Biederman 2020-02-12 14:59 ` Eric W. Biederman 2020-02-12 17:08 ` Alexey Gladkov 2020-02-12 17:08 ` Alexey Gladkov 2020-02-12 18:45 ` Linus Torvalds 2020-02-12 18:45 ` Linus Torvalds 2020-02-12 18:45 ` Linus Torvalds 2020-02-12 19:16 ` Eric W. Biederman 2020-02-12 19:16 ` Eric W. Biederman 2020-02-12 19:16 ` Eric W. Biederman 2020-02-12 19:49 ` Linus Torvalds 2020-02-12 19:49 ` Linus Torvalds 2020-02-12 19:49 ` Linus Torvalds 2020-02-12 20:03 ` Al Viro 2020-02-12 20:03 ` Al Viro 2020-02-12 20:35 ` Linus Torvalds 2020-02-12 20:35 ` Linus Torvalds 2020-02-12 20:35 ` Linus Torvalds 2020-02-12 20:38 ` Al Viro 2020-02-12 20:38 ` Al Viro 2020-02-12 20:41 ` Al Viro 2020-02-12 20:41 ` Al Viro 2020-02-12 21:02 ` Linus Torvalds 2020-02-12 21:02 ` Linus Torvalds 2020-02-12 21:02 ` Linus Torvalds 2020-02-12 21:46 ` Eric W. Biederman 2020-02-12 21:46 ` Eric W. Biederman 2020-02-12 21:46 ` Eric W. Biederman 2020-02-13 0:48 ` Linus Torvalds [this message] 2020-02-13 0:48 ` Linus Torvalds 2020-02-13 0:48 ` Linus Torvalds 2020-02-13 4:37 ` Eric W. Biederman 2020-02-13 4:37 ` Eric W. Biederman 2020-02-13 5:55 ` Al Viro 2020-02-13 21:30 ` Linus Torvalds 2020-02-13 21:30 ` Linus Torvalds 2020-02-13 22:23 ` Al Viro 2020-02-13 22:47 ` Linus Torvalds 2020-02-13 22:47 ` Linus Torvalds 2020-02-14 14:15 ` Eric W. Biederman 2020-02-14 14:15 ` Eric W. Biederman 2020-02-14 3:48 ` Eric W. Biederman 2020-02-14 3:48 ` Eric W. Biederman 2020-02-20 20:46 ` [PATCH 0/7] proc: Dentry flushing without proc_mnt Eric W. Biederman 2020-02-20 20:46 ` Eric W. Biederman 2020-02-20 20:47 ` [PATCH 1/7] proc: Rename in proc_inode rename sysctl_inodes sibling_inodes Eric W. Biederman 2020-02-20 20:47 ` Eric W. Biederman 2020-02-20 20:48 ` [PATCH 2/7] proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache Eric W. Biederman 2020-02-20 20:48 ` Eric W. Biederman 2020-02-20 20:49 ` [PATCH 3/7] proc: Mov rcu_read_(lock|unlock) in proc_prune_siblings_dcache Eric W. Biederman 2020-02-20 20:49 ` Eric W. Biederman 2020-02-20 22:33 ` Linus Torvalds 2020-02-20 22:33 ` Linus Torvalds 2020-02-20 20:49 ` [PATCH 4/7] proc: Use d_invalidate " Eric W. Biederman 2020-02-20 20:49 ` Eric W. Biederman 2020-02-20 22:43 ` Linus Torvalds 2020-02-20 22:43 ` Linus Torvalds 2020-02-20 22:54 ` Al Viro 2020-02-20 23:00 ` Linus Torvalds 2020-02-20 23:00 ` Linus Torvalds 2020-02-20 23:03 ` Al Viro 2020-02-20 23:39 ` Eric W. Biederman 2020-02-20 23:39 ` Eric W. Biederman 2020-02-20 20:51 ` [PATCH 5/7] proc: Clear the pieces of proc_inode that proc_evict_inode cares about Eric W. Biederman 2020-02-20 20:51 ` Eric W. Biederman 2020-02-20 20:52 ` [PATCH 6/7] proc: Use a list of inodes to flush from proc Eric W. Biederman 2020-02-20 20:52 ` Eric W. Biederman 2020-02-20 20:52 ` [PATCH 7/7] proc: Ensure we see the exit of each process tid exactly once Eric W. Biederman 2020-02-20 20:52 ` Eric W. Biederman 2020-02-21 16:50 ` Oleg Nesterov 2020-02-22 15:46 ` Eric W. Biederman 2020-02-22 15:46 ` Eric W. Biederman 2020-02-20 23:02 ` [PATCH 0/7] proc: Dentry flushing without proc_mnt Linus Torvalds 2020-02-20 23:02 ` Linus Torvalds 2020-02-20 23:07 ` Al Viro 2020-02-20 23:37 ` Eric W. Biederman 2020-02-20 23:37 ` Eric W. Biederman 2020-02-24 16:25 ` [PATCH v2 0/6] " Eric W. Biederman 2020-02-24 16:25 ` Eric W. Biederman 2020-02-24 16:26 ` [PATCH v2 1/6] proc: Rename in proc_inode rename sysctl_inodes sibling_inodes Eric W. Biederman 2020-02-24 16:26 ` Eric W. Biederman 2020-02-24 16:27 ` [PATCH v2 2/6] proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache Eric W. Biederman 2020-02-24 16:27 ` Eric W. Biederman 2020-02-24 16:27 ` [PATCH v2 3/6] proc: In proc_prune_siblings_dcache cache an aquired super block Eric W. Biederman 2020-02-24 16:27 ` Eric W. Biederman 2020-02-24 16:28 ` [PATCH v2 4/6] proc: Use d_invalidate in proc_prune_siblings_dcache Eric W. Biederman 2020-02-24 16:28 ` Eric W. Biederman 2020-02-24 16:28 ` [PATCH v2 5/6] proc: Clear the pieces of proc_inode that proc_evict_inode cares about Eric W. Biederman 2020-02-24 16:28 ` Eric W. Biederman 2020-02-24 16:29 ` [PATCH v2 6/6] proc: Use a list of inodes to flush from proc Eric W. Biederman 2020-02-24 16:29 ` Eric W. Biederman 2020-02-28 20:17 ` [PATCH 0/3] proc: Actually honor the mount options Eric W. Biederman 2020-02-28 20:17 ` Eric W. Biederman 2020-02-28 20:18 ` [PATCH 1/3] uml: Don't consult current to find the proc_mnt in mconsole_proc Eric W. Biederman 2020-02-28 20:18 ` Eric W. Biederman 2020-02-28 20:18 ` [PATCH 2/3] uml: Create a private mount of proc for mconsole Eric W. Biederman 2020-02-28 20:18 ` Eric W. Biederman 2020-02-28 20:30 ` Christian Brauner 2020-02-28 21:28 ` Eric W. Biederman 2020-02-28 21:28 ` Eric W. Biederman 2020-02-28 21:59 ` Christian Brauner 2020-02-28 20:19 ` [PATCH 3/3] proc: Remove the now unnecessary internal mount of proc Eric W. Biederman 2020-02-28 20:19 ` Eric W. Biederman 2020-02-28 20:39 ` Christian Brauner 2020-02-28 21:40 ` Eric W. Biederman 2020-02-28 21:40 ` Eric W. Biederman 2020-02-29 3:25 ` kbuild test robot 2020-02-29 3:25 ` kbuild test robot 2020-02-29 4:49 ` Eric W. Biederman 2020-02-29 4:49 ` Eric W. Biederman 2020-03-02 23:01 ` [kbuild-all] " Philip Li 2020-03-02 23:01 ` Philip Li 2020-03-12 2:03 ` [kbuild-all] " Li Zhijian 2020-03-12 2:03 ` Li Zhijian 2020-02-29 4:23 ` kbuild test robot 2020-02-29 4:23 ` kbuild test robot 2020-02-28 22:34 ` [PATCH 4/3] pid: Improve the comment about waiting in zap_pid_ns_processes Eric W. Biederman 2020-02-28 22:34 ` Eric W. Biederman 2020-02-29 2:59 ` Christian Brauner 2020-02-14 3:49 ` [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances Eric W. Biederman 2020-02-14 3:49 ` Eric W. Biederman 2020-02-12 19:47 ` Al Viro 2020-02-12 19:47 ` Al Viro 2020-02-11 22:45 ` Al Viro 2020-02-11 22:45 ` Al Viro 2020-02-12 14:26 ` Alexey Gladkov 2020-02-12 14:26 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 08/11] proc: instantiate only pids that we can ptrace on 'hidepid=4' mount option Alexey Gladkov 2020-02-10 16:29 ` Jordan Glover 2020-02-10 16:29 ` Jordan Glover 2020-02-12 14:34 ` Alexey Gladkov 2020-02-12 14:34 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 09/11] proc: add option to mount only a pids subset Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 10/11] docs: proc: add documentation for "hidepid=4" and "subset=pidfs" options and new mount behavior Alexey Gladkov 2020-02-10 18:29 ` Andy Lutomirski 2020-02-10 18:29 ` Andy Lutomirski 2020-02-10 18:29 ` Andy Lutomirski 2020-02-12 16:03 ` Alexey Gladkov 2020-02-12 16:03 ` Alexey Gladkov 2020-02-10 15:05 ` [PATCH v8 11/11] proc: Move hidepid values to uapi as they are user interface to mount Alexey Gladkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAHk-=wgmn9Qds0VznyphouSZW6e42GWDT5H1dpZg8pyGDGN+=w@mail.gmail.com' \ --to=torvalds@linux-foundation.org \ --cc=adobriyan@gmail.com \ --cc=akinobu.mita@gmail.com \ --cc=akpm@linux-foundation.org \ --cc=bfields@fieldses.org \ --cc=corbet@lwn.net \ --cc=danielmicay@gmail.com \ --cc=ebiederm@xmission.com \ --cc=gregkh@linuxfoundation.org \ --cc=jlayton@poochiereds.net \ --cc=keescook@chromium.org \ --cc=kernel-hardening@lists.openwall.com \ --cc=ldv@altlinux.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=luto@kernel.org \ --cc=mingo@kernel.org \ --cc=oleg@redhat.com \ --cc=solar@openwall.com \ --cc=tixxdz@gmail.com \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.