task_work_add (was: task_work_queue)
Subsystems (7)
v4 cover task_work_add (was: task_work_queue)
Hello.
Changes based on review from Andrew:
- s/task_work_queue/task_work_add/
- don't create a secret dependency upon task_work.h including sched.h in tracehook.h and keys/internal.h
- add more includes into task_work.[ch] to not rely on "sched.h includes everything"
The genirq patch comes as 2/3, it was already acked by Thomas.
3/3 still needs the ack from David, although his last review was positive and I hope I fully addressed his comments.
Oleg.
PATCH v4 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/
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.
--- 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 include/linux/task_work.h create mode 100644 kernel/task_work.c
include/linux/sched.h +2 -0
@@ -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
@@ -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 (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 */
include/linux/tracehook.h +11 -2
@@ -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 (¤t->task_works))) +task_work_run (current); } -#endif /* TIF_NOTIFY_RESUME */ #endif /* <linux/tracehook.h> */
kernel/Makefile +1 -1
@@ -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
@@ -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
@@ -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 +83 -0
83 lines not on this page · raw message
Signature · 1 line
1.5.5.1
Re: [PATCH v4 1/3] task_work_add: generic process-context callbacks
This is seriously buggy:
Quoted 21 lines · Oleg Nesterov
kernel/task_work.c new file +20
-0+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 (;;) {
kernel/task_work.c line 83
Quoted 10 lines
kernel/task_work.c new file +10
-0+ 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); + } +}
No can do. You've removed the task-work from the process list, and you no longer hold the spinlock that protects that list. That means that you *cannot* access the task-work data structure any more, because it may long be gone.
Look at the users of this interface that you wrote yourself. They allocate the task-work on the stack, and do a "task_work_cancel()" before returning. That data structure is *gone*. You can't dereference it any more.
So quite frankly, the only safe approach is to copy the twork->func while holding the lock. And passing in the "twork" to the function isn't safe either, as far as I can see, since it may be gone too.
Basically, *any* access of 'twork' after it is removed from the list and you have released the task spinlock is unsafe, as far as I can tell.
Alternatively, you must make the rule be that the data can only be freed by the caller *if* it was returned from "task_work_cancel()". But then you can't allocate it on the stack any more, and have to allocate it separately.
Or you need to implement some kind of "task_work_cancel_sync()" function that guarantees that it waits for the actual work function to finish. And I don't know how you'd do that.
But as it is, this series looks seriously buggy.
Linus
Re: [PATCH v4 1/3] task_work_add: generic process-context callbacks
Quoted 2 lines · Linus Torvalds
This is seriously buggy:
I am already sleep. But,
Quoted 38 lines · Oleg Nesterov
Quoted 21 lines
kernel/task_work.c new file +20
-0+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 (;;) {kernel/task_work.c line 83
Quoted 10 lines
kernel/task_work.c new file +10
-0+ 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); + } +}No can do. You've removed the task-work from the process list, and you no longer hold the spinlock that protects that list. That means that you *cannot* access the task-work data structure any more, because it may long be gone.
Quoted 4 lines
Look at the users of this interface that you wrote yourself. They allocate the task-work on the stack, and do a "
task_work_cancel()" before returning. That data structure is *gone*. You can't dereference it any more.
tsk is always "current", probably this should be documented, I'll add the comment.
So this can't race with irq_thread() which uses the task_work on stack.
Quoted 3 lines
Basically, *any* access of 'twork' after it is removed from the list and you have released the task spinlock is unsafe, as far as I can tell.
I don't follow.
Once task_work_run() removes task_work from list (and drops the lock) nobody can use this twork. task_work_cancel obviously can't find it, it will return NULL.
Oleg.
David Howells
maintainerRe: [PATCH v4 1/3] task_work_add: generic process-context callbacks
Quoted 2 lines · Oleg Nesterov
tsk is always "current", probably this should be documented, I'll add the comment.
If tsk is always current, then don't pass it as a parameter. Some arches keep current in a register and it's much more efficient to use current directly in those cases - and those that don't have to be able to obtain it reasonably quickly.
David
Re: [PATCH v4 1/3] task_work_add: generic process-context callbacks
Quoted 7 lines · David Howells
Quoted 2 lines · Oleg Nesterov
tsk is always "current", probably this should be documented, I'll add the comment.
If tsk is always current, then don't pass it as a parameter.
This is already changed, I removed this arg.
Quoted 3 lines
Some arches keep
current in a register and it's much more efficient to use current directly in those cases
OK, thanks.
Oleg.
Re: [PATCH v4 1/3] task_work_add: generic process-context callbacks
In reply to Linus Torvaldsprobably I should also comment this part...
Quoted 3 lines · Linus Torvalds
Alternatively, you must make the rule be that the data can only be freed by the caller *if* it was returned from "
task_work_cancel()".
Exactly.
Once the caller does task_work_add(twork), it no longer "owns" this twork.
But, if task_work_cancel() succeeds - you own it again.
Quoted 2 lines
But then you can't allocate it on the stack any more, and have to allocate it separately.
Yes, unless you do task_work_add/cancel(current).
Oleg.
Re: [PATCH v4 1/3] task_work_add: generic process-context callbacks
Quoted 10 lines · Oleg Nesterov
Quoted 3 lines
Alternatively, you must make the rule be that the data can only be freed by the caller *if* it was returned from "
task_work_cancel()".Exactly.
Once the caller does
task_work_add(twork), it no longer "owns" this twork.But, if
task_work_cancel()succeeds - you own it again.
*IF* it succeeds.
My argument was that it might race, and not successfully remove the work entry at all - but because it was allocated on the stack, it gets freed *regardless* of whether the cancel got it or not.
Quoted 4 lines
Quoted 2 lines
But then you can't allocate it on the stack any more, and have to allocate it separately.
Yes, unless you do task_work_add/cancel(current).
Ok, your argument seems to be that "current" is special, and can not race, because the work execution is always synchronous with the task it got scheduled on.
I can buy that. But if so, it needs some big honking comment. Maybe the whole "add yourself onto your *own* task work" should be an explicit separate interface.
And that whole "run_task_work()" function should *not* take a "task" pointer, because it would be horribly horribly wrong to ever run it in any context than "current".
Linus
Re: [PATCH v4 1/3] task_work_add: generic process-context callbacks
Quoted 9 lines · Linus Torvalds
Quoted 5 lines · Oleg Nesterov
Quoted 1 line
Once the caller does
task_work_add(twork), it no longer "owns" this twork.But, if
task_work_cancel()succeeds - you own it again.*IF* it succeeds.
Sure.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
But then you can't allocate it on the stack any more, and have to allocate it separately.
Yes, unless you do task_work_add/cancel(current).
Ok, your argument seems to be that "current" is special, and can not race, because the work execution is always synchronous with the task it got scheduled on.
Yes, exactly.
Quoted 3 lines
And that whole "
run_task_work()" function should *not* take a "task" pointer, because it would be horribly horribly wrong to ever run it in any context than "current".
And it was task_work_queue(void) initially. But then I decided to micro-optimize this, the callers already have this task_struct in the register. And we have other examples like this, say, exit_mm().
However. I agree that it would be more understandable and clean to use current in task_work_run(void), and percpu_read is cheap.
So I'll remove this argument and send v5 after David reviews 3/3.
Oleg.
PATCH v4 2/3 genirq: reimplement exit_irq_thread() hook via task_work_add()
In reply to Oleg Nesterovexit_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_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().
--- 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
@@ -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
@@ -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
@@ -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
David Howells
maintainerRe: [PATCH v4 2/3] genirq: reimplement exit_irq_thread() hook via task_work_add()
Quoted 6 lines · Oleg Nesterov
2. change
irq_thread()to dotask_queue_work(irq_thread_dtor)at the start andtask_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.
Hmmm... This seems wrong. You're now using the hook in two distinct ways:
the primary use of that the hook is to detect that userspace is about to resume processing (via TIF_NOTIFY_RESUME) and then you're abusing the fact that the hook is also invoked via do_exit() to perform a clean up because we've got to get rid of it somehow under that circumstance.
This only works for you because you're operating in a kernel thread which doesn't have a userspace (and so will never see TIF_NOTIFY_RESUME). However, if someone tries that in an ordinary thread, it is liable to malfunction as the record could be executed and deleted at some unpredictable point in the future.
David
Re: [PATCH v4 2/3] genirq: reimplement exit_irq_thread() hook via task_work_add()
Quoted 11 lines · David Howells
Quoted 6 lines · Oleg Nesterov
2. change
irq_thread()to dotask_queue_work(irq_thread_dtor)at the start andtask_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.Hmmm... This seems wrong.
I don't agree...
Quoted 3 lines
You're now using the hook in two distinct ways:
the primary use of that the hook is to detect that userspace is about to resume processing (viaTIF_NOTIFY_RESUME)
Yes,
Quoted 3 lines
and then you're abusing the fact
that the hook is also invoked viado_exit()to perform a clean up because we've got to get rid of it somehow under that circumstance.
Yes, and please note that this cleanup is only needed if irq thread crashes.
Quoted 2 lines
This only works for you because you're operating in a kernel thread which doesn't have a userspace (and so will never see
TIF_NOTIFY_RESUME).
Yes,
Quoted 2 lines
However,
if someone tries that in an ordinary thread,
Sure, nobody should do this with the ordinary thread. At least exactly this.
Quoted 3 lines
it is liable to malfunction as
the record could be executed and deleted at some unpredictable point in the future.
"In the future" is not possible (and this doesn't depend on PF_KTHREAD). irq_thread() does task_work_cancel() before return. And until it returns it returns the work can't be executed unless this task exits in between.
Oleg.
PATCH v4 3/3 cred: change keyctl_session_to_parent() to use task_work_add()
In reply to Oleg NesterovChange 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.
--- include/linux/key.h | 6 +-- security/keys/internal.h | 2 + security/keys/keyctl.c | 76 ++++++++++++++++++++---------------------- security/keys/process_keys.c | 25 ++++---------- 4 files changed, 47 insertions(+), 62 deletions(-)
include/linux/key.h +2 -4
@@ -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
@@ -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 0x04extern 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 +36 -40
136 lines not on this page · raw message
security/keys/process_keys.c +7 -18
@@ -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); }
Signature · 1 line
1.5.5.1
David Howells
maintainerRe: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
security/keys/keyctl.c long keyctl_get_security(key_serial_t keyid, · line 1431
Quoted 3 lines · Oleg Nesterov
security/keys/keyctl.c +2
-1-int ret; +struct cred *cred; +int err;
There's no need to change ret to be err. It produces slightly less churn if you don't.
security/keys/keyctl.c long keyctl_get_security(key_serial_t keyid, · line 1440
Quoted 4 lines
security/keys/keyctl.c +4
-0+ err = -ENOMEM ; + newwork =kmalloc (sizeof (struct task_work ),GFP_KERNEL ); +if (!newwork) + goto error_keyring;
This passes unnecessarily through kfree(newwork). Please add a new goto label and go to that.
Quoted 4 lines
-/*
- * Replace a process's session keyring on behalf of one of its children when
- * the target process is about to resume userspace execution.
- */
Please don't delete the banner comment saying what the function does.
security/keys/process_keys.c error: · line 831
Quoted 2 lines
security/keys/process_keys.c +1
-1-void key_replace_session_keyring (void ) +void key_change_session_keyring (struct task_work *twork)
There's no particular need to change the name of the function.
David
Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 9 lines · David Howells
security/keys/keyctl.c long keyctl_get_security(key_serial_t keyid, · line 1431
Quoted 3 lines · Oleg Nesterov
security/keys/keyctl.c +2
-1-int ret; +struct cred *cred; +int err;There's no need to change ret to be err. It produces slightly less churn if you don't.
OK, I'll change it back.
Quoted 7 lines
security/keys/keyctl.c long keyctl_get_security(key_serial_t keyid, · line 1440
Quoted 5 lines
security/keys/keyctl.c +4
-0+ err = -ENOMEM ; + newwork =kmalloc (sizeof (struct task_work ),GFP_KERNEL ); +if (!newwork) + goto error_keyring;This passes unnecessarily through kfree(newwork).
but harmless, newwork == NULL in this case,
Quoted 2 lines
Please add a new goto label
and go to that.
OK, probably this is more clear, will do.
Quoted 6 lines
Quoted 4 lines
-/*
- * Replace a process's session keyring on behalf of one of its children when
- * the target process is about to resume userspace execution.
- */Please don't delete the banner comment saying what the function does.
OOPS, sorry, this was unintentional.
Quoted 4 lines
security/keys/process_keys.c error: · line 831
Quoted 2 lines
security/keys/process_keys.c +1
-1-void key_replace_session_keyring (void ) +void key_change_session_keyring (struct task_work *twork)There's no particular need to change the name of the function.
There is.
Until we cleanup arch/* do_notify_resume() still does
if (current->replacement_session_keyring) key_replace_session_keyring();
Oleg.
Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 8 lines · Oleg Nesterov
Quoted 5 lines · David Howells
Review
Quoted 3 lines
(quoted diff) +1
-1-void key_replace_session_keyring(void) +void key_change_session_keyring(struct task_work *twork)There's no particular need to change the name of the function.
Quoted 6 lines
There is.
Until we cleanup
arch/*do_notify_resume()still doesif (current->replacement_session_keyring) key_replace_session_keyring();
So the rename is still here.
Anything else? Please see the updated patch below.
Oleg.
Quoted 1 line
From
2ce2ff8e8715d0d25cc0228fa58f5721caebb780Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <[email protected]>
Date: Thu, 12 Apr 2012 02:30:44 +0200
Subject: [PATCH] cred: change keyctl_session_to_parent() to use task_work_add()
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.
--- include/linux/key.h | 6 +-- security/keys/internal.h | 2 + security/keys/keyctl.c | 73 ++++++++++++++++++++---------------------- security/keys/process_keys.c | 21 ++++-------- 4 files changed, 46 insertions(+), 56 deletions(-)
include/linux/key.h +2 -4
@@ -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
@@ -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 0x04extern 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 -14
@@ -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; +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 +867,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); }
Signature · 1 line
1.5.5.1
David Howells
maintainerRe: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
include/linux/key.h typedef uint32_t key_perm_t; · line 37
Quoted 2 lines · Oleg Nesterov
include/linux/key.h +2
-0+#define key_replace_session_keyring () do { } while (0) +
Ummm... Why? You've made the function no longer exist and so this shouldn't be there anymore. You shouldn't just leave the call sites in place.
David
Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 8 lines · David Howells
include/linux/key.h typedef uint32_t key_perm_t; · line 37
Quoted 2 lines · Oleg Nesterov
include/linux/key.h +2
-0+#define key_replace_session_keyring () do { } while (0) +Ummm... Why? You've made the function no longer exist and so this shouldn't be there anymore. You shouldn't just leave the call sites in place.
Sure!
I'll send the (trivial) patches to cleanup the code in arch/ (24 callers), plus we should kill the no longer used task ->replacement_session_keyring and update copy_creds/exit_creds.
But I certainly do not want to put these changes in 3/3 or in this series. I'll send the patches to Andrew once (I hope ;) he takes 1-3.
Oleg.
Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
forgot to mention...
Quoted 10 lines · Oleg Nesterov
Quoted 8 lines · David Howells
include/linux/key.h typedef uint32_t key_perm_t; · line 37
Quoted 2 lines · Oleg Nesterov
include/linux/key.h +2
-0+#define key_replace_session_keyring () do { } while (0) +Ummm... Why? You've made the function no longer exist and so this shouldn't be there anymore. You shouldn't just leave the call sites in place.
Quoted 5 lines
Sure!
I'll send the (trivial) patches to cleanup the code in
arch/(24 callers), plus we should kill the no longer used task ->replacement_session_keyring and update copy_creds/exit_creds.
... and we should kill tracehook_notify_resume()
Quoted 2 lines
But I certainly do not want to put these changes in 3/3 or in this series. I'll send the patches to Andrew once (I hope ;) he takes 1-3.
Yes.
Oleg.
David Howells
maintainerRe: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
In reply to Oleg NesterovQuoted 1 line · Oleg Nesterov
But I certainly do not want to put these changes in 3/3 or in this series.
Why not? It seems the correct place for them.
David
Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 6 lines · David Howells
Quoted 1 line · Oleg Nesterov
But I certainly do not want to put these changes in 3/3 or in this series.
Why not? It seems the correct place for them.
I wanted to discuss the functional changes first, then send the trivial cleanups. And the changes in arch/ are always painful. And, "remove tracehook_notify_resume" is a bit off-topic, but I do not want to change the same code in arch/ twice.
Oleg.
Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 5 lines · Oleg Nesterov
Quoted 3 lines · David Howells
Oleg Nesterov <[email protected]> wrote:
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
But I certainly do not want to put these changes in 3/3 or in this series.
Why not? It seems the correct place for them.
I wanted to discuss the functional changes first, then send the trivial cleanups. And the changes in
arch/are always painful. And, "removetracehook_notify_resume" is a bit off-topic, but I do not want to change the same code inarch/twice.
That said, if you do not agree with 3/3 without "remove the call sites", I'll send v6 which doesn't rename key_replace_session_keyring() but updates the old callers:
arch/alpha/kernel/signal.c | 2 - arch/arm/kernel/signal.c | 2 - arch/avr32/kernel/signal.c | 2 - arch/blackfin/kernel/signal.c | 2 - arch/c6x/kernel/signal.c | 2 - arch/cris/kernel/ptrace.c | 2 - arch/frv/kernel/signal.c | 2 - arch/h8300/kernel/signal.c | 2 - arch/hexagon/kernel/signal.c | 3 +- arch/ia64/kernel/process.c | 2 - arch/m32r/kernel/signal.c | 2 - arch/mips/kernel/signal.c | 2 - arch/mn10300/kernel/signal.c | 2 - arch/openrisc/kernel/signal.c | 2 - arch/parisc/kernel/signal.c | 2 - arch/powerpc/kernel/signal.c | 2 - arch/s390/kernel/signal.c | 2 - arch/sh/kernel/signal_32.c | 2 - arch/sh/kernel/signal_64.c | 2 - arch/sparc/kernel/signal_32.c | 2 - arch/sparc/kernel/signal_64.c | 2 - arch/tile/kernel/process.c | 2 - arch/unicore32/kernel/signal.c | 2 - arch/x86/kernel/signal.c | 2 - include/linux/key.h | 4 -- include/linux/sched.h | 2 - kernel/cred.c | 9 ----- security/keys/internal.h | 2 + security/keys/keyctl.c | 73 +++++++++++++++++++--------------------- security/keys/process_keys.c | 20 ++++------- 30 files changed, 45 insertions(+), 114 deletions(-)
I am not going to argue, but honestly I don't think it is a good idea to mix the functional changes in keyctl.c and the simple (but obviously untested) cleanups.
Oleg.
David Howells
maintainerRe: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 3 lines · Oleg Nesterov
I am not going to argue, but honestly I don't think it is a good idea to mix the functional changes in
keyctl.cand the simple (but obviously untested) cleanups.
I meant add the cleanup as patch 4. That way they are still separate. I'll let you change the name of the function to make it easier;-)
David
Re: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 9 lines · David Howells
Quoted 3 lines · Oleg Nesterov
I am not going to argue, but honestly I don't think it is a good idea to mix the functional changes in
keyctl.cand the simple (but obviously untested) cleanups.I meant add the cleanup as patch 4. That way they are still separate. I'll let you change the name of the function to make it easier;-)
OK.
I'd like to avoid the unnecessary resends, lets discuss this before I send the patch(es).
At least I'd like to split this into 2 patches:
4/3: only removes the dead code from arch/*
5/3: updates copy_creds/exit_creds and kills
->replacement_session_keyring, plus renames key_change_ back to key_replace_
Do you agree?
Oleg.
David Howells
maintainerRe: [PATCH v4 3/3] cred: change keyctl_session_to_parent() to use task_work_add()
Quoted 18 lines · Oleg Nesterov
Quoted 9 lines · David Howells
Quoted 3 lines · Oleg Nesterov
I am not going to argue, but honestly I don't think it is a good idea to mix the functional changes in
keyctl.cand the simple (but obviously untested) cleanups.I meant add the cleanup as patch 4. That way they are still separate. I'll let you change the name of the function to make it easier;-)
OK.
I'd like to avoid the unnecessary resends, lets discuss this before I send the patch(es).
At least I'd like to split this into 2 patches:
Quoted 7 lines
4/3: only removes the dead code from
arch/*5/3: updates copy_creds/exit_creds and kills
->replacement_session_keyring, plus renameskey_change_back tokey_replace_Do you agree?
Well, don't bother renaming the function back, but apart from that, okay.
David
Quoted 10 lines · Oleg Nesterov
Changes based on review from Andrew:
- s/task_work_queue/task_work_add/
- don't create a secret dependency upon
task_work.hincludingsched.hintracehook.handkeys/internal.h- add more includes into task_work.[ch] to not rely on "
sched.hincludes everything"
I thought based on Andrew's comments that you were going to remove the extra code to do FIFO for no obvious reason. Apparently nothing actually wanted/needed it, so why do it?
Linus
Quoted 7 lines · Linus Torvalds
Quoted 5 lines · Oleg Nesterov
Changes based on review from Andrew:
- s/task_work_queue/task_work_add/
Quoted 8 lines
Quoted 5 lines
- don't create a secret dependency upon
task_work.hincludingsched.hintracehook.handkeys/internal.h- add more includes into task_work.[ch] to not rely on "
sched.hincludes everything"I thought based on Andrew's comments that you were going to remove the extra code to do FIFO for no obvious reason.
No, I tried to defense fifo,
Quoted 2 lines
Apparently nothing
actually wanted/needed it,
True, currently nothing. And most probably never will. task_work_add() should only be used for the "unlikely" events.
Quoted 1 line
so why do it?
I can remove it.
But, unless you strongly object, personally I'd prefer to keep fifo.
Once again. Imho of course, but fifo is simply "obviously better" from the common sense pov when it comes to submit-the-work-for- execution.
For example. keyctl_session_to_parent() does _cancel only to protect from exploits doing keyctl(KEYCTL_SESSION_TO_PARENT) in an endless loop. It could simply do task_work_add(), but in this case we need fifo for correctness.
But, again, again, I am not going to argue too much.
Oleg.
Peter Zijlstra
maintainerQuoted 9 lines · Oleg Nesterov
Hello.
Changes based on review from Andrew:
- s/task_work_queue/task_work_add/
- don't create a secret dependency upon
task_work.hincludingsched.hintracehook.handkeys/internal.h
Quoted 7 lines
- add more includes into task_work.[ch] to not rely on "
sched.hincludes everything"The genirq patch comes as 2/3, it was already acked by Thomas.
3/3 still needs the ack from David, although his last review was positive and I hope I fully addressed his comments.
Can you use this to get rid of TIF_UPROBE as well?
Quoted 2 lines · Peter Zijlstra
Can you use this to get rid of
TIF_UPROBEas well?
Yes, I thought about uprobes. May be it can use task_work, but it is not easy to avoid TIF_UPROBE. The problem is the first UTASK_BP_HIT when current->utask == NULL, and ->utask is the only "right" place to hold task_work. We will see.
Oleg.