[PATCH v3 0/3] task_work_queue() with 2 users

[PATCH v3 0/3] task_work_queue() with 2 users

[Oleg Nesterov]

Hello.

Compared to v2

	1/3: make it (partly) useable without TIF_NOTIFY_RESUME

	2/3: address the comments from David

	3/3: New. Looks trivial, but I guess needs more testing.
	     And of course, needs the review from Thomas.

Oleg.

 include/linux/interrupt.h    |    4 --
 include/linux/key.h          |    6 +--
 include/linux/sched.h        |   12 ++----
 include/linux/task_work.h    |   32 ++++++++++++++++
 include/linux/tracehook.h    |   14 ++++++--
 kernel/Makefile              |    2 +-
 kernel/exit.c                |    5 ++-
 kernel/fork.c                |    1 +
 kernel/irq/manage.c          |   69 +++++++++++++++++------------------
 kernel/task_work.c           |   81 ++++++++++++++++++++++++++++++++++++++++++
 security/keys/internal.h     |    3 +-
 security/keys/keyctl.c       |   76 ++++++++++++++++++--------------------
 security/keys/process_keys.c |   25 ++++---------
 13 files changed, 213 insertions(+), 117 deletions(-)

[PATCH v3 1/3] task_work_queue: add generic process-context callbacks

[Oleg Nesterov]

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

The caller does task_work_queue(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_queue/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.

v3:
	- task_work_queue() gets the new arg, "bool notify" to
	  conditionalize set_notify_resume(), this makes it useable
	  for kthreads and task_work_queue(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_queue().

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_queue(notify => true) fails with -ENOTSUPP.

	  However, ->replacement_session_keyring equally needs
	  this logic, task_work_queue() is not worse.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/sched.h     |    2 +
 include/linux/task_work.h |   32 ++++++++++++++++++
 include/linux/tracehook.h |   14 ++++++--
 kernel/Makefile           |    2 +-
 kernel/exit.c             |    5 ++-
 kernel/fork.c             |    1 +
 kernel/task_work.c        |   81 +++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 132 insertions(+), 5 deletions(-)
 create mode 100644 include/linux/task_work.h
 create mode 100644 kernel/task_work.c

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 81a173c..be004ac 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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;
diff --git a/include/linux/task_work.h b/include/linux/task_work.h
new file mode 100644
index 0000000..8574f61
--- /dev/null
+++ b/include/linux/task_work.h
@@ -0,0 +1,32 @@
+#ifndef _LINUX_TASK_WORK_H
+#define _LINUX_TASK_WORK_H	1
+
+#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_queue(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(struct task_struct *task);
+
+static inline void exit_task_work(struct task_struct *task)
+{
+	if (unlikely(!hlist_empty(&task->task_works)))
+		task_work_run(task);
+}
+
+#endif	/* _LINUX_TASK_WORK_H */
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 51bd91d..dd9eca0 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -46,7 +46,7 @@
 #ifndef _LINUX_TRACEHOOK_H
 #define _LINUX_TRACEHOOK_H	1
 
-#include <linux/sched.h>
+#include <linux/task_work.h>
 #include <linux/ptrace.h>
 #include <linux/security.h>
 struct linux_binprm;
@@ -153,7 +153,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 +164,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 +185,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_queue()->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(current);
 }
-#endif	/* TIF_NOTIFY_RESUME */
 
 #endif	/* <linux/tracehook.h> */
diff --git a/kernel/Makefile b/kernel/Makefile
index cb41b95..5790f8b 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -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
diff --git a/kernel/exit.c b/kernel/exit.c
index d8bd3b4..dc852c2 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -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_queue() 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()))
diff --git a/kernel/fork.c b/kernel/fork.c
index b9372a0..d1108ac 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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
diff --git a/kernel/task_work.c b/kernel/task_work.c
new file mode 100644
index 0000000..0ca8054
--- /dev/null
+++ b/kernel/task_work.c
@@ -0,0 +1,81 @@
+#include <linux/tracehook.h>
+
+int
+task_work_queue(struct task_struct *task, struct task_work *twork, bool notify)
+{
+	unsigned long flags;
+	int err = -ESRCH;
+
+#ifndef TIF_NOTIFY_RESUME
+	if (notify)
+		return -ENOTSUPP;
+#endif
+	/*
+	 * We must not insert the new work if the task has already passed
+	 * exit_task_work(). We rely on do_exit()->raw_spin_unlock_wait()
+	 * and check PF_EXITING under pi_lock.
+	 */
+	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	if (likely(!(task->flags & PF_EXITING))) {
+		hlist_add_head(&twork->hlist, &task->task_works);
+		err = 0;
+	}
+	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+
+	/* test_and_set_bit() implies mb(), see tracehook_notify_resume(). */
+	if (likely(!err) && notify)
+		set_notify_resume(task);
+	return err;
+}
+
+struct task_work *
+task_work_cancel(struct task_struct *task, task_work_func_t func)
+{
+	unsigned long flags;
+	struct task_work *twork;
+	struct hlist_node *pos;
+
+	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	hlist_for_each_entry(twork, pos, &task->task_works, hlist) {
+		if (twork->func == func) {
+			hlist_del(&twork->hlist);
+			goto found;
+		}
+	}
+	twork = NULL;
+ found:
+	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+
+	return twork;
+}
+
+void task_work_run(struct task_struct *task)
+{
+	struct hlist_head task_works;
+	struct hlist_node *pos;
+
+	raw_spin_lock_irq(&task->pi_lock);
+	hlist_move_list(&task->task_works, &task_works);
+	raw_spin_unlock_irq(&task->pi_lock);
+
+	if (unlikely(hlist_empty(&task_works)))
+		return;
+	/*
+	 * We use hlist to save the space in task_struct, but we want fifo.
+	 * Find the last entry, the list should be short, then process them
+	 * in reverse order.
+	 */
+	for (pos = task_works.first; pos->next; pos = pos->next)
+		;
+
+	for (;;) {
+		struct hlist_node **pprev = pos->pprev;
+		struct task_work *twork = container_of(pos, struct task_work,
+							hlist);
+		twork->func(twork);
+
+		if (pprev == &task_works.first)
+			break;
+		pos = container_of(pprev, struct hlist_node, next);
+	}
+}
-- 
1.5.5.1

