linux-kernel - Re: [PATCH v3 3/6] exit: postpone tty_kref_put() until after tasklist

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250204112236.GA6031@redhat.com>
Date: Tue, 4 Feb 2025 12:22:37 +0100
From: Oleg Nesterov <oleg@...hat.com>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: ebiederm@...ssion.com, brauner@...nel.org, akpm@...ux-foundation.org,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/6] exit: postpone tty_kref_put() until after
 tasklist_lock is dropped

On 02/03, Mateusz Guzik wrote:
>
> On Mon, Feb 3, 2025 at 7:07 PM Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > On 02/01, Mateusz Guzik wrote:
> > >
> > > Instead of smuggling the tty pointer directly, use a struct so that more
> > > things can be added later.
> >
> > I am not sure this particular change worth the effort, but I won't argue.
> > I'd like to know what Eric thinks.
> >
>
> it trivially whacks an atomic from an area protected by a global lock

Yes, but I am not sure this can make a noticeable difference... Let
me repeat, I am not really arguing, I leave this to you and Eric.
The patch looks correct and I have already acked it. Just it looks
"less obvious" to me than other changes in this series.

And even if we do this, I am not sure we need the new
"struct release_task_post", it seems we can simply move
tty_kref_put() from __exit_signal() to release_task(), see the
patch at the end.

Nobody can touch signal->tty after the group leader passes __exit_signal(),
if nothing else lock_task_sighand() can't succeed.

But again, feel free to ignore.

Oleg.

--- x/kernel/exit.c
+++ x/kernel/exit.c
@@ -146,7 +146,6 @@ static void __exit_signal(struct task_st
 	struct signal_struct *sig = tsk->signal;
 	bool group_dead = thread_group_leader(tsk);
 	struct sighand_struct *sighand;
-	struct tty_struct *tty;
 	u64 utime, stime;
 
 	sighand = rcu_dereference_check(tsk->sighand,
@@ -159,10 +158,7 @@ static void __exit_signal(struct task_st
 		posix_cpu_timers_exit_group(tsk);
 #endif
 
-	if (group_dead) {
-		tty = sig->tty;
-		sig->tty = NULL;
-	} else {
+	if (!group_dead) {
 		/*
 		 * If there is any task waiting for the group exit
 		 * then notify it:
@@ -210,10 +206,8 @@ static void __exit_signal(struct task_st
 
 	__cleanup_sighand(sighand);
 	clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
-	if (group_dead) {
+	if (group_dead)
 		flush_sigqueue(&sig->shared_pending);
-		tty_kref_put(tty);
-	}
 }
 
 static void delayed_put_task_struct(struct rcu_head *rhp)
@@ -279,6 +273,10 @@ repeat:
 	proc_flush_pid(thread_pid);
 	put_pid(thread_pid);
 	release_thread(p);
+
+	if (p == leader)
+		tty_kref_put(p->signal->tty);
+
 	put_task_struct_rcu_user(p);
 
 	p = leader;