linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: LKML <linux-kernel@vger.kernel.org>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Alexey Gladkov <legion@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Alexey Gladkov <gladkov.alexey@gmail.com>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: [PATCH v2 2/2] proc: Ensure we see the exit of each process tid exactly
Date: Thu, 23 Apr 2020 14:39:41 -0500	[thread overview]
Message-ID: <87wo66vvnm.fsf_-_@x220.int.ebiederm.org> (raw)
In-Reply-To: <87ftcv1nqe.fsf@x220.int.ebiederm.org> (Eric W. Biederman's message of "Wed, 22 Apr 2020 11:36:41 -0500")


When the thread group leader changes during exec and the old leaders
thread is reaped proc_flush_pid will flush the dentries for the entire
process because the leader still has it's original pid.

Fix this by exchanging the pids in an rcu safe manner,
and wrapping the code to do that up in a helper exchange_tids.

When I removed switch_exec_pids and introduced this behavior
in d73d65293e3e ("[PATCH] pidhash: kill switch_exec_pids") there
really was nothing that cared as flushing happened with
the cached dentry and de_thread flushed both of them on exec.

This lack of fully exchanging pids became a problem a few months later
when I introduced 48e6484d4902 ("[PATCH] proc: Rewrite the proc dentry
flush on exit optimization").  Which overlooked the de_thread case
was no longer swapping pids, and I was looking up proc dentries
by task->pid.

The current behavior isn't properly a bug as everything in proc will
continue to work correctly just a little bit less efficiently.  Fix
this just so there are no little surprise corner cases waiting to bite
people.

-- Oleg points out this could be an issue in next_tgid in proc where
   has_group_leader_pid is called, and reording some of the assignments
   should fix that.

-- Oleg points out this will break the 10 year old hack in __exit_signal.c
>	/*
>	 * 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.
>	 */
>	if (unlikely(has_group_leader_pid(tsk)))
>		posix_cpu_timers_exit_group(tsk);

The code in next_tgid has been changed to use PIDTYPE_TGID,
and the posix cpu timers code has been fixed so it does not
need the 10 year old hack, so this should be safe to merge
now.

Fixes: 48e6484d4902 ("[PATCH] proc: Rewrite the proc dentry flush on exit optimization").
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/exec.c           |  5 +----
 include/linux/pid.h |  1 +
 kernel/pid.c        | 16 ++++++++++++++++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 06b4c550af5d..9b60f927afd7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1186,11 +1186,8 @@ static int de_thread(struct task_struct *tsk)
 
 		/* Become a process group leader with the old leader's pid.
 		 * The old leader becomes a thread of the this thread group.
-		 * Note: The old leader also uses this pid until release_task
-		 *       is called.  Odd but simple and correct.
 		 */
-		tsk->pid = leader->pid;
-		change_pid(tsk, PIDTYPE_PID, task_pid(leader));
+		exchange_tids(tsk, leader);
 		transfer_pid(leader, tsk, PIDTYPE_TGID);
 		transfer_pid(leader, tsk, PIDTYPE_PGID);
 		transfer_pid(leader, tsk, PIDTYPE_SID);
diff --git a/include/linux/pid.h b/include/linux/pid.h
index cc896f0fc4e3..2159ffca63fc 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -102,6 +102,7 @@ extern void attach_pid(struct task_struct *task, enum pid_type);
 extern void detach_pid(struct task_struct *task, enum pid_type);
 extern void change_pid(struct task_struct *task, enum pid_type,
 			struct pid *pid);
+extern void exchange_tids(struct task_struct *task, struct task_struct *old);
 extern void transfer_pid(struct task_struct *old, struct task_struct *new,
 			 enum pid_type);
 
diff --git a/kernel/pid.c b/kernel/pid.c
index c835b844aca7..4ece32d8791a 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -363,6 +363,22 @@ void change_pid(struct task_struct *task, enum pid_type type,
 	attach_pid(task, type);
 }
 
+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);
+	WRITE_ONCE(ntask->pid_links[PIDTYPE_PID].pprev, &opid->tasks[PIDTYPE_PID].first);
+	WRITE_ONCE(otask->pid_links[PIDTYPE_PID].pprev, &npid->tasks[PIDTYPE_PID].first);
+	WRITE_ONCE(ntask->pid, pid_nr(opid));
+	WRITE_ONCE(otask->pid, pid_nr(npid));
+}
+
 /* transfer_pid is an optimization of attach_pid(new), detach_pid(old) */
 void transfer_pid(struct task_struct *old, struct task_struct *new,
 			   enum pid_type type)