Re: [PATCH v3 1/3] task_work_queue: add generic process-context callbacks

[Andrew Morton]

<pays some attention this time>

On Thu, 12 Apr 2012 23:12:08 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> Provide a simple mechanism that allows running code in the
> (nonatomic) context of the arbitrary task.
> 
> The caller does task_work_queue(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.

So if the task-work was kmalloced, it is the callback function's
responsibility to kfree it?  task_work_run() appears to handle that OK,
without any use-after-free.

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

I'm not seeing anything in the changelogs which explains what was wrong
with ->replacement_session_keyring, so the motivation for this patchset
eludes me.

The immediate reaction is "what's wrong with include/linux/notifier.h".
Presumably this has been discussed on some mailing list somewhere ;)


I was wondering if taskstats_exit() could use this.  But the call
happens from the wrong place.  And therein lies a problem with the
proposal: the callback site will be in the wrong place for many
potential users.  I suspect this new code won't be as useful as we hope
- perhaps we should stick with the current scheme of open-coded callouts
to subsystem-specific sites from within do_exit().

>
> ...
>
> --- /dev/null
> +++ b/include/linux/task_work.h
> @@ -0,0 +1,32 @@
> +#ifndef _LINUX_TASK_WORK_H
> +#define _LINUX_TASK_WORK_H	1
> +
> +#include <linux/sched.h>

Could just forward-declare struct task_struct, afaict.

This header doesn't include sufficient prerequisite headers, so
everything will deservedly explode if you make that change.

>
> ...
>
> --- a/include/linux/tracehook.h
> +++ b/include/linux/tracehook.h
> @@ -46,7 +46,7 @@
>  #ifndef _LINUX_TRACEHOOK_H
>  #define _LINUX_TRACEHOOK_H	1
>  
> -#include <linux/sched.h>
> +#include <linux/task_work.h>

