Skip to contentSkip to conversation

task_work_add: generic process-context callbacks

PATCH Superseded Oleg Nesterov 3 participants 7 messages Apr 19, 2012 – Apr 30, 2012

Subsystems (7)
MM core Tracing Scheduler Security (LSM) +3 more subsystems

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

v5 cover task_work_add: generic process-context callbacks

Hello.

Hopefully the final version.

Changes:

1/3: task_work_run(void) has no "tsk" argument,
it uses current explicitely

2/3: fix the typo in the changelog

3/3: restore the accidently deleted comment,
rename "int err" back to "ret"

Oleg.

author
#1 lore raw

PATCH v5 1/3 task_work_add: generic process-context callbacks

Provide a simple mechanism that allows running code in the (nonatomic) context of the arbitrary task.

The caller does task_work_add(task, task_work) and this task executes task_work->func() either from do_notify_resume() or from do_exit(). The callback can rely on PF_EXITING to detect the latter case.

"struct task_work" can be embedded in another struct, still it has "void *data" to handle the most common/simple case.

This allows us to kill the ->replacement_session_keyring hack, and potentially this can have more users.

Performance-wise, this adds 2 "unlikely(!hlist_empty())" checks into tracehook_notify_resume() and do_exit(). But at the same time we can remove the "replacement_session_keyring != NULL" checks from arch/*/signal.c and exit_creds().

Note: task_work_add/task_work_run abuses ->pi_lock. This is only because this lock is already used by lookup_pi_state() to synchronize with do_exit() setting PF_EXITING. Fortunately the scope of this lock in task_work.c is really tiny, and the code is unlikely anyway.

v2:
- implement task_work_cancel(func), it removes the first task_work with the same callback.
v3:
- task_work_add() gets the new arg, "bool notify" to conditionalize set_notify_resume(), this makes it useable for kthreads and task_work_add(notify => false) can work without TIF_NOTIFY_RESUME.

- don't add the dummy "ifndef TIF_NOTIFY_RESUME" inlines, just add the simple check in task_work_add().
v4:
- s/task_work_queue/task_work_add/
v5:
- task_work_run() uses current explicitely

Todo:
- move clear_thread_flag(TIF_NOTIFY_RESUME) from arch/ to tracehook_notify_resume()

- rename tracehook_notify_resume() and move it into linux/task_work.h

- m68k and xtensa don't have TIF_NOTIFY_RESUME and thus task_work_add(notify => true) fails with -ENOTSUPP.

However, ->replacement_session_keyring equally needs this flag, task_work_add() is not worse.

Signed-off-by Oleg Nesterov <[email protected]>
---
 include/linux/sched.h     |    2 +
 include/linux/task_work.h |   33 +++++++++++++++++
 include/linux/tracehook.h |   13 ++++++-
 kernel/Makefile           |    2 +-
 kernel/exit.c             |    5 ++-
 kernel/fork.c             |    1 +
 kernel/task_work.c        |   84 +++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 136 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/task_work.h
 create mode 100644 kernel/task_work.c
include/linux/sched.h +2 -0
index 81a173c..be004ac 100644
@@ -1445,6 +1445,8 @@ struct task_struct {
 	int (*notifier)(void *priv);
 	void *notifier_data;
 	sigset_t *notifier_mask;
+	struct hlist_head task_works;
+
 	struct audit_context *audit_context;
 #ifdef CONFIG_AUDITSYSCALL
 	uid_t loginuid;
include/linux/task_work.h new file +33 -0
new file mode 100644
index 0000000..294d5d5
@@ -0,0 +1,33 @@
+#ifndef _LINUX_TASK_WORK_H
+#define _LINUX_TASK_WORK_H
+
+#include <linux/list.h>
+#include <linux/sched.h>
+
+struct task_work;
+typedef void (*task_work_func_t)(struct task_work *);
+
+struct task_work {
+	struct hlist_node hlist;
+	task_work_func_t func;
+	void *data;
+};
+
+static inline void
+init_task_work(struct task_work *twork, task_work_func_t func, void *data)
+{
+	twork->func = func;
+	twork->data = data;
+}
+
+int task_work_add(struct task_struct *task, struct task_work *twork, bool);
+struct task_work *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)
+{
+	if (unlikely(!hlist_empty(&task->task_works)))
+		task_work_run();
+}
+
+#endif	/* _LINUX_TASK_WORK_H */
include/linux/tracehook.h +11 -2
index 51bd91d..6a4d82b 100644
@@ -49,6 +49,7 @@
 #include <linux/sched.h>
 #include <linux/ptrace.h>
 #include <linux/security.h>