-- 
2.20.1


  parent reply	other threads:[~2020-04-23 19:42 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-19 14:10 [PATCH v12 0/7] proc: modernize proc to support multiple private instances Alexey Gladkov
2020-04-19 14:10 ` [PATCH v12 1/7] proc: rename struct proc_fs_info to proc_fs_opts Alexey Gladkov
2020-04-19 14:10 ` [PATCH v12 2/7] proc: allow to mount many instances of proc in one pid namespace Alexey Gladkov
2020-04-23 11:28   ` [PATCH v13 " Alexey Gladkov
2020-04-23 12:16     ` Eric W. Biederman
2020-04-23 20:01       ` Alexey Gladkov
2020-04-19 14:10 ` [PATCH v12 3/7] proc: instantiate only pids that we can ptrace on 'hidepid=4' mount option Alexey Gladkov
2020-04-19 14:10 ` [PATCH v12 4/7] proc: add option to mount only a pids subset Alexey Gladkov
2020-04-19 14:10 ` [PATCH v12 5/7] docs: proc: add documentation for "hidepid=4" and "subset=pid" options and new mount behavior Alexey Gladkov
2020-04-19 14:10 ` [PATCH v12 6/7] proc: use human-readable values for hidepid Alexey Gladkov
2020-04-19 14:10 ` [PATCH v12 7/7] proc: use named enums for better readability Alexey Gladkov
     [not found] ` <87ftcv1nqe.fsf@x220.int.ebiederm.org>
2020-04-23 17:54   ` [PATCH v2 0/2] proc: Calling proc_flush_task exactly once per task Oleg Nesterov
2020-04-23 19:38     ` Eric W. Biederman
2020-04-23 19:39   ` [PATCH v2 1/2] proc: Use PIDTYPE_TGID in next_tgid Eric W. Biederman
2020-04-24 17:29     ` Oleg Nesterov
2020-04-23 19:39   ` Eric W. Biederman [this message]
2020-04-23 20:28     ` [PATCH v2 2/2] proc: Ensure we see the exit of each process tid exactly Linus Torvalds
2020-04-24  3:33       ` Eric W. Biederman
2020-04-24 18:02         ` Linus Torvalds
2020-04-24 18:46           ` Linus Torvalds
2020-04-24 19:51           ` Eric W. Biederman
2020-04-24 20:10             ` Linus Torvalds
2020-04-24 17:39     ` Oleg Nesterov
2020-04-24 18:10       ` Eric W. Biederman
2020-04-24 20:50       ` [PATCH] proc: Put thread_pid in release_task not proc_flush_pid Eric W. Biederman
     [not found]       ` <87mu6ymkea.fsf_-_@x220.int.ebiederm.org>
     [not found]         ` <87blnemj5t.fsf_-_@x220.int.ebiederm.org>
2020-04-26 17:22           ` [PATCH v3 2/6] posix-cpu-timers: Use PIDTYPE_TGID to simplify the logic in lookup_task Oleg Nesterov
2020-04-27 11:51             ` Eric W. Biederman
2020-04-28 18:03               ` Oleg Nesterov
2020-04-27 10:32           ` Thomas Gleixner
2020-04-27 19:46             ` Eric W. Biederman
     [not found]         ` <875zdmmj4y.fsf_-_@x220.int.ebiederm.org>
2020-04-26 17:40           ` [PATCH v3 3/6] rculist: Add hlist_swap_before_rcu Linus Torvalds
2020-04-27 14:28             ` Eric W. Biederman
2020-04-27 20:27               ` Linus Torvalds
2020-04-28 12:16                 ` [PATCH v4 0/2] proc: Ensure we see the exit of each process tid exactly Eric W. Biederman
2020-04-28 12:18                   ` [PATCH v4 1/2] rculist: Add hlists_swap_heads_rcu Eric W. Biederman
2020-04-28 12:19                   ` [PATCH v4 2/2] proc: Ensure we see the exit of each process tid exactly once Eric W. Biederman
2020-04-28 16:53                   ` [PATCH v4 0/2] proc: Ensure we see the exit of each process tid exactly Linus Torvalds
2020-04-28 17:55                     ` Eric W. Biederman
2020-04-28 18:55                     ` Eric W. Biederman
2020-04-28 19:36                       ` Linus Torvalds
2020-04-28 18:05                   ` Oleg Nesterov
2020-04-28 18:54                     ` Eric W. Biederman
2020-04-28 21:39                     ` [PATCH v1 0/4] signal: Removing has_group_leader_pid Eric W. Biederman
2020-04-28 21:45                       ` [PATCH v1 1/4] posix-cpu-timer: Tidy up group_leader logic in lookup_task Eric W. Biederman
2020-04-28 21:48                       ` [PATCH 2/4] posix-cpu-timer: Unify the now redundant code " Eric W. Biederman
2020-04-28 21:53                       ` [PATCH v1 3/4] exec: Remove BUG_ON(has_group_leader_pid) Eric W. Biederman
2020-04-28 21:56                       ` [PATCH v4 4/4] signal: Remove has_group_leader_pid Eric W. Biederman
2020-04-30 11:54                       ` [PATCH v1 0/3] posix-cpu-timers: Use pids not tasks in lookup Eric W. Biederman
2020-04-30 11:55                         ` [PATCH v1 1/3] posix-cpu-timers: Extend rcu_read_lock removing task_struct references Eric W. Biederman
2020-04-30 11:56                         ` [PATCH v1 2/3] posix-cpu-timers: Replace cpu_timer_pid_type with clock_pid_type Eric W. Biederman
2020-04-30 11:56                         ` [PATCH v1 3/3] posix-cpu-timers: Replace __get_task_for_clock with pid_for_clock Eric W. Biederman
     [not found]         ` <87h7x6mj6h.fsf_-_@x220.int.ebiederm.org>
2020-04-27  9:43           ` [PATCH v3 1/6] posix-cpu-timers: Always call __get_task_for_clock holding rcu_read_lock Thomas Gleixner
2020-04-27 11:53             ` Eric W. Biederman
     [not found]         ` <87r1w8ete7.fsf@x220.int.ebiederm.org>
2020-04-27 20:23           ` [PATCH v3] proc: Ensure we see the exit of each process tid exactly Eric W. Biederman

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=87wo66vvnm.fsf_-_@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=gladkov.alexey@gmail.com \
    --cc=legion@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=torvalds@linux-foundation.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).