ick.  So tracehook.h gains a secret dependency upon task_work.h
including sched.h.

>
> ...
>
> --- /dev/null
> +++ b/kernel/task_work.c
> @@ -0,0 +1,81 @@
> +#include <linux/tracehook.h>

This file also doesn't come close to including the headers which it
requires.  This practice is fragile and frequently results in sad
little patches months later.  Often to fix the alpha build, for unknown
reasons.


Having to spell out all the includes is rather a pain.  Inevitably some
are missed and some remain after they are no longer needed.  otoh if we
don't do this, things tend to break when unexpected configs are used. 
There's no nice solution, alas.

> +int
> +task_work_queue(struct task_struct *task, struct task_work *twork, bool notify)

hm, I see.  The "queue" in "task_work_queue" is "the action of
queueing", not "a queue".  Noun-vs-verb.  Perhaps task_work_add() would
be clearer.

> +{
> +	unsigned long flags;
> +	int err = -ESRCH;
> +
> +#ifndef TIF_NOTIFY_RESUME
> +	if (notify)
> +		return -ENOTSUPP;
> +#endif
> +	/*
> +	 * We must not insert the new work if the task has already passed
> +	 * exit_task_work(). We rely on do_exit()->raw_spin_unlock_wait()
> +	 * and check PF_EXITING under pi_lock.
> +	 */
> +	raw_spin_lock_irqsave(&task->pi_lock, flags);

It's unobvious (to this little reader) why the code uses raw_ locking. 
The raw_spin_unlock_wait() thing prevents us from using
spin_lock_irqsave()?  Add a nice comment?

> +	if (likely(!(task->flags & PF_EXITING))) {
> +		hlist_add_head(&twork->hlist, &task->task_works);
> +		err = 0;
> +	}
> +	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
> +
> +	/* test_and_set_bit() implies mb(), see tracehook_notify_resume(). */
> +	if (likely(!err) && notify)
> +		set_notify_resume(task);
> +	return err;
> +}
> +
> +struct task_work *
> +task_work_cancel(struct task_struct *task, task_work_func_t func)
> +{
> +	unsigned long flags;
> +	struct task_work *twork;
> +	struct hlist_node *pos;
> +
> +	raw_spin_lock_irqsave(&task->pi_lock, flags);
> +	hlist_for_each_entry(twork, pos, &task->task_works, hlist) {
> +		if (twork->func == func) {
> +			hlist_del(&twork->hlist);
> +			goto found;
> +		}
> +	}

I trust we won't be giving userspace a way of controlling the queue
length!

> +	twork = NULL;
> + found:
> +	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
> +
> +	return twork;
> +}
> +
> +void task_work_run(struct task_struct *task)
> +{
> +	struct hlist_head task_works;
> +	struct hlist_node *pos;
> +
> +	raw_spin_lock_irq(&task->pi_lock);
> +	hlist_move_list(&task->task_works, &task_works);
> +	raw_spin_unlock_irq(&task->pi_lock);
> +
> +	if (unlikely(hlist_empty(&task_works)))
> +		return;
> +	/*
> +	 * We use hlist to save the space in task_struct, but we want fifo.

hm.  How does the reader of this code work out why the author wanted fifo?
Perhaps because keyctl_session_to_parent() or exit_irq_thread() needed this.

> +	 * Find the last entry, the list should be short, then process them
> +	 * in reverse order.
> +	 */
> +	for (pos = task_works.first; pos->next; pos = pos->next)
> +		;
> +
> +	for (;;) {
> +		struct hlist_node **pprev = pos->pprev;
> +		struct task_work *twork = container_of(pos, struct task_work,
> +							hlist);
> +		twork->func(twork);
> +
> +		if (pprev == &task_works.first)
> +			break;
> +		pos = container_of(pprev, struct hlist_node, next);
> +	}
> +}

Re: [PATCH v3 1/3] task_work_queue: add generic process-context callbacks

[Oleg Nesterov]

On 04/12, Andrew Morton wrote:
>
> <pays some attention this time>

Thanks!