+#include <linux/task_work.h>
 struct linux_binprm;
 
 /*

@@ -153,7 +154,6 @@ static inline void tracehook_signal_handler(int sig, siginfo_t *info,
 		ptrace_notify(SIGTRAP);
 }
 
-#ifdef TIF_NOTIFY_RESUME
 /**
  * set_notify_resume - cause tracehook_notify_resume() to be called
  * @task:		task that will call tracehook_notify_resume()

@@ -165,8 +165,10 @@ static inline void tracehook_signal_handler(int sig, siginfo_t *info,
  */
 static inline void set_notify_resume(struct task_struct *task)
 {
+#ifdef TIF_NOTIFY_RESUME
 	if (!test_and_set_tsk_thread_flag(task, TIF_NOTIFY_RESUME))
 		kick_process(task);
+#endif
 }
 
 /**

@@ -184,7 +186,14 @@ static inline void set_notify_resume(struct task_struct *task)
  */
 static inline void tracehook_notify_resume(struct pt_regs *regs)
 {
+	/*
+	 * The caller just cleared TIF_NOTIFY_RESUME. This barrier
+	 * pairs with task_work_add()->set_notify_resume() after
+	 * hlist_add_head(task->task_works);
+	 */
+	smp_mb__after_clear_bit();
+	if (unlikely(!hlist_empty(&current->task_works)))
+		task_work_run();
 }
-#endif	/* TIF_NOTIFY_RESUME */
 
 #endif	/* <linux/tracehook.h> */
kernel/Makefile +1 -1
index cb41b95..5790f8b 100644
@@ -10,7 +10,7 @@ obj-y     = fork.o exec_domain.o panic.o printk.o \
 	    kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \
 	    hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \
 	    notifier.o ksysfs.o cred.o \
-	    async.o range.o groups.o
+	    async.o range.o groups.o task_work.o
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace debug files and internal ftrace files
kernel/exit.c +4 -1
index d8bd3b4..b82c38e 100644
@@ -946,11 +946,14 @@ void do_exit(long code)
 	exit_signals(tsk);  /* sets PF_EXITING */
 	/*
 	 * tsk->flags are checked in the futex code to protect against
-	 * an exiting task cleaning up the robust pi futexes.
+	 * an exiting task cleaning up the robust pi futexes, and in
+	 * task_work_add() to avoid the race with exit_task_work().
 	 */
 	smp_mb();
 	raw_spin_unlock_wait(&tsk->pi_lock);
 
+	exit_task_work(tsk);
+
 	exit_irq_thread();
 
 	if (unlikely(in_atomic()))
kernel/fork.c +1 -0
index b9372a0..d1108ac 100644
@@ -1380,6 +1380,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	 */
 	p->group_leader = p;
 	INIT_LIST_HEAD(&p->thread_group);
+	INIT_HLIST_HEAD(&p->task_works);
 
 	/* Now that the task is set up, run cgroup callbacks if
 	 * necessary. We need to run them before the task is visible
kernel/task_work.c new file +84 -0

84 lines not on this page · raw message

Signature · 1 line
1.5.5.1
maintainer Acked-by on v5 1/3
#2 lore raw
Quoted 48 lines · Oleg Nesterov

Provide a simple mechanism that allows running code in the (nonatomic) context of the arbitrary task.

The caller does task_work_add(task, task_work) and this task executes task_work->func() either from do_notify_resume() or from do_exit(). The callback can rely on PF_EXITING to detect the latter case.

"struct task_work" can be embedded in another struct, still it has "void *data" to handle the most common/simple case.

This allows us to kill the ->replacement_session_keyring hack, and potentially this can have more users.

Performance-wise, this adds 2 "unlikely(!hlist_empty())" checks into tracehook_notify_resume() and do_exit(). But at the same time we can remove the "replacement_session_keyring != NULL" checks from arch/*/signal.c and exit_creds().

Note: task_work_add/task_work_run abuses ->pi_lock. This is only because this lock is already used by lookup_pi_state() to synchronize with do_exit() setting PF_EXITING. Fortunately the scope of this lock in task_work.c is really tiny, and the code is unlikely anyway.

v2:
- implement task_work_cancel(func), it removes the first task_work with the same callback.
v3:
- task_work_add() gets the new arg, "bool notify" to conditionalize set_notify_resume(), this makes it useable for kthreads and task_work_add(notify => false) can work without TIF_NOTIFY_RESUME.

