sched/numa: fix unsafe get_task_struct() in task_numa_assign()
Subsystems (3)
PATCH v4 1/1 sched/numa: fix unsafe get_task_struct() in task_numa_assign()
Unlocked access to dst_rq->curr in task_numa_compare() is racy. If curr task is exiting this may be a reason of use-after-free:
task_numa_compare() do_exit()
... current->flags |= PF_EXITING;
... release_task()
... ~~delayed_put_task_struct()~~
... schedule()
rcu_read_lock() ...
cur = ACCESS_ONCE(dst_rq->curr) ...
... rq->curr = next;
... context_switch()
... finish_task_switch()
... put_task_struct()
... __put_task_struct()
... free_task_struct()
task_numa_assign() ...
get_task_struct() ...As noted by Oleg:
<<The lockless get_task_struct(tsk) is only safe if tsk == current
and didn't pass exit_notify(), or if this tsk was found on a rcu protected list (say, for_each_process() or find_task_by_vpid()). IOW, it is only safe if release_task() was not called before we take rcu_read_lock(), in this case we can rely on the fact that delayed_put_pid() can not drop the (potentially) last reference until rcu_read_unlock().
And as Kirill pointed out task_numa_compare()->task_numa_assign() path does get_task_struct(dst_rq->curr) and this is not safe. The task_struct itself can't go away, but rcu_read_lock() can't save us from the final put_task_struct() in finish_task_switch(); this reference goes away without rcu gp>>
The patch provides simple check of PF_EXITING flag. If it's not set, this guarantees that call_rcu() of delayed_put_task_struct() callback hasn't happened yet, so we can safely do get_task_struct() in task_numa_assign().
Locked dst_rq->lock protects from concurrency with the last schedule(). Reusing or unmapping of cur's memory may happen without it.
--- kernel/sched/fair.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
kernel/sched/fair.c +12 -2
@@ -1164,9 +1164,19 @@ static void task_numa_compare(struct task_numa_env *env,long moveimp = imp;rcu_read_lock (); - cur =ACCESS_ONCE (dst_rq->curr); -if (cur->pid ==0 )/* idle */ + +raw_spin_lock_irq (&dst_rq->lock); + cur = dst_rq->curr; +/* +* No need to move the exiting task, and this ensures that ->curr +* wasn't reaped and thus get_task_struct() in task_numa_assign() +* is safe under RCU read lock. +* Note that rcu_read_lock() itself can't protect from the final +* put_task_struct() after the last schedule(). +*/ +if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) cur =NULL ; +raw_spin_unlock_irq (&dst_rq->lock);/* * "imp" is the fault differential for the source task between the
introduce task_rcu_dereference?
Quoted 3 lines · Kirill Tkhai
Unlocked access to
dst_rq->currintask_numa_compare()is racy. If curr task is exiting this may be a reason of use-after-free:
Thanks.
And as you pointed out, there are other examples of unlocked foreign_rq->curr usage.
So, Kirill, Peter, do you think that the patch below can help? Can we change task_numa_group() and ->select_task_rq() to do nothing if rq_curr_rcu_safe() returns NULL? It seems we can...
task_numa_compare() can use it too, we can make another patch on top of this one.
- Obviously just for the early review. Lacks the changelog and the comments (at least).
- Once again, I won't insist on probe_slab_address(). We can add SDBR and change task_rcu_dereference() to simply read ->sighand.
- Also, I won't argue if you think that we do not need a generic helper. In this case we can move this logic into rq_curr_rcu_safe() and it will be a bit simpler.
- OTOH, I am not sure we need rq_curr_rcu_safe(). The callers can just use task_rcu_dereference() and check IS_ERR_OR_NULL, I guess retry doesn't buy too much in this case.
Or do you think we need something else?
Oleg.
include/linux/sched.h +1 -0
@@ -2300,6 +2300,7 @@ extern void block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask);extern void unblock_all_signals (void );extern void release_task (struct task_struct * p); +extern struct task_struct *task_rcu_dereference (struct task_struct **ptask);extern int send_sig_info (int ,struct siginfo *,struct task_struct *);extern int force_sigsegv (int ,struct task_struct *);extern int force_sig_info (int ,struct siginfo *,struct task_struct *);
kernel/exit.c +31 -0
@@ -213,6 +213,37 @@ repeat: goto repeat; } +struct task_struct *task_rcu_dereference (struct task_struct **ptask) +{ +struct task_struct *task; +struct sighand_struct *sighand; + + task =rcu_dereference (*ptask); +if (!task) +return NULL ; + +/* If it fails the check below must fail too */ +probe_slab_address (&task->sighand, sighand); +/* +* Pairs with atomic_dec_and_test() in put_task_struct(task). +* If we have read the freed/reused memory, we must see that +* the pointer was updated. The caller might want to retry in +* this case. +*/ +smp_rmb (); +if (unlikely (task !=ACCESS_ONCE (*ptask))) +return ERR_PTR (-EAGAIN ); + +/* +* release_task(task) was already called; potentially before +* the caller took rcu_read_lock() and in this case it can be +* freed before rcu_read_unlock(). +*/ +if (!sighand) +return ERR_PTR (-EINVAL ); +return task; +} +/* * This checks not only the pgrp, but falls back on the pid if no * satisfactory pgrp is found. I dunno - gdb doesn't work correctly
kernel/sched/sched.h +12 -0
@@ -655,6 +655,18 @@ DECLARE_PER_CPU(struct rq, runqueues);#define cpu_curr (cpu) (cpu_rq(cpu)->curr)#define raw_rq () (&__raw_get_cpu_var(runqueues)) +static inline struct task_struct *rq_curr_rcu_safe (struct rq *rq) +{ +for (;;) { +struct task_struct *curr =task_rcu_dereference (&rq->curr); +/* NULL is not possible */ +if (likely (!IS_ERR (curr))) +return curr; +if (PTR_ERR (curr) != -EAGAIN ) +return NULL ; + } +} +static inline u64 rq_clock (struct rq *rq) {return rq->clock;
Re: introduce task_rcu_dereference?
Damn.
Quoted 11 lines · Oleg Nesterov
kernel/exit.c +10
-0+struct task_struct *task_rcu_dereference (struct task_struct **ptask) +{ +struct task_struct *task; +struct sighand_struct *sighand; + + task =rcu_dereference (*ptask); +if (!task) +return NULL ; + +/* If it fails the check below must fail too */
kernel/exit.c repeat: · line 235
Quoted 10 lines
kernel/exit.c +10
-0+probe_slab_address (&task->sighand, sighand); +/* +* Pairs with atomic_dec_and_test() in put_task_struct(task). +* If we have read the freed/reused memory, we must see that +* the pointer was updated. The caller might want to retry in +* this case. +*/ +smp_rmb (); +if (unlikely (task !=ACCESS_ONCE (*ptask))) +return ERR_PTR (-EAGAIN );
This is not exactly right. task == *ptask can be false positive.
It can be freed, then resused (so that sighand != NULL can be false positive), then freed again, and then reused again as task_struct.
This is not that bad, we still can safely use this task_struct, but the comment should be updated. Plus -EINVAL below can be wrong in this case although this minor.
Yeees, SLAB_DESTTROY_BY_RCU closes this race. Not sure why I'd like to avoid it, but I do ;)
Oleg.
Re: introduce task_rcu_dereference?
Quoted 2 lines · Oleg Nesterov
Damn.
Yes.
Quoted 25 lines · Oleg Nesterov
Quoted 11 lines
kernel/exit.c +10
-0+struct task_struct *task_rcu_dereference (struct task_struct **ptask) +{ +struct task_struct *task; +struct sighand_struct *sighand; + + task =rcu_dereference (*ptask); +if (!task) +return NULL ; + +/* If it fails the check below must fail too */ kernel/exit.c repeat: · line 235
Quoted 10 lines
kernel/exit.c +10
-0+probe_slab_address (&task->sighand, sighand); +/* +* Pairs with atomic_dec_and_test() in put_task_struct(task). +* If we have read the freed/reused memory, we must see that +* the pointer was updated. The caller might want to retry in +* this case. +*/ +smp_rmb (); +if (unlikely (task !=ACCESS_ONCE (*ptask))) +return ERR_PTR (-EAGAIN );This is not exactly right. task == *ptask can be false positive.
Quoted 6 lines
It can be freed, then resused (so that sighand != NULL can be false positive), then freed again, and then reused again as
task_struct.This is not that bad, we still can safely use this
task_struct, but the comment should be updated. Plus-EINVALbelow can be wrong in this case although this minor.
Yes.
Quoted 2 lines
Yeees,
SLAB_DESTTROY_BY_RCUcloses this race. Not sure why I'd like to avoid it, but I do ;)
Argh. I only meant that SLAB_DESTTROY_BY_RCU can make the comments simpler. "closes this race" applies too "check below must fail too" too. Sorry if I confused you.
"task == *ptask can be false positive" is true with or without SLAB_DESTTROY_BY_RCU, and this needs a good comment. Yes, it can't be reused twice, but still we can't 100% trust the "sighand != NULL" check.
So let me repeat, SDBR can only turn probe_slab_address() into a plain load.
But I can't think properly today, will try to recheck tomorrow and send v2.
Oleg.
Re: introduce task_rcu_dereference?
In reply to Oleg NesterovВ Ср, 22/10/2014 в 23:30 +0200, Oleg Nesterov пишет:
Quoted 104 lines · Kirill Tkhai
Quoted 3 lines
Unlocked access to
dst_rq->currintask_numa_compare()is racy. If curr task is exiting this may be a reason of use-after-free:Thanks.
And as you pointed out, there are other examples of unlocked
foreign_rq->currusage.So, Kirill, Peter, do you think that the patch below can help? Can we change
task_numa_group()and ->select_task_rq() to do nothing ifrq_curr_rcu_safe()returns NULL? It seems we can...
task_numa_compare()can use it too, we can make another patch on top of this one.- Obviously just for the early review. Lacks the changelog and the comments (at least).
- Once again, I won't insist on
probe_slab_address(). We can add SDBR and changetask_rcu_dereference()to simply read ->sighand.- Also, I won't argue if you think that we do not need a generic helper. In this case we can move this logic into
rq_curr_rcu_safe()and it will be a bit simpler.- OTOH, I am not sure we need
rq_curr_rcu_safe(). The callers can just usetask_rcu_dereference()and checkIS_ERR_OR_NULL, I guess retry doesn't buy too much in this case.Or do you think we need something else?
Oleg.
include/linux/sched.h +1
-0@@ -2300,6 +2300,7 @@ extern void block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask);extern void unblock_all_signals (void );extern void release_task (struct task_struct * p); +extern struct task_struct *task_rcu_dereference (struct task_struct **ptask);extern int send_sig_info (int ,struct siginfo *,struct task_struct *);extern int force_sigsegv (int ,struct task_struct *);extern int force_sig_info (int ,struct siginfo *,struct task_struct *);kernel/exit.c +31
-0@@ -213,6 +213,37 @@ repeat: goto repeat; } +struct task_struct *task_rcu_dereference (struct task_struct **ptask) +{ +struct task_struct *task; +struct sighand_struct *sighand; + + task =rcu_dereference (*ptask); +if (!task) +return NULL ; + +/* If it fails the check below must fail too */ +probe_slab_address (&task->sighand, sighand); +/* +* Pairs with atomic_dec_and_test() in put_task_struct(task). +* If we have read the freed/reused memory, we must see that +* the pointer was updated. The caller might want to retry in +* this case. +*/ +smp_rmb (); +if (unlikely (task !=ACCESS_ONCE (*ptask))) +return ERR_PTR (-EAGAIN ); + +/* +* release_task(task) was already called; potentially before +* the caller took rcu_read_lock() and in this case it can be +* freed before rcu_read_unlock(). +*/ +if (!sighand) +return ERR_PTR (-EINVAL ); +return task; +} +/* * This checks not only the pgrp, but falls back on the pid if no * satisfactory pgrp is found. I dunno - gdb doesn't work correctly kernel/sched/sched.h +5
-0@@ -655,6 +655,18 @@ DECLARE_PER_CPU(struct rq, runqueues);#define cpu_curr (cpu) (cpu_rq(cpu)->curr)#define raw_rq () (&__raw_get_cpu_var(runqueues)) +static inline struct task_struct *rq_curr_rcu_safe (struct rq *rq) +{ +for (;;) { +struct task_struct *curr =task_rcu_dereference (&rq->curr); +/* NULL is not possible */
kernel/sched/sched.h DECLARE_PER_CPU(struct rq, runqueues); · line 672
Quoted 10 lines
kernel/sched/sched.h +7
-0+if (likely (!IS_ERR (curr))) +return curr; +if (PTR_ERR (curr) != -EAGAIN ) +return NULL ; + } +} +static inline u64 rq_clock (struct rq *rq) {return rq->clock;
I'm agree generic helper is better. But probe_slab_address() has a sence if we know that SDBR is worse in our subject area. Less of code is easier to support :) probe_slab_address() it's not a trivial logic. Also, if we use mm primitives this increases kernel modularity.
Kirill
Re: introduce task_rcu_dereference?
Quoted 3 lines · Kirill Tkhai
I'm agree generic helper is better. But
probe_slab_address()has a sence if we know that SDBR is worse in our subject area.
And I still think it is worse.
Quoted 2 lines
Less of code is
easier to support :)
Sure, but ignoring the comments, SDBR needs the same code in task_rcu_dereference() ? Except, of course
- probe_slab_address(&task->sighand, sighand); + sighand = task->sighand;
or how do you think we can simplify it?
Quoted 1 line
probe_slab_address()it's not a trivial logic.
But it already has a user. And probably it can have more.
To me the usage of SDBR is not trivial (and confusing) in this case. Once again, ignoring the CONFIG_DEBUG_PAGEALLOC problems it does not help at all.
With or without SDBR rq->curr can be reused and we need to avoid this race. The fact that with SDBR it can be reused only as another instance of task_struct is absolutely immaterial imo.
Not to mention that SDBR still adds some overhead while probe_slab() is free unless CONFIG_DEBUG_PAGEALLOC, but this option adds a large slowdown anyway.
But again, I can't really work today, perhaps I missed something. Perhaps you can show a better code which relies on SDBR?
Oleg.
Re: introduce task_rcu_dereference?
В Чт, 23/10/2014 в 20:18 +0200, Oleg Nesterov пишет:
Quoted 10 lines · Kirill Tkhai
Quoted 3 lines
I'm agree generic helper is better. But
probe_slab_address()has a sence if we know that SDBR is worse in our subject area.And I still think it is worse.
Quoted 2 lines
Less of code is
easier to support :)
Quoted 7 lines
Sure, but ignoring the comments, SDBR needs the same code in
task_rcu_dereference()? Except, of course- probe_slab_address(&task->sighand, sighand); + sighand = task->sighand;or how do you think we can simplify it?
Ok, really, not big simplification there. Your variant is good.
Quoted 12 lines
Quoted 1 line
probe_slab_address()it's not a trivial logic.But it already has a user. And probably it can have more.
To me the usage of SDBR is not trivial (and confusing) in this case. Once again, ignoring the
CONFIG_DEBUG_PAGEALLOCproblems it does not help at all.With or without SDBR
rq->currcan be reused and we need to avoid this race. The fact that with SDBR it can be reused only as another instance oftask_structis absolutely immaterial imo.
Quoted 7 lines
Not to mention that SDBR still adds some overhead while
probe_slab()is free unlessCONFIG_DEBUG_PAGEALLOC, but this option adds a large slowdown anyway.But again, I can't really work today, perhaps I missed something. Perhaps you can show a better code which relies on SDBR?
No, it would be the same except probe_slab_address(). So, let's stay on probe_slab_address() variant and fix the bug this way.
Kirill
cover introduce task_rcu_dereference()
In reply to Kirill TkhaiPeter, let me repeat once again, if you still prefer to avoid probe_slab_address() and use SLAB_DESTROY_BY_RCU I won't argue.
I do not like SLAB_DESTROY_BY_RCU in this particular case. With or without SDBR rq->curr can be reused and we need to avoid this race. The fact that with SDBR it can be reused only as another instance of task_struct is absolutely immaterial imo.
Not to mention that SDBR still adds some overhead while probe_slab() is free unless CONFIG_DEBUG_PAGEALLOC, but this option adds a large slowdown anyway.
However, my arguments against SDBR are not strictly technical, and I think that this falls into "maintainer is always right" category.
So please tell me if you prefer v2 with SDBR. In this case 2/3 is not needed, and 3/3 can simply read ->sighand. Otherwise the code (and even the comments) will be the same.
Compared to the draft patch I sent before
- update the comments
- do not use ERR_PTR(), just return the task or NULL, so kernel/sched/ doesn't need another helper.
This means that task_rcu_dereference() does retry itself. We can add __task_rcu_dereference() if we have another which do not need/want to retry.
Oleg.
include/linux/sched.h | 1 + include/linux/uaccess.h | 30 +++++++++++++++------------- kernel/exit.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++ mm/slub.c | 6 +---- 4 files changed, 67 insertions(+), 19 deletions(-)
PATCH 1/3 probe_kernel_address() can use __probe_kernel_read()
probe_kernel_address() can just do __probe_kernel_read(sizeof(retval)), no need to open-code its implementation.
--- include/linux/uaccess.h | 17 ++--------------- 1 files changed, 2 insertions(+), 15 deletions(-)
include/linux/uaccess.h +2 -15
@@ -66,22 +66,9 @@ static inline unsigned long __copy_from_user_nocache(void *to,* already holds mmap_sem, or other locks which nest inside mmap_sem. * This must be a macro because __get_user() needs to know the types of the * args. -* -* We don't include enough header files to be able to do the set_fs(). We -* require that the probe_kernel_address() caller will do that. */ -#define probe_kernel_address (addr, retval) \ - ({ \ - long ret; \ - mm_segment_t old_fs = get_fs(); \ - \ - set_fs(KERNEL_DS); \ - pagefault_disable(); \ - ret = __copy_from_user_inatomic(&(retval), (__force typeof(retval) __user *)(addr), sizeof(retval)); \ - pagefault_enable(); \ - set_fs(old_fs); \ - ret; \ - }) +#define probe_kernel_address (addr, retval) \ + __probe_kernel_read(&(retval), (__force void *)(addr), sizeof(retval))/* * probe_kernel_read(): safely attempt to read from a location
Signature · 1 line
1.5.5.1
PATCH 2/3 introduce probe_slab_address()
In reply to Oleg NesterovExtract the ifdef(CONFIG_DEBUG_PAGEALLOC) code from get_freepointer_safe() into the new generic helper, probe_slab_address(). The next patch will add another user.
--- include/linux/uaccess.h | 15 +++++++++++++++ mm/slub.c | 6 +----- 2 files changed, 16 insertions(+), 5 deletions(-)
include/linux/uaccess.h +15 -0
@@ -71,6 +71,21 @@ static inline unsigned long __copy_from_user_nocache(void *to,__probe_kernel_read (&(retval), (__force void *)(addr),sizeof (retval))/* +* Same as probe_kernel_address(), but @addr must be the valid pointer +* to a slab object, potentially freed/reused/unmapped. + */ +#ifdef CONFIG_DEBUG_PAGEALLOC +#define probe_slab_address (addr, retval) \ + probe_kernel_address(addr, retval) +#else +#define probe_slab_address (addr, retval) \ + ({ \ + (retval) = *(typeof(retval) *)(addr); \ + 0; \ + }) +#endif + +/* * probe_kernel_read(): safely attempt to read from a location * @dst: pointer to the buffer that shall take the data * @src: address to read from
mm/slub.c +1 -5
@@ -265,11 +265,7 @@ static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) {void *p; -#ifdef CONFIG_DEBUG_PAGEALLOC -probe_kernel_read (&p, (void **)(object + s->offset),sizeof (p)); -#else - p =get_freepointer (s, object); -#endif +probe_slab_address (object + s->offset, p);return p; }
Signature · 1 line
1.5.5.1
Re: [PATCH 2/3] introduce probe_slab_address()
Quoted 3 lines · Oleg Nesterov
Extract the
ifdef(CONFIG_DEBUG_PAGEALLOC)code fromget_freepointer_safe()into the new generic helper,probe_slab_address(). The next patch will add another user.
Re: [PATCH 2/3] introduce probe_slab_address()
In reply to Oleg NesterovВ Пн, 27/10/2014 в 20:54 +0100, Oleg Nesterov пишет:
Quoted 43 lines
Extract the
ifdef(CONFIG_DEBUG_PAGEALLOC)code fromget_freepointer_safe()into the new generic helper,probe_slab_address(). The next patch will add another user.Signed-off-by Oleg Nesterov <[email protected]>--- include/linux/uaccess.h | 15 +++++++++++++++ mm/slub.c | 6 +----- 2 files changed, 16 insertions(+), 5 deletions(-)include/linux/uaccess.h +15
-0@@ -71,6 +71,21 @@ static inline unsigned long __copy_from_user_nocache(void *to,__probe_kernel_read (&(retval), (__force void *)(addr),sizeof (retval))/* +* Same as probe_kernel_address(), but @addr must be the valid pointer +* to a slab object, potentially freed/reused/unmapped. + */ +#ifdef CONFIG_DEBUG_PAGEALLOC +#define probe_slab_address (addr, retval) \ + probe_kernel_address(addr, retval) +#else +#define probe_slab_address (addr, retval) \ + ({ \ + (retval) = *(typeof(retval) *)(addr); \ + 0; \ + }) +#endif + +/* * probe_kernel_read(): safely attempt to read from a location * @dst: pointer to the buffer that shall take the data * @src: address to read from mm/slub.c +0
-0@@ -265,11 +265,7 @@ static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) {void *p;
mm/slub.c static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) · line 271
Quoted 10 lines
mm/slub.c +1
-5-#ifdef CONFIG_DEBUG_PAGEALLOC -probe_kernel_read (&p, (void **)(object + s->offset),sizeof (p)); -#else - p =get_freepointer (s, object); -#endif +probe_slab_address (object + s->offset, p);return p; }
probe_kernel_read() was arch-dependent on tree platforms:
arch/blackfin/mm/maccess.carch/parisc/lib/memcpy.carch/um/kernel/maccess.c
But now we skip these arch-dependent implementations. Is there no a problem?
Kirill
Re: [PATCH 2/3] introduce probe_slab_address()
В Вт, 28/10/2014 в 08:44 +0300, Kirill Tkhai пишет:
Quoted 45 lines
В Пн, 27/10/2014 в 20:54 +0100, Oleg Nesterov пишет:
Quoted 44 lines
Extract the
ifdef(CONFIG_DEBUG_PAGEALLOC)code fromget_freepointer_safe()into the new generic helper,probe_slab_address(). The next patch will add another user.Signed-off-by Oleg Nesterov <[email protected]>--- include/linux/uaccess.h | 15 +++++++++++++++ mm/slub.c | 6 +----- 2 files changed, 16 insertions(+), 5 deletions(-)include/linux/uaccess.h +15
-0@@ -71,6 +71,21 @@ static inline unsigned long __copy_from_user_nocache(void *to,__probe_kernel_read (&(retval), (__force void *)(addr),sizeof (retval))/* +* Same as probe_kernel_address(), but @addr must be the valid pointer +* to a slab object, potentially freed/reused/unmapped. + */ +#ifdef CONFIG_DEBUG_PAGEALLOC +#define probe_slab_address (addr, retval) \ + probe_kernel_address(addr, retval) +#else +#define probe_slab_address (addr, retval) \ + ({ \ + (retval) = *(typeof(retval) *)(addr); \ + 0; \ + }) +#endif + +/* * probe_kernel_read(): safely attempt to read from a location * @dst: pointer to the buffer that shall take the data * @src: address to read from mm/slub.c +0
-0@@ -265,11 +265,7 @@ static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) {void *p;
Quoted 11 lines
mm/slub.c static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) · line 271
Quoted 9 lines
mm/slub.c +1
-5-#ifdef CONFIG_DEBUG_PAGEALLOC -probe_kernel_read (&p, (void **)(object + s->offset),sizeof (p)); -#else - p =get_freepointer (s, object); -#endif +probe_slab_address (object + s->offset, p);return p; }
probe_kernel_read()was arch-dependent on tree platforms:
Of course, I mean get_freepointer_safe() used to use arch-dependent probe_kernel_read() on blackfin, parisc and um.
Quoted 5 lines
arch/blackfin/mm/maccess.carch/parisc/lib/memcpy.carch/um/kernel/maccess.cBut now we skip these arch-dependent implementations. Is there no a problem?
Peter Zijlstra
maintainerRe: [PATCH 2/3] introduce probe_slab_address()
In reply to Kirill TkhaiQuoted 7 lines · Kirill Tkhai
В Пн, 27/10/2014 в 20:54 +0100, Oleg Nesterov пишет:
include/linux/uaccess.h static inline unsigned long __copy_from_user_nocache(void *to, · line 79
Quoted 2 lines
include/linux/uaccess.h +2
-0+#define probe_slab_address (addr, retval) \ + probe_kernel_address(addr, retval)
probe_kernel_read()was arch-dependent on tree platforms:
Quoted 5 lines
arch/blackfin/mm/maccess.carch/parisc/lib/memcpy.carch/um/kernel/maccess.cBut now we skip these arch-dependent implementations. Is there no a problem?
Nope, see the first patch, it makes probe_kernel_address use __probe_kernel_read().
Re: [PATCH 2/3] introduce probe_slab_address()
Quoted 8 lines · Peter Zijlstra
Quoted 7 lines · Kirill Tkhai
В Пн, 27/10/2014 в 20:54 +0100, Oleg Nesterov пишет:
include/linux/uaccess.h static inline unsigned long __copy_from_user_nocache(void *to, · line 79
Quoted 2 lines
include/linux/uaccess.h +2
-0+#define probe_slab_address (addr, retval) \ + probe_kernel_address(addr, retval)
probe_kernel_read()was arch-dependent on tree platforms:
Quoted 9 lines
Quoted 5 lines
arch/blackfin/mm/maccess.carch/parisc/lib/memcpy.carch/um/kernel/maccess.cBut now we skip these arch-dependent implementations. Is there no a problem?
Nope, see the first patch, it makes
probe_kernel_addressuse__probe_kernel_read().
Yes, probe_kernel_read() is in [1/3], but it's not the same as __probe_kernel_read() for blackfin, for example.
It's defined as
long __weak probe_kernel_read(void *dst, const void *src, size_t size)
__attribute__((alias("__probe_kernel_read")));But blackfin's probe_kernel_read() redefines this __weak function, isn't it? Didn't get_freepointer_safe() use to call architecture's probe_kernel_read() before?
I don't see how it is called now...
Re: [PATCH 2/3] introduce probe_slab_address()
Quoted 13 lines · Kirill Tkhai
Quoted 12 lines · Peter Zijlstra
Quoted 11 lines · Kirill Tkhai
В Пн, 27/10/2014 в 20:54 +0100, Oleg Nesterov пишет:
include/linux/uaccess.h static inline unsigned long __copy_from_user_nocache(void *to, · line 79
Quoted 2 lines
include/linux/uaccess.h +2
-0+#define probe_slab_address (addr, retval) \ + probe_kernel_address(addr, retval)
probe_kernel_read()was arch-dependent on tree platforms:
arch/blackfin/mm/maccess.carch/parisc/lib/memcpy.carch/um/kernel/maccess.c
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
But now we skip these arch-dependent implementations. Is there no a problem?
Nope, see the first patch, it makes
probe_kernel_addressuse__probe_kernel_read().Yes,
probe_kernel_read()is in [1/3], but it's not the same as__probe_kernel_read()for blackfin, for example.
Vise versa, I mean __probe_kernel_read() is in [1/3].
Quoted 11 lines
It's defined as
long
__weakprobe_kernel_read(void *dst, const void *src, size_t size)__attribute__((alias("__probe_kernel_read")));But blackfin's
probe_kernel_read()redefines this__weakfunction, isn't it? Didn'tget_freepointer_safe()use to call architecture'sprobe_kernel_read()before?I don't see how it is called now...
Re: [PATCH 2/3] introduce probe_slab_address()
In reply to Kirill TkhaiQuoted 4 lines · Kirill Tkhai
Yes,
probe_kernel_read()is in [1/3], but it's not the same as__probe_kernel_read()for blackfin, for example.
Quoted 8 lines
It's defined as
long
__weakprobe_kernel_read(void *dst, const void *src, size_t size)__attribute__((alias("__probe_kernel_read")));But blackfin's
probe_kernel_read()redefines this__weakfunction, isn't it? Didn'tget_freepointer_safe()use to call architecture'sprobe_kernel_read()before?
I _think_ that __probe_kernel_read(slab_ddr) should be fine.
Yes, an architecture may want to reimplement probe_kernel_read() to allow to safely access the special areas, or special addresses.
But again, in this case we know that this address points to the "normal" kernel memory, __copy_from_user_inatomic() should work fine.
Oleg.
Re: [PATCH 2/3] introduce probe_slab_address()
Quoted 15 lines · Oleg Nesterov
Quoted 4 lines · Kirill Tkhai
Yes,
probe_kernel_read()is in [1/3], but it's not the same as__probe_kernel_read()for blackfin, for example.Quoted 8 lines
It's defined as
long
__weakprobe_kernel_read(void *dst, const void *src, size_t size)__attribute__((alias("__probe_kernel_read")));But blackfin's
probe_kernel_read()redefines this__weakfunction, isn't it? Didn'tget_freepointer_safe()use to call architecture'sprobe_kernel_read()before?
Quoted 7 lines
I _think_ that
__probe_kernel_read(slab_ddr)should be fine.Yes, an architecture may want to reimplement
probe_kernel_read()to allow to safely access the special areas, or special addresses.But again, in this case we know that this address points to the "normal" kernel memory,
__copy_from_user_inatomic()should work fine.
OTOH, perhaps probe_kernel_address() should use probe_kernel_read(), not __probe_kernel_read(). But currently it just calls __copy_inatomic() so 1/3 follows this logic.
Oleg.
Re: [PATCH 2/3] introduce probe_slab_address()
В Вт, 28/10/2014 в 21:12 +0100, Oleg Nesterov пишет:
Quoted 21 lines · Oleg Nesterov
Quoted 20 lines · Kirill Tkhai
Quoted 4 lines
Yes,
probe_kernel_read()is in [1/3], but it's not the same as__probe_kernel_read()for blackfin, for example.Quoted 8 lines
It's defined as
long
__weakprobe_kernel_read(void *dst, const void *src, size_t size)__attribute__((alias("__probe_kernel_read")));But blackfin's
probe_kernel_read()redefines this__weakfunction, isn't it? Didn'tget_freepointer_safe()use to call architecture'sprobe_kernel_read()before?I _think_ that
__probe_kernel_read(slab_ddr)should be fine.Yes, an architecture may want to reimplement
probe_kernel_read()to allow to safely access the special areas, or special addresses.
Quoted 6 lines
Quoted 2 lines
But again, in this case we know that this address points to the "normal" kernel memory,
__copy_from_user_inatomic()should work fine.OTOH, perhaps
probe_kernel_address()should useprobe_kernel_read(), not__probe_kernel_read(). But currently it just calls__copy_inatomic()so 1/3 follows this logic.
Ok, thanks for the explanation, Oleg.
PATCH 3/3 introduce task_rcu_dereference()
In reply to Oleg Nesterovtask_struct is only protected by RCU if it was found on a RCU protected list (say, for_each_process() or find_task_by_vpid()).
And as Kirill pointed out rq->curr isn't protected by RCU, the scheduler drops the (potentially) last reference without RCU gp, this means that we need to fix the code which uses foreign_rq->curr under rcu_read_lock().
Add a new helper which can be used to dereferene rq->curr or any other pointer to task_struct assuming that it should be cleared or updated before the final put_task_struct(). It returns non-NULL only if this task can't go away before rcu_read_unlock().
--- include/linux/sched.h | 1 + kernel/exit.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 0 deletions(-)
include/linux/sched.h +1 -0
@@ -2300,6 +2300,7 @@ extern void block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask);extern void unblock_all_signals (void );extern void release_task (struct task_struct * p); +extern struct task_struct *task_rcu_dereference (struct task_struct **ptask);extern int send_sig_info (int ,struct siginfo *,struct task_struct *);extern int force_sigsegv (int ,struct task_struct *);extern int force_sig_info (int ,struct siginfo *,struct task_struct *);
kernel/exit.c +49 -0
55 lines not on this page · raw message
Signature · 1 line
1.5.5.1
Re: [PATCH 3/3] introduce task_rcu_dereference()
В Пн, 27/10/2014 в 20:54 +0100, Oleg Nesterov пишет:
Quoted 81 lines
task_structis only protected by RCU if it was found on a RCU protected list (say,for_each_process()orfind_task_by_vpid()).And as Kirill pointed out
rq->currisn't protected by RCU, the scheduler drops the (potentially) last reference without RCU gp, this means that we need to fix the code which usesforeign_rq->currunderrcu_read_lock().Add a new helper which can be used to dereferene
rq->curror any other pointer totask_structassuming that it should be cleared or updated before the finalput_task_struct(). It returns non-NULL only if this task can't go away beforercu_read_unlock().Suggested-by Kirill Tkhai <[email protected]>Signed-off-by Oleg Nesterov <[email protected]>--- include/linux/sched.h | 1 + kernel/exit.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 0 deletions(-)include/linux/sched.h +1
-0@@ -2300,6 +2300,7 @@ extern void block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask);extern void unblock_all_signals (void );extern void release_task (struct task_struct * p); +extern struct task_struct *task_rcu_dereference (struct task_struct **ptask);extern int send_sig_info (int ,struct siginfo *,struct task_struct *);extern int force_sigsegv (int ,struct task_struct *);extern int force_sig_info (int ,struct siginfo *,struct task_struct *);kernel/exit.c +42
-0@@ -213,6 +213,55 @@ repeat: goto repeat; } +struct task_struct *task_rcu_dereference (struct task_struct **ptask) +{ +struct task_struct *task; +struct sighand_struct *sighand; + +/* +* We need to verify that release_task() was not called and thus +* delayed_put_task_struct() can't run and drop the last reference +* before rcu_read_unlock(). We check task->sighand != NULL, but +* we can read the already freed and reused memory. +*/ + retry: + task =rcu_dereference (*ptask); +if (!task) +return NULL ; + +probe_slab_address (&task->sighand, sighand); +/* +* Pairs with atomic_dec_and_test(usage) in put_task_struct(task). +* If this task was already freed we can not miss the preceding +* update of this pointer. +*/ +smp_rmb (); +if (unlikely (task !=ACCESS_ONCE (*ptask))) + goto retry; + +/* +* Either this is the same task and we can trust sighand != NULL, or +* its memory was re-instantiated as another instance of task_struct. +* In the latter case the new task can not go away until another rcu +* gp pass, so the only problem is that sighand == NULL can be false +* positive but we can pretend we got this NULL before it was freed. +*/ +if (sighand) +return task; + +/* +* We could even eliminate the false positive mentioned above: +* +* probe_slab_address(&task->sighand, sighand); +* if (sighand) +* goto retry;
kernel/exit.c repeat: · line 267
Quoted 10 lines
kernel/exit.c +7
-0+* +* if sighand != NULL because we read the freed memory we should +* see the new pointer, otherwise we will likely return this task. +*/ +return NULL ; +} +/* * This checks not only the pgrp, but falls back on the pid if no * satisfactory pgrp is found. I dunno - gdb doesn't work correctly
Peter Zijlstra
maintainerRe: [PATCH 3/3] introduce task_rcu_dereference()
In reply to Oleg NesterovSo I keep coming back to this, and every time I look at it my brain explodes.
Review
Quoted 4 lines · Oleg Nesterov
(quoted diff) +4
-0+struct task_struct *task_rcu_dereference(struct task_struct **ptask) +{ + struct task_struct *task; + struct sighand_struct *sighand;
I think that needs: ' = NULL', because if
Review
Quoted 13 lines
(quoted diff) +13
-0+ + /* + * We need to verify that release_task() was not called and thus + * delayed_put_task_struct() can't run and drop the last reference + * before rcu_read_unlock(). We check task->sighand != NULL, but + * we can read the already freed and reused memory. + */ + retry: + task = rcu_dereference(*ptask); + if (!task) + return NULL; + + probe_slab_address(&task->sighand, sighand);
this fails because of DEBUG_PAGE_ALLOC, we might not write to sighand at all, and
Quoted 7 lines
(quoted diff) +7
-0+ /* + * Pairs with atomic_dec_and_test(usage) in put_task_struct(task). + * If this task was already freed we can not miss the preceding + * update of this pointer. + */ + smp_rmb(); + if (unlikely(task != ACCESS_ONCE(*ptask)))
Review
Quoted 10 lines
(quoted diff) +10
-0+ goto retry; + + /* + * Either this is the same task and we can trust sighand != NULL, or + * its memory was re-instantiated as another instance of task_struct. + * In the latter case the new task can not go away until another rcu + * gp pass, so the only problem is that sighand == NULL can be false + * positive but we can pretend we got this NULL before it was freed. + */ + if (sighand)
this will be inspecting random values on the stack.
Quoted 4 lines
(quoted diff) +4
-0+ return task; + + /* + * We could even eliminate the false positive mentioned above:
Review
Quoted 10 lines
(quoted diff) +10
-0+ * + * probe_slab_address(&task->sighand, sighand); + * if (sighand) + * goto retry; + * + * if sighand != NULL because we read the freed memory we should + * see the new pointer, otherwise we will likely return this task. + */ + return NULL; +}
This thing does more than rcu_dereference; because it also guarantees that task->usage > 0 for the entire RCU section you do this under. Because delayed_put_task_struct() will be delayed until at least the matching rcu_read_unlock().
Should we instead create a primitive like try_get_task_struct() which returns true if it got a reference? Because that is typically what people end up doing..
A little like so?
--- include/linux/sched.h | 2 ++ kernel/exit.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ kernel/sched/fair.c | 29 +++++++---------------------- 3 files changed, 55 insertions(+), 22 deletions(-)
include/linux/sched.h +2 -0
@@ -2132,6 +2132,8 @@ static inline void put_task_struct(struct task_struct *t)__put_task_struct (t); } +struct task_struct *try_get_task_struct (struct task_struct **ptask); +#ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN extern void task_cputime (struct task_struct *t,cputime_t *utime,cputime_t *stime);
kernel/exit.c +46 -0
52 lines not on this page · raw message
kernel/sched/fair.c +7 -22
@@ -1374,30 +1374,15 @@ static void task_numa_compare(struct task_numa_env *env,int dist = env->dist;bool assigned = false; -rcu_read_lock (); - -raw_spin_lock_irq (&dst_rq->lock); - cur = dst_rq->curr; -/* -* No need to move the exiting task or idle task. -*/ -if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) - cur =NULL ; -else { -/* -* The task_struct must be protected here to protect the -* p->numa_faults access in the task_weight since the -* numa_faults could already be freed in the following path: -* finish_task_switch() -* --> put_task_struct() -* --> __put_task_struct() -* --> task_numa_free() -*/ -get_task_struct (cur); + cur =try_get_task_struct (&dst_rq->curr); +if (cur) { +if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) { +put_task_struct (cur); + cur =NULL ; + } } -raw_spin_unlock_irq (&dst_rq->lock); - +rcu_read_lock ();/* * Because we have preemption enabled we can get migrated around and * end try selecting ourselves (current == env->p) as a swap candidate.
Re: [PATCH 3/3] introduce task_rcu_dereference()
Quoted 3 lines · Peter Zijlstra
So I keep coming back to this, and every time I look at it my brain explodes.
Heh ;) I forgot about this completely.
Quoted 36 lines · Oleg Nesterov
Review
Quoted 4 lines
(quoted diff) +4
-0+struct task_struct *task_rcu_dereference(struct task_struct **ptask) +{ + struct task_struct *task; + struct sighand_struct *sighand;I think that needs: ' = NULL', because if
kernel/exit.c void release_task(struct task_struct *p)
Quoted 13 lines
kernel/exit.c +13
-0+ +/* +* We need to verify that release_task() was not called and thus +* delayed_put_task_struct() can't run and drop the last reference +* before rcu_read_unlock(). We check task->sighand != NULL, but +* we can read the already freed and reused memory. +*/ + retry: + task =rcu_dereference (*ptask); +if (!task) +return NULL ; + +probe_slab_address (&task->sighand, sighand);this fails because of
DEBUG_PAGE_ALLOC, we might not write to sighand at all, andQuoted 11 lines
kernel/exit.c +11
-0+/* +* Pairs with atomic_dec_and_test(usage) in put_task_struct(task). +* If this task was already freed we can not miss the preceding +* update of this pointer. +*/ +smp_rmb (); +if (unlikely (task !=ACCESS_ONCE (*ptask))) + goto retry; + +/* +* Either this is the same task and we can trust sighand != NULL, or
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ * its memory was re-instantiated as another instance of task_struct. + * In the latter case the new task can not go away until another rcu + * gp pass, so the only problem is that sighand == NULL can be false + * positive but we can pretend we got this NULL before it was freed. + */ + if (sighand)this will be inspecting random values on the stack.
Yes, but thi_s doesn't differ from the case when we inspect the random value if this task_struct was freed, then reallocated as another thing, then freed and reallocated as task_struct again.
Please note the comment about the false positive above, and another one below:
Quoted 5 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ * We could even eliminate the false positive mentioned above: + * + * probe_slab_address(&task->sighand, sighand); + * if (sighand) + * goto retry;
this one.
IOW. We can never know if we have a garbage in "sighand" or the real value, this task_struct can be freed/reallocated when we do probe_slab_address().
And this is fine. We re-check that "task == *ptask" after that. Now we have two different cases:
1. This is actually the same task/task_struct. In this case sighand != NULL tells us it is still alive.
2. This is another task which got the same memory for task_struct. We can't know this of course, and we can not trust sighand != NULL.
In this case we actually return a random value, but this is correct.
If we return NULL - we can pretend that we actually noticed that *ptask was updated when the previous task has exited. Or pretend that probe_slab_address(&sighand) reads NULL.
If we return the new task (because sighand is not NULL for any reason) - this is fine too. This (new) task can't go away before another gp pass.
And please note again the "We could even eliminate the false positive" comment above (hmm, it should probably say false negative). We could re-read task->sighand once again to avoid the falsely NULL.
But this case is very unlikely so I think we do not really care.
And perhaps this idea can find more users...
Quoted 4 lines
This thing does more than
rcu_dereference; because it also guarantees thattask->usage> 0 for the entire RCU section you do this under. Becausedelayed_put_task_struct()will be delayed until at least the matchingrcu_read_unlock().
Yes, yes, exactly.
Quoted 3 lines
Should we instead create a primitive like
try_get_task_struct()which returns true if it got a reference? Because that is typically what people end up doing..
Well, I won't really argue... but personally I'd prefer to leave it to the caller. Note that task_rcu_dereference() doesn't even do rcu_read_lock(). It really acts as "rcu dereference".
I agree, try_get_task_struct(ptask) makes sense too, but it should be implemented as a trivial wrapper on top of task_rcu_dereference().
Oleg.
Peter Zijlstra
maintainerRe: [PATCH 3/3] introduce task_rcu_dereference()
Quoted 22 lines · Oleg Nesterov
IOW. We can never know if we have a garbage in "sighand" or the real value, this
task_structcan be freed/reallocated when we doprobe_slab_address().And this is fine. We re-check that "task == *ptask" after that. Now we have two different cases:
1. This is actually the same task/task_struct. In this case sighand != NULL tells us it is still alive.
2. This is another task which got the same memory for
task_struct. We can't know this of course, and we can not trust sighand != NULL.In this case we actually return a random value, but this is correct.
If we return NULL - we can pretend that we actually noticed that *ptask was updated when the previous task has exited. Or pretend that
probe_slab_address(&sighand)reads NULL.If we return the new task (because sighand is not NULL for any reason) - this is fine too. This (new) task can't go away before another gp pass.
Quoted 6 lines
And please note again the "We could even eliminate the false positive" comment above (hmm, it should probably say false negative). We could re-read
task->sighandonce again to avoid the falsely NULL.But this case is very unlikely so I think we do not really care.
Ah right, lets stick that in.. :-)
OK, something like so then?
--- include/linux/sched.h | 3 ++ kernel/exit.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++ kernel/sched/fair.c | 29 +++++--------------- 3 files changed, 86 insertions(+), 22 deletions(-)
include/linux/sched.h +3 -0
@@ -2134,6 +2134,9 @@ static inline void put_task_struct(struct task_struct *t)__put_task_struct (t); } +struct task_struct *task_rcu_dereference (struct task_struct **ptask); +struct task_struct *try_get_task_struct (struct task_struct **ptask); +#ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN extern void task_cputime (struct task_struct *t,cputime_t *utime,cputime_t *stime);
kernel/exit.c +76 -0
82 lines not on this page · raw message
kernel/sched/fair.c +7 -22
@@ -1374,30 +1374,15 @@ static void task_numa_compare(struct task_numa_env *env,int dist = env->dist;bool assigned = false; -rcu_read_lock (); - -raw_spin_lock_irq (&dst_rq->lock); - cur = dst_rq->curr; -/* -* No need to move the exiting task or idle task. -*/ -if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) - cur =NULL ; -else { -/* -* The task_struct must be protected here to protect the -* p->numa_faults access in the task_weight since the -* numa_faults could already be freed in the following path: -* finish_task_switch() -* --> put_task_struct() -* --> __put_task_struct() -* --> task_numa_free() -*/ -get_task_struct (cur); + cur =try_get_task_struct (&dst_rq->curr); +if (cur) { +if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) { +put_task_struct (cur); + cur =NULL ; + } } -raw_spin_unlock_irq (&dst_rq->lock); - +rcu_read_lock ();/* * Because we have preemption enabled we can get migrated around and * end try selecting ourselves (current == env->p) as a swap candidate.
Re: [PATCH 3/3] introduce task_rcu_dereference()
Quoted 2 lines · Peter Zijlstra
OK, something like so then?
Yes thanks!
Just one note,
Quoted 7 lines
(quoted diff) +7
-0+struct task_struct *task_rcu_dereference(struct task_struct **ptask) +{ + struct sighand_struct *sighand; + struct task_struct *task; + + /* + * We need to verify that release_task() was not called and thus
Review
Quoted 10 lines
(quoted diff) +10
-0+ * delayed_put_task_struct() can't run and drop the last reference + * before rcu_read_unlock(). We check task->sighand != NULL, + * but we can read the already freed and reused memory. + */ +retry: + task = rcu_dereference(*ptask); + if (!task) + return NULL; + + probe_kernel_address(&task->sighand, sighand);
OK. Then I'll re-send the patch which adds the probe_slab_address() helper on top of this change. We do not want __probe_kernel_read() if if CONFIG_DEBUG_PAGEALLOC=n.
Quoted 17 lines
kernel/sched/fair.c +0
-11@@ -1374,30 +1374,15 @@ static void task_numa_compare(struct task_numa_env *env,int dist = env->dist;bool assigned = false; -rcu_read_lock (); - -raw_spin_lock_irq (&dst_rq->lock); - cur = dst_rq->curr; -/* -* No need to move the exiting task or idle task. -*/ -if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) - cur =NULL ; -else { -/*
kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1377
Quoted 10 lines
kernel/sched/fair.c +1
-9-* The task_struct must be protected here to protect the -* p->numa_faults access in the task_weight since the -* numa_faults could already be freed in the following path: -* finish_task_switch() -* --> put_task_struct() -* --> __put_task_struct() -* --> task_numa_free() -*/ -get_task_struct (cur); + cur =try_get_task_struct (&dst_rq->curr);
Do we really want try_get_task_struct() here? How about the change below?
To me it would be more clean to do get_task_struct() in task_numa_assign(), it clearly pairs with put_task_struct(best_task) and task_numa_compare() looks a bit simpler this way, no need to put_task_struct() if we nullify cur.
What do you think? In any case I think the change in sched/fair.c should probably come as a separate patch, but this is up to you.
Oleg.
kernel/sched/fair.c +4 -30
@@ -1254,6 +1254,8 @@ static void task_numa_assign(struct task_numa_env *env, {if (env->best_task)put_task_struct (env->best_task); +if (p) +get_task_struct (p); env->best_task = p; env->best_imp = imp;
@@ -1321,31 +1323,11 @@ static void task_numa_compare(struct task_numa_env *env,long imp = env->p->numa_group ? groupimp : taskimp;long moveimp = imp;int dist = env->dist; -bool assigned = false;rcu_read_lock (); - -raw_spin_lock_irq (&dst_rq->lock); - cur = dst_rq->curr; -/* -* No need to move the exiting task or idle task. -*/ -if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) + cur =task_rcu_dereference (&dst_rq->curr); +if (cur && ((cur->flags &PF_EXITING ) ||is_idle_task (cur))) cur =NULL ; -else { -/* -* The task_struct must be protected here to protect the -* p->numa_faults access in the task_weight since the -* numa_faults could already be freed in the following path: -* finish_task_switch() -* --> put_task_struct() -* --> __put_task_struct() -* --> task_numa_free() -*/ -get_task_struct (cur); - } - -raw_spin_unlock_irq (&dst_rq->lock);/* * Because we have preemption enabled we can get migrated around and
@@ -1428,7 +1410,6 @@ balance:*/ if (!load_too_imbalanced (src_load, dst_load, env)) { imp = moveimp -1 ; -put_task_struct (cur); cur =NULL ; goto assign; }
@@ -1454,16 +1435,9 @@ balance: env->dst_cpu =select_idle_sibling (env->p, env->dst_cpu); assign: - assigned = true;task_numa_assign (env, cur, imp); unlock:rcu_read_unlock (); -/* -* The dst_rq->curr isn't assigned. The protection for task_struct is -* finished. -*/ -if (cur && !assigned) -put_task_struct (cur); } static void task_numa_find_cpu(struct task_numa_env *env,
Peter Zijlstra
maintainerRe: [PATCH 3/3] introduce task_rcu_dereference()
Quoted 9 lines · Oleg Nesterov
Do we really want
try_get_task_struct()here? How about the change below?To me it would be more clean to do
get_task_struct()intask_numa_assign(), it clearly pairs withput_task_struct(best_task)andtask_numa_compare()looks a bit simpler this way, no need toput_task_struct()if we nullify cur.What do you think? In any case I think the change in
sched/fair.cshould probably come as a separate patch, but this is up to you.
You are quite right. I've added your SoB to this patch if you don't mind -- and I've attributed the task_rcu_dereference() thing to you too, as all I did was copy paste different bits of your emails together while trying to get my head around it ;-)
tip-bot for Oleg Nesterov
merge bot[tip:sched/core] sched/fair: Use task_rcu_dereference()
In reply to Oleg Nesterovsched/fair: Use task_rcu_dereference()
Commit-ID: bac7857319bcf7fed329a10bb760053e761115c0 Gitweb: http://git.kernel.org/tip/bac7857319bcf7fed329a10bb760053e761115c0 Author: Oleg Nesterov <[email protected]> AuthorDate: Wed, 18 May 2016 21:57:33 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 3 Jun 2016 09:18:58 +0200
sched/fair: Use task_rcu_dereference()
Simplify task_numa_compare()'s task reference magic by using task_rcu_dereference().
--- kernel/sched/fair.c | 34 ++++------------------------------ 1 file changed, 4 insertions(+), 30 deletions(-)
kernel/sched/fair.c +4 -30
@@ -1305,6 +1305,8 @@ static void task_numa_assign(struct task_numa_env *env, {if (env->best_task)put_task_struct (env->best_task); +if (p) +get_task_struct (p); env->best_task = p; env->best_imp = imp;
@@ -1372,31 +1374,11 @@ static void task_numa_compare(struct task_numa_env *env,long imp = env->p->numa_group ? groupimp : taskimp;long moveimp = imp;int dist = env->dist; -bool assigned = false;rcu_read_lock (); - -raw_spin_lock_irq (&dst_rq->lock); - cur = dst_rq->curr; -/* -* No need to move the exiting task or idle task. -*/ -if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) + cur =task_rcu_dereference (&dst_rq->curr); +if (cur && ((cur->flags &PF_EXITING ) ||is_idle_task (cur))) cur =NULL ; -else { -/* -* The task_struct must be protected here to protect the -* p->numa_faults access in the task_weight since the -* numa_faults could already be freed in the following path: -* finish_task_switch() -* --> put_task_struct() -* --> __put_task_struct() -* --> task_numa_free() -*/ -get_task_struct (cur); - } - -raw_spin_unlock_irq (&dst_rq->lock);/* * Because we have preemption enabled we can get migrated around and
@@ -1479,7 +1461,6 @@ balance:*/ if (!load_too_imbalanced (src_load, dst_load, env)) { imp = moveimp -1 ; -put_task_struct (cur); cur =NULL ; goto assign; }
@@ -1505,16 +1486,9 @@ balance: env->dst_cpu =select_idle_sibling (env->p, env->dst_cpu); assign: - assigned = true;task_numa_assign (env, cur, imp); unlock:rcu_read_unlock (); -/* -* The dst_rq->curr isn't assigned. The protection for task_struct is -* finished. -*/ -if (cur && !assigned) -put_task_struct (cur); } static void task_numa_find_cpu(struct task_numa_env *env,
tip-bot for Oleg Nesterov
merge bot[tip:sched/core] sched/api: Introduce task_rcu_dereference() and try_get_task_struct()
In reply to Peter Zijlstrasched/api: Introduce task_rcu_dereference() and try_get_task_struct()
Commit-ID: 150593bf869393d10a79f6bd3df2585ecc20a9bb Gitweb: http://git.kernel.org/tip/150593bf869393d10a79f6bd3df2585ecc20a9bb Author: Oleg Nesterov <[email protected]> AuthorDate: Wed, 18 May 2016 19:02:18 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Fri, 3 Jun 2016 09:18:57 +0200
sched/api: Introduce task_rcu_dereference() and try_get_task_struct()
Generally task_struct is only protected by RCU if it was found on a RCU protected list (say, for_each_process() or find_task_by_vpid()).
As Kirill pointed out rq->curr isn't protected by RCU, the scheduler drops the (potentially) last reference without RCU gp, this means that we need to fix the code which uses foreign_rq->curr under rcu_read_lock().
Add a new helper which can be used to dereference rq->curr or any other pointer to task_struct assuming that it should be cleared or updated before the final put_task_struct(). It returns non-NULL only if this task can't go away before rcu_read_unlock().
( Also add try_get_task_struct() to make it easier to use this API
correctly. )
--- include/linux/sched.h | 3 ++ kernel/exit.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+)
include/linux/sched.h +3 -0
@@ -2139,6 +2139,9 @@ static inline void put_task_struct(struct task_struct *t)__put_task_struct (t); } +struct task_struct *task_rcu_dereference (struct task_struct **ptask); +struct task_struct *try_get_task_struct (struct task_struct **ptask); +#ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN extern void task_cputime (struct task_struct *t,cputime_t *utime,cputime_t *stime);
kernel/exit.c +76 -0
82 lines not on this page · raw message
tip-bot for Kirill Tkhai
merge bot[tip:sched/core] sched/numa: Fix unsafe get_task_struct() in task_numa_assign()
In reply to Kirill Tkhaisched/numa: Fix unsafe get_task_struct() in task_numa_assign()
Commit-ID: 1effd9f19324efb05fccc7421530e11a52db0278 Gitweb: http://git.kernel.org/tip/1effd9f19324efb05fccc7421530e11a52db0278 Author: Kirill Tkhai <[email protected]> AuthorDate: Wed, 22 Oct 2014 11:17:11 +0400 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 28 Oct 2014 10:46:02 +0100
sched/numa: Fix unsafe get_task_struct() in task_numa_assign()
Unlocked access to dst_rq->curr in task_numa_compare() is racy. If curr task is exiting this may be a reason of use-after-free:
task_numa_compare() do_exit()
... current->flags |= PF_EXITING;
... release_task()
... ~~delayed_put_task_struct()~~
... schedule()
rcu_read_lock() ...
cur = ACCESS_ONCE(dst_rq->curr) ...
... rq->curr = next;
... context_switch()
... finish_task_switch()
... put_task_struct()
... __put_task_struct()
... free_task_struct()
task_numa_assign() ...
get_task_struct() ...As noted by Oleg:
<<The lockless get_task_struct(tsk) is only safe if tsk == current
and didn't pass exit_notify(), or if this tsk was found on a rcu protected list (say, for_each_process() or find_task_by_vpid()). IOW, it is only safe if release_task() was not called before we take rcu_read_lock(), in this case we can rely on the fact that delayed_put_pid() can not drop the (potentially) last reference until rcu_read_unlock().
And as Kirill pointed out task_numa_compare()->task_numa_assign() path does get_task_struct(dst_rq->curr) and this is not safe. The task_struct itself can't go away, but rcu_read_lock() can't save us from the final put_task_struct() in finish_task_switch(); this reference goes away without rcu gp>>
The patch provides simple check of PF_EXITING flag. If it's not set, this guarantees that call_rcu() of delayed_put_task_struct() callback hasn't happened yet, so we can safely do get_task_struct() in task_numa_assign().
Locked dst_rq->lock protects from concurrency with the last schedule(). Reusing or unmapping of cur's memory may happen without it.
--- kernel/sched/fair.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
kernel/sched/fair.c +12 -2
@@ -1164,9 +1164,19 @@ static void task_numa_compare(struct task_numa_env *env,long moveimp = imp;rcu_read_lock (); - cur =ACCESS_ONCE (dst_rq->curr); -if (cur->pid ==0 )/* idle */ + +raw_spin_lock_irq (&dst_rq->lock); + cur = dst_rq->curr; +/* +* No need to move the exiting task, and this ensures that ->curr +* wasn't reaped and thus get_task_struct() in task_numa_assign() +* is safe under RCU read lock. +* Note that rcu_read_lock() itself can't protect from the final +* put_task_struct() after the last schedule(). +*/ +if ((cur->flags &PF_EXITING ) ||is_idle_task (cur)) cur =NULL ; +raw_spin_unlock_irq (&dst_rq->lock);/* * "imp" is the fault differential for the source task between the
Quoted 2 lines · Kirill Tkhai
Unlocked access to
dst_rq->currintask_numa_compare()is racy. If curr task is exiting this may be a reason of use-after-free:
[...]
I've complained about an unrelated issue in that part of the code a while back (https://lkml.org/lkml/2014/5/12/508) which PeterZ ended up fixing (https://lkml.org/lkml/2014/5/21/428) but it seems that both of us forgot to follow up on that and the fix never got upstream.
Ever since this patch made it upstream, Peter's patch which I was carrying in my tree stopped applying and I've started seeing:
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13 [ 829.539212] CPU: 10 PID: 11067 Comm: trinity-c594 Not tainted 3.18.0-rc3-next-20141106-sasha-00054-g09b7ccf-dirty #1448 [ 829.539226] 0000000000000000 0000000000000000 ffff880053acb000 ffff88032b71f828 [ 829.539235] ffffffffa009fb5a 0000000000000057 ffff880631dd6b80 ffff88032b71f868 [ 829.539243] ffffffff963f0c57 ffff880053acbd80 ffff880053acbdb0 ffff88032b71f858 [ 829.539246] Call Trace: [ 829.539265] dump_stack (lib/dump_stack.c:52) [ 829.539277] spin_dump (kernel/locking/spinlock_debug.c:68 (discriminator 8)) [ 829.539282] spin_bug (kernel/locking/spinlock_debug.c:76) [ 829.539288] do_raw_spin_lock (kernel/locking/spinlock_debug.c:84 kernel/locking/spinlock_debug.c:135) [ 829.539304] ? __schedule (kernel/sched/core.c:2803) [ 829.539313] _raw_spin_lock_irq (include/linux/spinlock_api_smp.h:129 kernel/locking/spinlock.c:167) [ 829.539321] ? task_numa_find_cpu (kernel/sched/fair.c:1258 kernel/sched/fair.c:1385) [ 829.539330] ? rcu_is_watching (./arch/x86/include/asm/preempt.h:95 kernel/rcu/tree.c:827) [ 829.539336] task_numa_find_cpu (kernel/sched/fair.c:1258 kernel/sched/fair.c:1385) [ 829.539342] ? task_numa_find_cpu (kernel/sched/fair.c:1253 kernel/sched/fair.c:1385) [ 829.539352] ? preempt_count_sub (kernel/sched/core.c:2644) [ 829.539358] task_numa_migrate (kernel/sched/fair.c:1452) [ 829.539364] ? task_numa_migrate (kernel/sched/fair.c:1391) [ 829.539376] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:87 arch/x86/kernel/kvmclock.c:85) [ 829.539386] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:304) [ 829.539392] ? sched_clock_local (kernel/sched/clock.c:202) [ 829.539399] numa_migrate_preferred (kernel/sched/fair.c:1539) [ 829.539404] ? sched_clock_local (kernel/sched/clock.c:202) [ 829.539411] task_numa_fault (kernel/sched/fair.c:2073) [ 829.539417] ? sched_clock_cpu (kernel/sched/clock.c:311) [ 829.539429] ? debug_smp_processor_id (lib/smp_processor_id.c:57) [ 829.539438] ? get_lock_stats (kernel/locking/lockdep.c:249) [ 829.539446] ? get_parent_ip (kernel/sched/core.c:2588) [ 829.539461] handle_mm_fault (mm/memory.c:3187 mm/memory.c:3233 mm/memory.c:3346 mm/memory.c:3375) [ 829.539466] ? find_vma (mm/mmap.c:2048) [ 829.539477] __do_page_fault (arch/x86/mm/fault.c:1246) [ 829.539485] ? context_tracking_user_exit (kernel/context_tracking.c:144) [ 829.539491] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63) [ 829.539498] ? trace_hardirqs_off_caller (kernel/locking/lockdep.c:2640 (discriminator 8)) [ 829.539505] trace_do_page_fault (arch/x86/mm/fault.c:1329 include/linux/jump_label.h:114 include/linux/context_tracking_state.h:27 include/linux/context_tracking.h:45 arch/x86/mm/fault.c:1330) [ 829.539510] do_async_page_fault (arch/x86/kernel/kvm.c:280) [ 829.539516] async_page_fault (arch/x86/kernel/entry_64.S:1301)
Thanks,
Sasha
Hi,
В Пт, 07/11/2014 в 22:48 -0500, Sasha Levin пишет:
Quoted 45 lines · Kirill Tkhai
Quoted 2 lines
Unlocked access to
dst_rq->currintask_numa_compare()is racy. If curr task is exiting this may be a reason of use-after-free:[...]
I've complained about an unrelated issue in that part of the code a while back (https://lkml.org/lkml/2014/5/12/508) which PeterZ ended up fixing (https://lkml.org/lkml/2014/5/21/428) but it seems that both of us forgot to follow up on that and the fix never got upstream.
Ever since this patch made it upstream, Peter's patch which I was carrying in my tree stopped applying and I've started seeing:
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13 [ 829.539212] CPU: 10 PID: 11067 Comm: trinity-c594 Not tainted 3.18.0-rc3-next-20141106-sasha-00054-g09b7ccf-dirty #1448 [ 829.539226] 0000000000000000 0000000000000000 ffff880053acb000 ffff88032b71f828 [ 829.539235] ffffffffa009fb5a 0000000000000057 ffff880631dd6b80 ffff88032b71f868 [ 829.539243] ffffffff963f0c57 ffff880053acbd80 ffff880053acbdb0 ffff88032b71f858 [ 829.539246] Call Trace: [ 829.539265] dump_stack (lib/dump_stack.c:52) [ 829.539277] spin_dump (kernel/locking/spinlock_debug.c:68 (discriminator 8)) [ 829.539282] spin_bug (kernel/locking/spinlock_debug.c:76) [ 829.539288] do_raw_spin_lock (kernel/locking/spinlock_debug.c:84 kernel/locking/spinlock_debug.c:135) [ 829.539304] ? __schedule (kernel/sched/core.c:2803) [ 829.539313] _raw_spin_lock_irq (include/linux/spinlock_api_smp.h:129 kernel/locking/spinlock.c:167) [ 829.539321] ? task_numa_find_cpu (kernel/sched/fair.c:1258 kernel/sched/fair.c:1385) [ 829.539330] ? rcu_is_watching (./arch/x86/include/asm/preempt.h:95 kernel/rcu/tree.c:827) [ 829.539336] task_numa_find_cpu (kernel/sched/fair.c:1258 kernel/sched/fair.c:1385) [ 829.539342] ? task_numa_find_cpu (kernel/sched/fair.c:1253 kernel/sched/fair.c:1385) [ 829.539352] ? preempt_count_sub (kernel/sched/core.c:2644) [ 829.539358] task_numa_migrate (kernel/sched/fair.c:1452) [ 829.539364] ? task_numa_migrate (kernel/sched/fair.c:1391) [ 829.539376] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:87 arch/x86/kernel/kvmclock.c:85) [ 829.539386] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:304) [ 829.539392] ? sched_clock_local (kernel/sched/clock.c:202) [ 829.539399] numa_migrate_preferred (kernel/sched/fair.c:1539) [ 829.539404] ? sched_clock_local (kernel/sched/clock.c:202) [ 829.539411] task_numa_fault (kernel/sched/fair.c:2073) [ 829.539417] ? sched_clock_cpu (kernel/sched/clock.c:311) [ 829.539429] ? debug_smp_processor_id (lib/smp_processor_id.c:57) [ 829.539438] ? get_lock_stats (kernel/locking/lockdep.c:249) [ 829.539446] ? get_parent_ip (kernel/sched/core.c:2588) [ 829.539461] handle_mm_fault (mm/memory.c:3187 mm/memory.c:3233 mm/memory.c:3346 mm/memory.c:3375)
Quoted 8 lines
[ 829.539466] ? find_vma (mm/mmap.c:2048) [ 829.539477] __do_page_fault (arch/x86/mm/fault.c:1246) [ 829.539485] ? context_tracking_user_exit (kernel/context_tracking.c:144) [ 829.539491] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63) [ 829.539498] ? trace_hardirqs_off_caller (kernel/locking/lockdep.c:2640 (discriminator 8)) [ 829.539505] trace_do_page_fault (arch/x86/mm/fault.c:1329 include/linux/jump_label.h:114 include/linux/context_tracking_state.h:27 include/linux/context_tracking.h:45 arch/x86/mm/fault.c:1330) [ 829.539510] do_async_page_fault (arch/x86/kernel/kvm.c:280) [ 829.539516] async_page_fault (arch/x86/kernel/entry_64.S:1301)
The bellow is equal to the patch suggested by Peter.
The only thing, I'm doubt, is about the comparison of cpus instead of nids. Should task_numa_compare() be able to be called with src_nid == dst_nid like this may happens now?! Maybe better, we should change task_numa_migrate() and check for env.dst_nid != env.src.nid.
kernel/sched/fair.c +2 -0
@@ -1380,6 +1380,8 @@ static void task_numa_find_cpu(struct task_numa_env *env,/* Skip this CPU if the source task cannot migrate */ if (!cpumask_test_cpu (cpu,tsk_cpus_allowed (env->p)))continue ; +if (cpu == env->src_cpu) +continue ; env->dst_cpu = cpu;task_numa_compare (env, taskimp, groupimp);
Quoted 5 lines · Kirill Tkhai
Quoted 5 lines
I've complained about an unrelated issue in that part of the code a while back (https://lkml.org/lkml/2014/5/12/508) which PeterZ ended up fixing (https://lkml.org/lkml/2014/5/21/428) but it seems that both of us forgot to follow up on that and the fix never got upstream.
Argh, sorry for that!
Quoted 13 lines
The bellow is equal to the patch suggested by Peter.
The only thing, I'm doubt, is about the comparison of cpus instead of nids. Should
task_numa_compare()be able to be called withsrc_nid==dst_nidlike this may happens now?! Maybe better, we should changetask_numa_migrate()and check for env.dst_nid != env.src.nid.kernel/sched/fair.c +0
-0
kernel/sched/fair.c static void task_numa_find_cpu(struct task_numa_env *env, · line 1387
Quoted 8 lines
kernel/sched/fair.c +2
-0@@ -1380,6 +1380,8 @@ static void task_numa_find_cpu(struct task_numa_env *env,/* Skip this CPU if the source task cannot migrate */ if (!cpumask_test_cpu (cpu,tsk_cpus_allowed (env->p)))continue ; +if (cpu == env->src_cpu) +continue ; env->dst_cpu = cpu;task_numa_compare (env, taskimp, groupimp);
Not quite the same, current can still get migrated to another cpu than env->src_cpu, at which point we can still end up selecting ourselves.
How about the below instead, that is pretty much the old patch, but with a nice comment.
---
Subject: sched, numa: Avoid selecting oneself as swap target
From: Peter Zijlstra <[email protected]>
Date: Mon Nov 10 10:54:35 CET 2014
Because the whole numa task selection stuff runs with preemption enabled (its long and expensive) we can end up migrating and selecting oneself as a swap target. This doesn't really work out well -- we end up trying to acquire the same lock twice for the swap migrate -- so avoid this.
--- kernel/sched/fair.c | 7 +++++++ 1 file changed, 7 insertions(+)
kernel/sched/fair.c +7 -0
@@ -1268,6 +1268,13 @@ static void task_numa_compare(struct tasraw_spin_unlock_irq (&dst_rq->lock);/* +* Because we have preemption enabled we can get migrated around and +* end try selecting ourselves (current == env->p) as a swap candidate. +*/ +if (cur == env->p) + goto unlock; + +/* * "imp" is the fault differential for the source task between the * source and destination node. Calculate the total differential for * the source task and potential destination task. The more negative
kernel/sched/fair.c static void task_numa_compare(struct tas · line 1275
Quoted 11 lines · Peter Zijlstra
kernel/sched/fair.c +5
-0@@ -1268,6 +1268,13 @@ static void task_numa_compare(struct tasraw_spin_unlock_irq (&dst_rq->lock);/* +* Because we have preemption enabled we can get migrated around and +* end try selecting ourselves (current == env->p) as a swap candidate. +*/ +if (cur == env->p) + goto unlock;
This is too late though, because currently the lockup happens couple of lines above that at:
raw_spin_lock_irq(&dst_rq->lock); <=== here
cur = dst_rq->curr;Which got me a bit stuck trying to use your old patch since we can't access '->curr' without locking dst_rq, but locking dst_rq is causing a lockup.
Thanks,
Sasha
Peter Zijlstra
maintainerQuoted 16 lines · Sasha Levin
kernel/sched/fair.c static void task_numa_compare(struct tas · line 1275
Quoted 11 lines · Peter Zijlstra
kernel/sched/fair.c +5
-0@@ -1268,6 +1268,13 @@ static void task_numa_compare(struct tasraw_spin_unlock_irq (&dst_rq->lock);/* +* Because we have preemption enabled we can get migrated around and +* end try selecting ourselves (current == env->p) as a swap candidate. +*/ +if (cur == env->p) + goto unlock;This is too late though, because currently the lockup happens couple of lines above that at:
Quoted 5 lines
raw_spin_lock_irq(&dst_rq->lock); <=== here cur = dst_rq->curr;Which got me a bit stuck trying to use your old patch since we can't access '->curr' without locking
dst_rq, but lockingdst_rqis causing a lockup.
confused... how can we lock up there. We should not be holding _any_ lock there.
That a different problem that the originally reported one.
tip-bot for Peter Zijlstra
merge bot[tip:sched/urgent] sched/numa: Avoid selecting oneself as swap target
In reply to Peter Zijlstrasched/numa: Avoid selecting oneself as swap target
Commit-ID: 7af683350cb0ddd0e9d3819b4eb7abe9e2d3e709 Gitweb: http://git.kernel.org/tip/7af683350cb0ddd0e9d3819b4eb7abe9e2d3e709 Author: Peter Zijlstra <[email protected]> AuthorDate: Mon, 10 Nov 2014 10:54:35 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Sun, 16 Nov 2014 10:04:17 +0100
sched/numa: Avoid selecting oneself as swap target
Because the whole numa task selection stuff runs with preemption enabled (its long and expensive) we can end up migrating and selecting oneself as a swap target. This doesn't really work out well -- we end up trying to acquire the same lock twice for the swap migrate -- so avoid this.
--- kernel/sched/fair.c | 7 +++++++ 1 file changed, 7 insertions(+)
kernel/sched/fair.c +7 -0
@@ -1180,6 +1180,13 @@ static void task_numa_compare(struct task_numa_env *env,raw_spin_unlock_irq (&dst_rq->lock);/* +* Because we have preemption enabled we can get migrated around and +* end try selecting ourselves (current == env->p) as a swap candidate. +*/ +if (cur == env->p) + goto unlock; + +/* * "imp" is the fault differential for the source task between the * source and destination node. Calculate the total differential for * the source task and potential destination task. The more negative
Peter Zijlstra
maintainerQuoted 2 lines · Sasha Levin
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13
Ooh, look at that. CPU#10 vs .owner_cpu: 13 on the _same_ task.
One of those again :/
Quoted 7 lines · Peter Zijlstra
Quoted 2 lines · Sasha Levin
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13Ooh, look at that. CPU#10 vs .owner_cpu: 13 on the _same_ task.
One of those again :/
Hum. I missed that one.
Hold on, but the magic here is fine and the owner pointer is fine, why would just the owner cpu be wrong?
Thanks,
Sasha
Peter Zijlstra
maintainerQuoted 5 lines · Sasha Levin
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines · Sasha Levin
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13
Quoted 8 lines
Quoted 3 lines
Ooh, look at that. CPU#10 vs .owner_cpu: 13 on the _same_ task.
One of those again :/
Hum. I missed that one.
Hold on, but the magic here is fine and the owner pointer is fine, why would just the owner cpu be wrong?
I've no clue, but I've seen them before. Complete mystery that.
В Пн, 10/11/2014 в 17:03 +0100, Peter Zijlstra пишет:
Quoted 7 lines · Sasha Levin
Quoted 2 lines
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13Ooh, look at that. CPU#10 vs .owner_cpu: 13 on the _same_ task.
One of those again :/
We do not initialyse task_struct::numa_preferred_nid for INIT_TASK. It there no a problem?
В Пн, 10/11/2014 в 19:10 +0300, Kirill Tkhai пишет:
Quoted 5 lines
В Пн, 10/11/2014 в 17:03 +0100, Peter Zijlstra пишет:
Quoted 4 lines · Sasha Levin
Quoted 2 lines
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13
Quoted 7 lines
Quoted 3 lines
Ooh, look at that. CPU#10 vs .owner_cpu: 13 on the _same_ task.
One of those again :/
We do not initialyse
task_struct::numa_preferred_nidforINIT_TASK. It there no a problem?
I mean task_numa_find_cpu(). If a garbage is in cpumask_of_node(env->dst_nid) and cpu is bigger than mask, the check
cpumask_test_cpu(cpu, tsk_cpus_allowed(env->p)
may be true.
So, we dereference wrong rq in task_numa_compare(). It's not rq at all. Strange cpu may be from here. It's just a int number in a wrong memory.
A hypothesis that below may help:
kernel/sched/fair.c +3 -0
@@ -1376,6 +1376,9 @@ static void task_numa_find_cpu(struct task_numa_env *env, {int cpu; +if (!node_online (env->dst_nid)) +return ; +for_each_cpu (cpu,cpumask_of_node (env->dst_nid)) {/* Skip this CPU if the source task cannot migrate */ if (!cpumask_test_cpu (cpu,tsk_cpus_allowed (env->p)))
Quoted 9 lines · Kirill Tkhai
I mean
task_numa_find_cpu(). If a garbage is incpumask_of_node(env->dst_nid)and cpu is bigger than mask, the checkcpumask_test_cpu(cpu,
tsk_cpus_allowed(env->p)may be true.
So, we dereference wrong rq in
task_numa_compare(). It's not rq at all. Strange cpu may be from here. It's just a int number in a wrong memory.
But the odds of the spinlock magic and owner pointer matching up are slim to none in that case. The memory is also likely to be valid since KASAN didn't complain about the access, so I don't believe it to be an access to freed memory.
kernel/sched/fair.c static void task_numa_find_cpu(struct task_numa_env *env, · line 1380
Quoted 13 lines
A hypothesis that below may help:
kernel/sched/fair.c +2
-0@@ -1376,6 +1376,9 @@ static void task_numa_find_cpu(struct task_numa_env *env, {int cpu; +if (!node_online (env->dst_nid)) +return ;
I've changed that to BUG_ON(!node_online(env->dst_nid)) and will run it for a bit.
Thanks,
Sasha
10.11.2014, 19:45, "Sasha Levin" <[email protected]>:
Quoted 6 lines · Kirill Tkhai
Quoted 5 lines
I mean
task_numa_find_cpu(). If a garbage is incpumask_of_node(env->dst_nid)and cpu is bigger than mask, the checkcpumask_test_cpu(cpu,
tsk_cpus_allowed(env->p)
Quoted 8 lines
Quoted 4 lines
may be true.
So, we dereference wrong rq in
task_numa_compare(). It's not rq at all. Strange cpu may be from here. It's just a int number in a wrong memory.But the odds of the spinlock magic and owner pointer matching up are slim to none in that case. The memory is also likely to be valid since KASAN didn't complain about the access, so I don't believe it to be an access to freed memory.
I'm not good with lockdep checks, so I can't judge right now... Just a hypothesis.
Quoted 10 lines
Quoted 10 lines
A hypothesis that below may help:
diff --git
a/kernel/sched/fair.cb/kernel/sched/fair.c
index 826fdf3..a2b4a8a 100644
---a/kernel/sched/fair.c
+++b/kernel/sched/fair.c
@@ -1376,6 +1376,9 @@ static void task_numa_find_cpu(structtask_numa_env*env, {
int cpu;
Quoted 5 lines
Quoted 2 lines
+ if (!node_online(env->dst_nid))
+ return;I've changed that to BUG_ON(!node_online(env->dst_nid)) and will run it for a bit.
I've looked one more time, and it looks like it's better to check for BUG_ON(env->dst_nid > MAX_NUMNODES). node_online() may do not work for insane nids.
Anyway, even if it's not connected, we need to initialize numa_preferred_nid of init_task, because it's inherited by kernel_init() (and /sbin/init too). I'll send the patch tomorrow.
Kirill
В Пн, 10/11/2014 в 23:01 +0300, Kirill Tkhai пишет:
Quoted 36 lines
10.11.2014, 19:45, "Sasha Levin" <[email protected]>:
Quoted 6 lines · Kirill Tkhai
Quoted 5 lines
I mean
task_numa_find_cpu(). If a garbage is incpumask_of_node(env->dst_nid)and cpu is bigger than mask, the checkcpumask_test_cpu(cpu,
tsk_cpus_allowed(env->p)Quoted 8 lines
Quoted 4 lines
may be true.
So, we dereference wrong rq in
task_numa_compare(). It's not rq at all. Strange cpu may be from here. It's just a int number in a wrong memory.But the odds of the spinlock magic and owner pointer matching up are slim to none in that case. The memory is also likely to be valid since KASAN didn't complain about the access, so I don't believe it to be an access to freed memory.
I'm not good with lockdep checks, so I can't judge right now... Just a hypothesis.
Quoted 10 lines
Quoted 10 lines
A hypothesis that below may help:
diff --git
a/kernel/sched/fair.cb/kernel/sched/fair.c
index 826fdf3..a2b4a8a 100644
---a/kernel/sched/fair.c
+++b/kernel/sched/fair.c
@@ -1376,6 +1376,9 @@ static void task_numa_find_cpu(structtask_numa_env*env,{ int cpu;Quoted 5 lines
Quoted 2 lines
+ if (!node_online(env->dst_nid))
+ return;I've changed that to BUG_ON(!node_online(env->dst_nid)) and will run it for a bit.
Quoted 7 lines
I've looked one more time, and it looks like it's better to check for
BUG_ON(env->dst_nid > MAX_NUMNODES).node_online()may do not work for insane nids.Anyway, even if it's not connected, we need to initialize
numa_preferred_nidofinit_task, because it's inherited bykernel_init()(and/sbin/inittoo). I'll send the patch tomorrow.
Also we can check for cpu. A problem may be in how nodes are built etc..
kernel/sched/fair.c +8 -0
@@ -1381,6 +1381,14 @@ static void task_numa_find_cpu(struct task_numa_env *env,if (!cpumask_test_cpu (cpu,tsk_cpus_allowed (env->p)))continue ; +/* +* Is cpumask_of_node() broken? In sched_init() we +* initialize only possible RQs (their rq->lock etc). +*/ +BUG_ON (cpu >=NR_CPUS || !cpu_possible (cpu)); +/* Insane node id? */ +BUG_ON (env->dst_nid >=MAX_NUMNODES || !node_online (env->dst_nid)); + env->dst_cpu = cpu;task_numa_compare (env, taskimp, groupimp); }
Quoted 6 lines · Peter Zijlstra
Quoted 2 lines · Sasha Levin
Quoted 2 lines
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13Ooh, look at that. CPU#10 vs .owner_cpu: 13 on the _same_ task.
One of those again :/
This actually reproduces pretty easily. The stack trace seems to be different but the end result is the same as above. Anything we can do to debug it? I'm really not sure how just the owner_cpu can be different here.
Thanks,
Sasha
Quoted 10 lines · Sasha Levin
Quoted 6 lines · Peter Zijlstra
Quoted 6 lines · Sasha Levin
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
[ 829.539183] BUG: spinlock recursion on CPU#10, trinity-c594/11067 [ 829.539203] lock: 0xffff880631dd6b80, .magic: dead4ead, .owner: trinity-c594/11067, .owner_cpu: 13Ooh, look at that. CPU#10 vs .owner_cpu: 13 on the _same_ task.
One of those again :/
This actually reproduces pretty easily. The stack trace seems to be different but the end result is the same as above. Anything we can do to debug it? I'm really not sure how just the
owner_cpucan be different here.
I've added saving stack traces on raw spinlock lock/unlock. Had to keep it at 8 entries, but it's enough to get the gist of it. The trace I'm seeing is:
[ 1239.788220] BUG: spinlock recursion on CPU#0, trinity-c767/32076 [ 1239.788270] irq event stamp: 9135954 [ 1239.788455] hardirqs last enabled at (9135953): free_pages_prepare (mm/page_alloc.c:770) [ 1239.788573] hardirqs last disabled at (9135954): _raw_spin_lock_irqsave (include/linux/spinlock_api_smp.h:109 kernel/locking/spinlock.c:159) [ 1239.788687] softirqs last enabled at (9134166): __do_softirq (./arch/x86/include/asm/preempt.h:22 kernel/softirq.c:296) [ 1239.788796] softirqs last disabled at (9134161): irq_exit (kernel/softirq.c:346 kernel/softirq.c:387) [ 1239.791152] lock: 0xffff8805c3dd9100, .magic: dead4ead, .owner: trinity-c767/32076, .owner_cpu: -1 [ 1239.791152] lock trace: [ 1239.791152] save_stack_trace (arch/x86/kernel/stacktrace.c:64) [ 1239.791152] do_raw_spin_trylock (kernel/locking/spinlock_debug.c:171) [ 1239.791152] _raw_spin_lock_irq (include/linux/spinlock_api_smp.h:129 kernel/locking/spinlock.c:167) [ 1239.791152] __schedule (kernel/sched/core.c:2803) [ 1239.791152] schedule (kernel/sched/core.c:2870) [ 1239.791152] p9_client_rpc (net/9p/client.c:756 (discriminator 13)) [ 1239.791152] p9_client_read (net/9p/client.c:1582) [ 1239.791152] v9fs_fid_readn (fs/9p/vfs_file.c:386) [ 1239.791152] unlock trace: [ 1239.791152] save_stack_trace (arch/x86/kernel/stacktrace.c:64) [ 1239.791152] do_raw_spin_unlock (kernel/locking/spinlock_debug.c:120 include/linux/jump_label.h:114 ./arch/x86/include/asm/spinlock.h:149 kernel/locking/spinlock_debug.c:176) [ 1239.791152] _raw_spin_unlock_irq (include/linux/spinlock_api_smp.h:168 kernel/locking/spinlock.c:199) [ 1239.791152] finish_task_switch (./arch/x86/include/asm/current.h:14 kernel/sched/core.c:2251) [ 1239.791152] __schedule (kernel/sched/core.c:2358 kernel/sched/core.c:2840) [ 1239.791152] schedule_preempt_disabled (kernel/sched/core.c:2897) [ 1239.791152] cpu_startup_entry (kernel/sched/idle.c:252 kernel/sched/idle.c:274) [ 1239.791152] start_secondary (arch/x86/kernel/smpboot.c:238) [ 1239.791152] CPU: 0 PID: 32076 Comm: trinity-c767 Not tainted 3.18.0-rc4-next-20141117-sasha-00046-g481e3e8-dirty #1471 [ 1239.791152] 0000000000000000 0000000000000000 ffff88089a90b000 ffff880867223728 [ 1239.791152] ffffffff920d1ec1 0000000000000030 ffff8805c3dd9100 ffff880867223768 [ 1239.791152] ffffffff815d0617 ffffffff9eec7730 ffff88089a90bf58 ffff8805c3dd9100 [ 1239.791152] Call Trace: [ 1239.791152] dump_stack (lib/dump_stack.c:52) [ 1239.791152] spin_dump (kernel/locking/spinlock_debug.c:81 (discriminator 8)) [ 1239.791152] spin_bug (kernel/locking/spinlock_debug.c:89) [ 1239.791152] do_raw_spin_lock (kernel/locking/spinlock_debug.c:97 kernel/locking/spinlock_debug.c:152) [ 1239.791152] ? load_balance (kernel/sched/fair.c:5582 kernel/sched/fair.c:6884) [ 1239.791152] _raw_spin_lock_irqsave (include/linux/spinlock_api_smp.h:117 kernel/locking/spinlock.c:159) [ 1239.791152] ? load_balance (kernel/sched/fair.c:5582 kernel/sched/fair.c:6884) [ 1239.791152] load_balance (kernel/sched/fair.c:5582 kernel/sched/fair.c:6884) [ 1239.791152] pick_next_task_fair (kernel/sched/fair.c:7154 kernel/sched/fair.c:5093) [ 1239.791152] ? pick_next_task_fair (kernel/sched/fair.c:7131 kernel/sched/fair.c:5093) [ 1239.791152] __schedule (kernel/sched/core.c:2716 kernel/sched/core.c:2830) [ 1239.791152] ? preempt_count_sub (kernel/sched/core.c:2644) [ 1239.791152] schedule (kernel/sched/core.c:2870) [ 1239.791152] p9_client_rpc (net/9p/client.c:756 (discriminator 13)) [ 1239.791152] ? __init_waitqueue_head (kernel/sched/wait.c:292) [ 1239.791152] ? p9_idpool_put (net/9p/util.c:127) [ 1239.791152] p9_client_read (net/9p/client.c:1582) [ 1239.791152] ? p9_free_req (net/9p/client.c:410) [ 1239.791152] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3734) [ 1239.791152] ? might_fault (mm/memory.c:3734) [ 1239.791152] v9fs_fid_readn (fs/9p/vfs_file.c:386) [ 1239.791152] v9fs_file_read (fs/9p/vfs_file.c:447) [ 1239.791152] do_loop_readv_writev (fs/read_write.c:724) [ 1239.791152] ? v9fs_fid_readn (fs/9p/vfs_file.c:433) [ 1239.791152] ? v9fs_fid_readn (fs/9p/vfs_file.c:433) [ 1239.791152] do_readv_writev (fs/read_write.c:856) [ 1239.791152] ? save_stack_trace (arch/x86/kernel/stacktrace.c:64) [ 1239.791152] vfs_readv (fs/read_write.c:882) [ 1239.791152] SyS_readv (fs/read_write.c:908 fs/read_write.c:899) [ 1239.791152] tracesys_phase2 (arch/x86/kernel/entry_64.S:529)
So I still can't explain why owner_cpu is "-1" rather than something that makes sense, but the trace *kinda* makes sense.
We locked the rq once in __schedule(), proceeded to finding the same rq as the busiest one in load_balance() and locking it yet again.
The odd thing here is that load_balance() has a check just for that:
BUG_ON(busiest == env.dst_rq);
But that doesn't get hit, although we lock it only a bit later:
more_balance:
raw_spin_lock_irqsave(&busiest->lock, flags);And there's no check before the actual lock there, so I've added another BUG_ON there and will update with results.
Thanks,
Sasha