> On Thu, 12 Apr 2012 23:12:08 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Provide a simple mechanism that allows running code in the
> > (nonatomic) context of the arbitrary task.
> >
> > The caller does task_work_queue(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.
>
> So if the task-work was kmalloced, it is the callback function's
> responsibility to kfree it?

Yes,

> task_work_run() appears to handle that OK,
> without any use-after-free.

I hope ;)

> > This allows us to kill the ->replacement_session_keyring hack,
> > and potentially this can have more users.
>
> I'm not seeing anything in the changelogs which explains what was wrong
> with ->replacement_session_keyring, so the motivation for this patchset
> eludes me.

->replacement_session_keyring in fact implements the notifier too,
just it is very limited. task_work simply tries to generalize and
cleanup the logic we already have. And at the same cost.

Besides, the current code is racy. And it is not convenient to fix
all users in arch/.

> The immediate reaction is "what's wrong with include/linux/notifier.h".
> Presumably this has been discussed on some mailing list somewhere ;)

Ah, please no. Imho we do not want _notifier_head in task_struct,
plus I'd certainly like to avoid the new locks in
do_exit()->exit_task_work() path.

> I was wondering if taskstats_exit() could use this.

Unlikely, I think. taskstats is "global", every task should report
if there are listeners.

> But the call
> happens from the wrong place.  And therein lies a problem with the
> proposal: the callback site will be in the wrong place for many
> potential users.  I suspect this new code won't be as useful as we hope
> - perhaps we should stick with the current scheme of open-coded callouts
> to subsystem-specific sites from within do_exit().

May be we can add more task_work_run's later, but right now I do
not think this is needed. And I do not expect it will find a lot
of new users.

> > --- a/include/linux/tracehook.h
> > +++ b/include/linux/tracehook.h
> > @@ -46,7 +46,7 @@
> >  #ifndef _LINUX_TRACEHOOK_H
> >  #define _LINUX_TRACEHOOK_H	1
> >
> > -#include <linux/sched.h>
> > +#include <linux/task_work.h>
>
> ick.  So tracehook.h gains a secret dependency upon task_work.h
> including sched.h.

Hmm. I thought that it is alway fine to remove the unnecessary include.
I can put it back, but we are going to kill tracehook.h altogether.

> > --- /dev/null
> > +++ b/kernel/task_work.c
> > @@ -0,0 +1,81 @@
> > +#include <linux/tracehook.h>
>
> This file also doesn't come close to including the headers which it
> requires.  This practice is fragile and frequently results in sad
> little patches months later.  Often to fix the alpha build, for unknown
> reasons.

OK, I'll add "include linix/task_work.h".

> > +int
> > +task_work_queue(struct task_struct *task, struct task_work *twork, bool notify)
>
> hm, I see.  The "queue" in "task_work_queue" is "the action of
> queueing", not "a queue".  Noun-vs-verb.

Cough. Peter, do you read this? I stole the naming and the first
line of the changelog from kernel/irq_work.c!

> Perhaps task_work_add() would
> be clearer.

OK, I'll rename.

> > +	unsigned long flags;
> > +	int err = -ESRCH;
> > +
> > +#ifndef TIF_NOTIFY_RESUME
> > +	if (notify)
> > +		return -ENOTSUPP;
> > +#endif
> > +	/*
> > +	 * We must not insert the new work if the task has already passed
> > +	 * exit_task_work(). We rely on do_exit()->raw_spin_unlock_wait()
> > +	 * and check PF_EXITING under pi_lock.
> > +	 */
> > +	raw_spin_lock_irqsave(&task->pi_lock, flags);
>
> It's unobvious (to this little reader) why the code uses raw_ locking.
> The raw_spin_unlock_wait() thing prevents us from using
> spin_lock_irqsave()?  Add a nice comment?

Well, but ->pi_lock is raw_spinlock_t ?