- don't add the dummy "ifndef TIF_NOTIFY_RESUME" inlines, just add the simple check in task_work_add().
v4:
- s/task_work_queue/task_work_add/
v5:
- task_work_run() uses current explicitely

Todo:
- move clear_thread_flag(TIF_NOTIFY_RESUME) from arch/ to tracehook_notify_resume()

- rename tracehook_notify_resume() and move it into linux/task_work.h

Quoted 7 lines

- m68k and xtensa don't have TIF_NOTIFY_RESUME and thus task_work_add(notify => true) fails with -ENOTSUPP.

However, ->replacement_session_keyring equally needs this flag, task_work_add() is not worse.

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by David Howells <[email protected]>
author
#3 lore raw

PATCH v5 2/3 genirq: reimplement exit_irq_thread() hook via task_work_add()

In reply to Oleg Nesterov

exit_irq_thread() and task->irq_thread are needed to handle the unexpected (and unlikely) exit of irq-thread.

We can use task_work instead and make this all private to kernel/irq/manage.c, cleanup plus micro-optimization.

1. rename exit_irq_thread() to irq_thread_dtor(), make it static, and move it up before irq_thread().

2. change irq_thread() to do task_work_add(irq_thread_dtor) at the start and task_work_cancel() before return.

tracehook_notify_resume() can never play with kthreads, only do_exit()->exit_task_work() can call the callback and this is what we want.

3. remove task_struct->irq_thread and the special hook in do_exit().

Signed-off-by Oleg Nesterov <[email protected]>
Reviewed-by Thomas Gleixner <[email protected]>
---
 include/linux/interrupt.h |    4 --
 include/linux/sched.h     |   10 +-----
 kernel/exit.c             |    2 -
 kernel/irq/manage.c       |   69 +++++++++++++++++++++-----------------------
 4 files changed, 35 insertions(+), 50 deletions(-)
