Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Oleg Nesterov <oleg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	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>,
	Solar Designer <solar@openwall.com>
Subject: Re: [PATCH 7/7] proc: Ensure we see the exit of each process tid exactly once
Date: Sat, 22 Feb 2020 09:46:29 -0600
Message-ID: <87mu9a4obu.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20200221165036.GB16646@redhat.com> (Oleg Nesterov's message of "Fri, 21 Feb 2020 17:50:37 +0100")

Oleg Nesterov <oleg@redhat.com> writes:

> On 02/20, Eric W. Biederman wrote:
>>
>> +void exchange_tids(struct task_struct *ntask, struct task_struct *otask)
>> +{
>> +	/* pid_links[PIDTYPE_PID].next is always NULL */
>> +	struct pid *npid = READ_ONCE(ntask->thread_pid);
>> +	struct pid *opid = READ_ONCE(otask->thread_pid);
>> +
>> +	rcu_assign_pointer(opid->tasks[PIDTYPE_PID].first, &ntask->pid_links[PIDTYPE_PID]);
>> +	rcu_assign_pointer(npid->tasks[PIDTYPE_PID].first, &otask->pid_links[PIDTYPE_PID]);
>> +	rcu_assign_pointer(ntask->thread_pid, opid);
>> +	rcu_assign_pointer(otask->thread_pid, npid);
>
> this breaks has_group_leader_pid()...
>
> proc_pid_readdir() can miss a process doing mt-exec but this looks fixable,
> just we need to update ntask->thread_pid before updating ->first.
>
> The more problematic case is __exit_signal() which does
> 		
> 		if (unlikely(has_group_leader_pid(tsk)))
> 			posix_cpu_timers_exit_group(tsk);

Along with the comment:
		/*
		 * This can only happen if the caller is de_thread().
		 * FIXME: this is the temporary hack, we should teach
		 * posix-cpu-timers to handle this case correctly.
		 */
So I suspect this is fixable and the above fix might be part of that.

Hmm looking at your commit:

commit e0a70217107e6f9844628120412cb27bb4cea194
Author: Oleg Nesterov <oleg@redhat.com>
Date:   Fri Nov 5 16:53:42 2010 +0100

    posix-cpu-timers: workaround to suppress the problems with mt exec
    
    posix-cpu-timers.c correctly assumes that the dying process does
    posix_cpu_timers_exit_group() and removes all !CPUCLOCK_PERTHREAD
    timers from signal->cpu_timers list.
    
    But, it also assumes that timer->it.cpu.task is always the group
    leader, and thus the dead ->task means the dead thread group.
    
    This is obviously not true after de_thread() changes the leader.
    After that almost every posix_cpu_timer_ method has problems.
    
    It is not simple to fix this bug correctly. First of all, I think
    that timer->it.cpu should use struct pid instead of task_struct.
    Also, the locking should be reworked completely. In particular,
    tasklist_lock should not be used at all. This all needs a lot of
    nontrivial and hard-to-test changes.
    
    Change __exit_signal() to do posix_cpu_timers_exit_group() when
    the old leader dies during exec. This is not the fix, just the
    temporary hack to hide the problem for 2.6.37 and stable. IOW,
    this is obviously wrong but this is what we currently have anyway:
    cpu timers do not work after mt exec.
    
    In theory this change adds another race. The exiting leader can
    detach the timers which were attached to the new leader. However,
    the window between de_thread() and release_task() is small, we
    can pretend that sys_timer_create() was called before de_thread().
    
    Signed-off-by: Oleg Nesterov <oleg@redhat.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

It looks like the data structures need fixing.  Possibly to use struct
pid.  Possibly to move the group data to signal struct.

I think I played with some of that awhile ago.

I am going to move this change to another patchset.  So I don't wind up
playing shift the bug around.  I thought I would need this to get the
other code working but it turns out we remain bug compatible without
this.

Hopefully I can get something out in the next week or so that addresses
the issues you have pointed out.

Eric


  reply index

Thread overview: 85+ 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 ` [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 18:23   ` Andy Lutomirski
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 ` [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-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 ` [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 19:23     ` Al Viro
2020-02-11  1:36   ` Eric W. Biederman
2020-02-11  4:01     ` Eric W. Biederman
2020-02-12 14:49     ` Alexey Gladkov
2020-02-12 14:59       ` Eric W. Biederman
2020-02-12 17:08         ` Alexey Gladkov
2020-02-12 18:45         ` Linus Torvalds
2020-02-12 19:16           ` Eric W. Biederman
2020-02-12 19:49             ` Linus Torvalds
2020-02-12 20:03               ` Al Viro
2020-02-12 20:35                 ` Linus Torvalds
2020-02-12 20:38                   ` Al Viro
2020-02-12 20:41                     ` Al Viro
2020-02-12 21:02                       ` Linus Torvalds
2020-02-12 21:46                         ` Eric W. Biederman
2020-02-13  0:48                           ` Linus Torvalds
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 22:23                                   ` Al Viro
2020-02-13 22:47                                     ` Linus Torvalds
2020-02-14 14:15                                       ` 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:47                                 ` [PATCH 1/7] proc: Rename in proc_inode rename sysctl_inodes sibling_inodes 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:49                                 ` [PATCH 3/7] proc: Mov rcu_read_(lock|unlock) in proc_prune_siblings_dcache Eric W. Biederman
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 22:43                                   ` Linus Torvalds
2020-02-20 22:54                                   ` Al Viro
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 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:52                                 ` [PATCH 6/7] proc: Use a list of inodes to flush from proc 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-21 16:50                                   ` Oleg Nesterov
2020-02-22 15:46                                     ` Eric W. Biederman [this message]
2020-02-20 23:02                                 ` [PATCH 0/7] proc: Dentry flushing without proc_mnt Linus Torvalds
2020-02-20 23:07                                   ` Al Viro
2020-02-20 23:37                                     ` Eric W. Biederman
2020-02-24 16:25                                 ` [PATCH v2 0/6] " 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:27                                   ` [PATCH v2 2/6] proc: Generalize proc_sys_prune_dcache into proc_prune_siblings_dcache 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:28                                   ` [PATCH v2 4/6] proc: Use d_invalidate in proc_prune_siblings_dcache 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:29                                   ` [PATCH v2 6/6] proc: Use a list of inodes to flush from proc Eric W. Biederman
2020-02-28 20:17                                   ` [PATCH 0/3] proc: Actually honor the mount options 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                                     ` [PATCH 2/3] uml: Create a private mount of proc for mconsole Eric W. Biederman
2020-02-28 20:30                                       ` Christian Brauner
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:39                                       ` Christian Brauner
2020-02-28 21:40                                         ` Eric W. Biederman
2020-02-28 22:34                                     ` [PATCH 4/3] pid: Improve the comment about waiting in zap_pid_ns_processes 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-12 19:47           ` Al Viro
2020-02-11 22:45   ` Al Viro
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-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-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=87mu9a4obu.fsf@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --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=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=torvalds@linux-foundation.org \
    --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: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git