> > +struct task_work *
> > +task_work_cancel(struct task_struct *task, task_work_func_t func)
> > +{
> > +	unsigned long flags;
> > +	struct task_work *twork;
> > +	struct hlist_node *pos;
> > +
> > +	raw_spin_lock_irqsave(&task->pi_lock, flags);
> > +	hlist_for_each_entry(twork, pos, &task->task_works, hlist) {
> > +		if (twork->func == func) {
> > +			hlist_del(&twork->hlist);
> > +			goto found;
> > +		}
> > +	}
>
> I trust we won't be giving userspace a way of controlling the queue
> length!

Exactly. From 2/3:

	Note that we do task_work_cancel() before task_work_queue() 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.


> > +void task_work_run(struct task_struct *task)
> > +{
> > +	struct hlist_head task_works;
> > +	struct hlist_node *pos;
> > +
> > +	raw_spin_lock_irq(&task->pi_lock);
> > +	hlist_move_list(&task->task_works, &task_works);
> > +	raw_spin_unlock_irq(&task->pi_lock);
> > +
> > +	if (unlikely(hlist_empty(&task_works)))
> > +		return;
> > +	/*
> > +	 * We use hlist to save the space in task_struct, but we want fifo.
>
> hm.  How does the reader of this code work out why the author wanted fifo?
> Perhaps because keyctl_session_to_parent() or exit_irq_thread() needed this.

Argh.

No, keyctl_session_to_parent() and exit_irq_thread() do not care,
they can't add more than one work.

Just I think that fifo makes more sense in general. Just for example,
suppose that keyctl_session_to_parent() didn't cancel the pending
request(s). In this case fifo would be needed for correctness.

Oleg.

Re: [PATCH v3 1/3] task_work_queue: add generic process-context callbacks

[Peter Zijlstra]

On Fri, 2012-04-13 at 01:15 +0200, Oleg Nesterov wrote:
> > hm, I see.  The "queue" in "task_work_queue" is "the action of
> > queueing", not "a queue".  Noun-vs-verb.
> 
> Cough. Peter, do you read this? I stole the naming and the first
> line of the changelog from kernel/irq_work.c! 

Am now.. right, so that was modeled after queue_work(), although I now
realize I reversed the words and made it irq_work_queue() instead of
queue_irq_work().

I'd prefer to keep the queue thing as per workqueue and irq_work etc..

[PATCH v3 2/3] cred: change keyctl_session_to_parent() to use task_work_queue()

[Oleg Nesterov]

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

Note that we do task_work_cancel() before task_work_queue() 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 <oleg@redhat.com>
---
 include/linux/key.h          |    6 +--
 security/keys/internal.h     |    3 +-
 security/keys/keyctl.c       |   76 ++++++++++++++++++++----------------------
 security/keys/process_keys.c |   25 ++++----------
 4 files changed, 47 insertions(+), 63 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 96933b1..0c263d6 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -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__ */
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 65647f8..a81c319 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -12,7 +12,7 @@
 #ifndef _INTERNAL_H
 #define _INTERNAL_H
 
-#include <linux/sched.h>
+#include <linux/task_work.h>
 #include <linux/key-type.h>
 
 #ifdef __KDEBUG
@@ -148,6 +148,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;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index fb767c6..5ccfff5 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1423,50 +1423,57 @@ long keyctl_get_security(key_serial_t keyid,
  */
 long keyctl_session_to_parent(void)
 {
-#ifdef TIF_NOTIFY_RESUME
 	struct task_struct *me, *parent;
 	const struct cred *mycred, *pcred;
-	struct cred *cred, *oldcred;
+	struct task_work *newwork, *oldwork;
 	key_ref_t keyring_r;
-	int ret;
+	struct cred *cred;
+	int err;
 
 	keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0, KEY_LINK);
 	if (IS_ERR(keyring_r))
 		return PTR_ERR(keyring_r);
 
+	err = -ENOMEM;
+	newwork = kmalloc(sizeof(struct task_work), GFP_KERNEL);
+	if (!newwork)
+		goto error_keyring;
+
 	/* our parent is going to need a new cred struct, a new tgcred struct
 	 * and new security data, so we allocate them here to prevent ENOMEM in
 	 * our parent */
-	ret = -ENOMEM;
 	cred = cred_alloc_blank();
 	if (!cred)
 		goto error_keyring;
 
 	cred->tgcred->session_keyring = key_ref_to_ptr(keyring_r);
-	keyring_r = NULL;
+	init_task_work(newwork, key_change_session_keyring, cred);
 
 	me = current;
 	rcu_read_lock();
 	write_lock_irq(&tasklist_lock);
 
+	err = -EPERM;
+	oldwork = NULL;
 	parent = me->real_parent;
-	ret = -EPERM;
 
 	/* the parent mustn't be init and mustn't be a kernel thread */
 	if (parent->pid <= 1 || !parent->mm)
-		goto not_permitted;
+		goto unlock;
 
 	/* the parent must be single threaded */
 	if (!thread_group_empty(parent))
-		goto not_permitted;
+		goto unlock;
 
 	/* the parent and the child must have different session keyrings or
 	 * there's no point */
 	mycred = current_cred();
 	pcred = __task_cred(parent);
 	if (mycred == pcred ||
-	    mycred->tgcred->session_keyring == pcred->tgcred->session_keyring)
-		goto already_same;
+	    mycred->tgcred->session_keyring == pcred->tgcred->session_keyring) {
+		err = 0;
+		goto unlock;
+	}
 
 	/* the parent must have the same effective ownership and mustn't be
 	 * SUID/SGID */
@@ -1476,50 +1483,39 @@ long keyctl_session_to_parent(void)
 	    pcred->gid	!= mycred->egid	||
 	    pcred->egid	!= mycred->egid	||
 	    pcred->sgid	!= mycred->egid)
-		goto not_permitted;
+		goto unlock;
 
 	/* the keyrings must have the same UID */
 	if ((pcred->tgcred->session_keyring &&
 	     pcred->tgcred->session_keyring->uid != mycred->euid) ||
 	    mycred->tgcred->session_keyring->uid != mycred->euid)
-		goto not_permitted;
+		goto unlock;
 
-	/* if there's an already pending keyring replacement, then we replace
-	 * that */
-	oldcred = parent->replacement_session_keyring;
+	/* cancel an already pending keyring replacement */
+	oldwork = task_work_cancel(parent, key_change_session_keyring);
 
 	/* the replacement session keyring is applied just prior to userspace
 	 * restarting */
-	parent->replacement_session_keyring = cred;
-	cred = NULL;
-	set_ti_thread_flag(task_thread_info(parent), TIF_NOTIFY_RESUME);
-
-	write_unlock_irq(&tasklist_lock);
-	rcu_read_unlock();
-	if (oldcred)
-		put_cred(oldcred);
-	return 0;
-
-already_same:
-	ret = 0;
-not_permitted:
+	err = task_work_queue(parent, newwork, true);
+	if (!err)
+		newwork = NULL;
+unlock:
 	write_unlock_irq(&tasklist_lock);
 	rcu_read_unlock();
-	put_cred(cred);
-	return ret;
+	if (oldwork) {
+		put_cred(oldwork->data);
+		kfree(oldwork);
+	}
+	if (newwork) {
+		put_cred(newwork->data);
+		kfree(newwork);
+	}
+	return err;
 
 error_keyring:
+	kfree(newwork);
 	key_ref_put(keyring_r);
-	return ret;
-
-#else /* !TIF_NOTIFY_RESUME */
-	/*
-	 * To be removed when TIF_NOTIFY_RESUME has been implemented on
-	 * m68k/xtensa
-	 */
-#warning TIF_NOTIFY_RESUME not implemented
-	return -EOPNOTSUPP;
-#endif /* !TIF_NOTIFY_RESUME */
+	return err;
 }
 
 /*
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index be7ecb2..eb6d804 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -828,27 +828,17 @@ error:
 	return ret;
 }
 
-/*
- * 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;
+	const struct cred *old = current_cred();
+	struct cred *new = twork->data;
 
-	if (!current->replacement_session_keyring)
-		return;
-
-	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;
@@ -873,6 +863,5 @@ void key_replace_session_keyring(void)
 	new->tgcred->process_keyring = key_get(old->tgcred->process_keyring);
 
 	security_transfer_creds(new, old);
-
 	commit_creds(new);
 }
-- 
1.5.5.1

[PATCH v3 3/3] genirq: reimplement exit_irq_thread() hook via task_work_queue()

[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, simplification 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_queue_work(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 <oleg@redhat.com>
---
 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(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 2aea5d2..1cdd4d0 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -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 *);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index be004ac..e36edfb 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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 */