include/linux/interrupt.h +0 -4
index 2aea5d2..1cdd4d0 100644
@@ -142,8 +142,6 @@ request_any_context_irq(unsigned int irq, irq_handler_t handler,
 extern int __must_check
 request_percpu_irq(unsigned int irq, irq_handler_t handler,
 		   const char *devname, void __percpu *percpu_dev_id);
-
-extern void exit_irq_thread(void);
 #else
 
 extern int __must_check

@@ -177,8 +175,6 @@ request_percpu_irq(unsigned int irq, irq_handler_t handler,
 {
 	return request_irq(irq, handler, 0, devname, percpu_dev_id);
 }
-
-static inline void exit_irq_thread(void) { }
 #endif
 
 extern void free_irq(unsigned int, void *);
include/linux/sched.h +2 -8
index be004ac..e36edfb 100644
@@ -1346,11 +1346,6 @@ struct task_struct {
 	unsigned sched_reset_on_fork:1;
 	unsigned sched_contributes_to_load:1;
 
-#ifdef CONFIG_GENERIC_HARDIRQS
-	/* IRQ handler threads */
-	unsigned irq_thread:1;
-#endif
-
 	pid_t pid;
 	pid_t tgid;
 

@@ -1358,10 +1353,9 @@ struct task_struct {
 	/* Canary value for the -fstack-protector gcc feature */
 	unsigned long stack_canary;
 #endif
-
-	/* 
+	/*
 	 * pointers to (original) parent process, youngest child, younger sibling,
-	 * older sibling, respectively.  (p->father can be replaced with 
+	 * older sibling, respectively.  (p->father can be replaced with
 	 * p->real_parent->pid)
 	 */
 	struct task_struct __rcu *real_parent; /* real parent process */
kernel/exit.c +0 -2
index b82c38e..8135243 100644
@@ -954,8 +954,6 @@ void do_exit(long code)
 
 	exit_task_work(tsk);
 
-	exit_irq_thread();
-
 	if (unlikely(in_atomic()))
 		printk(KERN_INFO "note: %s[%d] exited with preempt_count %d\n",
 				current->comm, task_pid_nr(current),
kernel/irq/manage.c +33 -36

102 lines not on this page · raw message

Signature · 1 line
1.5.5.1
author
#4 lore raw

PATCH v5 3/3 cred: change keyctl_session_to_parent() to use task_work_add()

In reply to Oleg Nesterov

Change keyctl_session_to_parent() to use task_work_add() and move key_replace_session_keyring() logic into task_work->func().

Note that we do task_work_cancel() before task_work_add() to ensure that only one work can be pending at any time. This is important, we must not allow user-space to abuse the parent's ->task_works list.

The callback, replace_session_keyring(), checks PF_EXITING. I guess this is not really needed but looks better.

As a side effect, this fixes the (unlikely) race. The callers of key_replace_session_keyring() and keyctl_session_to_parent() lack the necessary barriers, the parent can miss the request.

Now we can remove task_struct->replacement_session_keyring and related code.

Signed-off-by Oleg Nesterov <[email protected]>
---
 include/linux/key.h          |    6 +--
 security/keys/internal.h     |    2 +
 security/keys/keyctl.c       |   73 ++++++++++++++++++++----------------------
 security/keys/process_keys.c |   20 ++++-------
 4 files changed, 46 insertions(+), 55 deletions(-)
include/linux/key.h +2 -4
index 96933b1..0c263d6 100644
@@ -33,6 +33,8 @@ typedef uint32_t key_perm_t;
 
 struct key;
 
+#define key_replace_session_keyring()	do { } while (0)
+
 #ifdef CONFIG_KEYS
 
 #undef KEY_DEBUGGING

@@ -302,9 +304,6 @@ static inline bool key_is_instantiated(const struct key *key)
 #ifdef CONFIG_SYSCTL
 extern ctl_table key_sysctls[];
 #endif
-
-extern void key_replace_session_keyring(void);
-
 /*
  * the userspace interface
  */

@@ -327,7 +326,6 @@ extern void key_init(void);
 #define key_fsuid_changed(t)		do { } while(0)
 #define key_fsgid_changed(t)		do { } while(0)
 #define key_init()			do { } while(0)
-#define key_replace_session_keyring()	do { } while(0)
 
 #endif /* CONFIG_KEYS */
 #endif /* __KERNEL__ */
security/keys/internal.h +2 -0
index 65647f8..c2986da 100644
@@ -14,6 +14,7 @@
 
 #include <linux/sched.h>
 #include <linux/key-type.h>
+#include <linux/task_work.h>
 
 #ifdef __KDEBUG
 #define kenter(FMT, ...) \

@@ -148,6 +149,7 @@ extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
 #define KEY_LOOKUP_FOR_UNLINK	0x04
 
 extern long join_session_keyring(const char *name);
+extern void key_change_session_keyring(struct task_work *twork);
 
 extern struct work_struct key_gc_work;
 extern unsigned key_gc_delay;
security/keys/keyctl.c +35 -38

135 lines not on this page · raw message

security/keys/process_keys.c +7 -13
index be7ecb2..7ab0d8b 100644
@@ -832,23 +832,17 @@ error:
  * Replace a process's session keyring on behalf of one of its children when
  * the target  process is about to resume userspace execution.
  */
-void key_replace_session_keyring(void)
+void key_change_session_keyring(struct task_work *twork)
 {
-	const struct cred *old;
-	struct cred *new;
-
-	if (!current->replacement_session_keyring)
-		return;
+	const struct cred *old = current_cred();
+	struct cred *new = twork->data;
 
-	write_lock_irq(&tasklist_lock);
-	new = current->replacement_session_keyring;
-	current->replacement_session_keyring = NULL;
-	write_unlock_irq(&tasklist_lock);
-
-	if (!new)
+	kfree(twork);
+	if (unlikely(current->flags & PF_EXITING)) {
+		put_cred(new);
 		return;
+	}
 
-	old = current_cred();
 	new->  uid	= old->  uid;
 	new-> euid	= old-> euid;
 	new-> suid	= old-> suid;
Signature · 1 line
1.5.5.1
maintainer Acked-by on v5 3/3
#5 lore raw

Re: [PATCH v5 3/3] cred: change keyctl_session_to_parent() to use task_work_add()

Quoted 11 lines · Oleg Nesterov

Change keyctl_session_to_parent() to use task_work_add() and move key_replace_session_keyring() logic into task_work->func().

Note that we do task_work_cancel() before task_work_add() to ensure that only one work can be pending at any time. This is important, we must not allow user-space to abuse the parent's ->task_works list.

The callback, replace_session_keyring(), checks PF_EXITING. I guess this is not really needed but looks better.

Quoted 8 lines

As a side effect, this fixes the (unlikely) race. The callers of key_replace_session_keyring() and keyctl_session_to_parent() lack the necessary barriers, the parent can miss the request.

Now we can remove task_struct->replacement_session_keyring and related code.

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by David Howells <[email protected]>