linux-kernel - Re: [RFC] exit: do exit_task_work() before shooting off mm

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: <ce5be208-99eb-f7bd-e602-9361008ff83c@gmail.com>
Date:   Sun, 20 Dec 2020 12:50:58 +0000
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-kernel@...r.kernel.org, Jens Axboe <axboe@...nel.dk>,
        Oleg Nesterov <oleg@...hat.com>
Subject: Re: [RFC] exit: do exit_task_work() before shooting off mm

On 08/12/2020 01:37, Al Viro wrote:
> On Thu, Dec 03, 2020 at 02:30:46AM +0000, Pavel Begunkov wrote:
>> Handle task works and lock it earlier before it starts killing off
>> task's resources like mm. io_uring makes use of it a lot and it'd
>> nicer to have all added task_work finding tasks in a consistent state.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
>> ---
>>
>> Would it be correct? I clearly don't know all the exit invariants, but
>> can't find any users relying on task_works in-between.
> 
> You've just gotten rid of exit_task_work() anywhere after exit_files().
> And exit_mm() can trigger the final fput() just as easily as exit_files().
> 
> IOW, you have just made the effect of final close on exit() completely
> asynchronous.

One more moment, after we've set PF_EXITING any task_work_run() would be
equivalent to exit_task_work()

static inline void exit_task_work(struct task_struct *task)
{
       task_work_run();
}

It sounds from your words that this is not expected, is it? io_uring
may want (currently doesn't) to run works for cancellation purposes.
Shouldn't it be like below (not tested)? Also simplifies task_work_run().

CC Oleg

diff --git a/include/linux/task_work.h b/include/linux/task_work.h
index 0d848a1e9e62..6aae0e6c3a04 100644
--- a/include/linux/task_work.h
+++ b/include/linux/task_work.h
@@ -24,10 +24,6 @@ int task_work_add(struct task_struct *task, struct callback_head *twork,
 
 struct callback_head *task_work_cancel(struct task_struct *, task_work_func_t);
 void task_work_run(void);
-
-static inline void exit_task_work(struct task_struct *task)
-{
-	task_work_run();
-}
+void exit_task_work(struct task_struct *task);
 
 #endif	/* _LINUX_TASK_WORK_H */
diff --git a/kernel/task_work.c b/kernel/task_work.c
index 9cde961875c0..60715f3d91a0 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -107,26 +107,15 @@ task_work_cancel(struct task_struct *task, task_work_func_t func)
 void task_work_run(void)
 {
 	struct task_struct *task = current;
-	struct callback_head *work, *head, *next;
+	struct callback_head *work, *next;
 
 	for (;;) {
-		/*
-		 * work->func() can do task_work_add(), do not set
-		 * work_exited unless the list is empty.
-		 */
 		do {
-			head = NULL;
 			work = READ_ONCE(task->task_works);
-			if (!work) {
-				if (task->flags & PF_EXITING)
-					head = &work_exited;
-				else
-					break;
-			}
-		} while (cmpxchg(&task->task_works, work, head) != work);
+			if (!work)
+				return;
+		} while (cmpxchg(&task->task_works, work, NULL) != work);
 
-		if (!work)
-			break;
 		/*
 		 * Synchronize with task_work_cancel(). It can not remove
 		 * the first entry == work, cmpxchg(task_works) must fail.
@@ -143,3 +132,17 @@ void task_work_run(void)
 		} while (work);
 	}
 }
+
+void exit_task_work(struct task_struct *task)
+{
+	WARN_ON(task != current);
+	WARN_ON(!(task->flags & PF_EXITING));
+	WARN_ON(READ_ONCE(task->task_works) == &work_exited);
+
+	/*
+	 * work->func() can do task_work_add(), do not set
+	 * work_exited unless the list is empty.
+	 */
+	while (cmpxchg(&task->task_works, NULL, &work_exited) != NULL)
+		task_work_run();
+}

-- 
Pavel Begunkov