diff --git a/kernel/exit.c b/kernel/exit.c
index dc852c2..c1ca919 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -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),
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 89a3ea8..8b0d648 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
+#include <linux/task_work.h>
 
 #include "internals.h"
 
@@ -773,11 +774,39 @@ static void wake_threads_waitq(struct irq_desc *desc)
 		wake_up(&desc->wait_for_threads);
 }
 
+static void irq_thread_dtor(struct task_work *unused)
+{
+	struct task_struct *tsk = current;
+	struct irq_desc *desc;
+	struct irqaction *action;
+
+	if (WARN_ON_ONCE(!(current->flags & PF_EXITING)))
+		return;
+
+	action = kthread_data(tsk);
+
+	printk(KERN_ERR
+	       "exiting task \"%s\" (%d) is an active IRQ thread (irq %d)\n",
+	       tsk->comm ? tsk->comm : "", tsk->pid, action->irq);
+
+	desc = irq_to_desc(action->irq);
+	/*
+	 * If IRQTF_RUNTHREAD is set, we need to decrement
+	 * desc->threads_active and wake possible waiters.
+	 */
+	if (test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags))
+		wake_threads_waitq(desc);
+
+	/* Prevent a stale desc->threads_oneshot */
+	irq_finalize_oneshot(desc, action);
+}
+
 /*
  * Interrupt handler thread
  */
 static int irq_thread(void *data)
 {
+	struct task_work on_exit_work;
 	static const struct sched_param param = {
 		.sched_priority = MAX_USER_RT_PRIO/2,
 	};
@@ -793,7 +822,9 @@ static int irq_thread(void *data)
 		handler_fn = irq_thread_fn;
 
 	sched_setscheduler(current, SCHED_FIFO, &param);
-	current->irq_thread = 1;
+
+	init_task_work(&on_exit_work, irq_thread_dtor, NULL);
+	task_work_queue(current, &on_exit_work, false);
 
 	while (!irq_wait_for_interrupt(action)) {
 		irqreturn_t action_ret;
@@ -815,45 +846,11 @@ static int irq_thread(void *data)
 	 * cannot touch the oneshot mask at this point anymore as
 	 * __setup_irq() might have given out currents thread_mask
 	 * again.
-	 *
-	 * Clear irq_thread. Otherwise exit_irq_thread() would make
-	 * fuzz about an active irq thread going into nirvana.
 	 */
-	current->irq_thread = 0;
+	task_work_cancel(current, irq_thread_dtor);
 	return 0;
 }
 
-/*
- * Called from do_exit()
- */
-void exit_irq_thread(void)
-{
-	struct task_struct *tsk = current;
-	struct irq_desc *desc;
-	struct irqaction *action;
-
-	if (!tsk->irq_thread)
-		return;
-
-	action = kthread_data(tsk);
-
-	printk(KERN_ERR
-	       "exiting task \"%s\" (%d) is an active IRQ thread (irq %d)\n",
-	       tsk->comm ? tsk->comm : "", tsk->pid, action->irq);
-
-	desc = irq_to_desc(action->irq);
-
-	/*
-	 * If IRQTF_RUNTHREAD is set, we need to decrement
-	 * desc->threads_active and wake possible waiters.
-	 */
-	if (test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags))
-		wake_threads_waitq(desc);
-
-	/* Prevent a stale desc->threads_oneshot */
-	irq_finalize_oneshot(desc, action);
-}
-
 static void irq_setup_forced_threading(struct irqaction *new)
 {
 	if (!force_irqthreads)
-- 
1.5.5.1

Re: [PATCH v3 3/3] genirq: reimplement exit_irq_thread() hook via task_work_queue()

[Thomas Gleixner]

On Thu, 12 Apr 2012, Oleg Nesterov wrote:

> 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, simplification 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_queue_work(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 <oleg@redhat.com>
> ---
>  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(-)

Nice cleanup.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>