Fixes for common mistakes w/ for_each_process and task->mm
Subsystems (7)
v3 cover Fixes for common mistakes w/ for_each_process and task->mm
Hi all,
This is another resend of several task->mm fixes, the bugs I found during LMK code audit. Architectures were traverse the tasklist in an unsafe manner, plus there are a few cases of unsafe access to task->mm in general.
There were no objections on the previous resend, and the final words were somewhere along "the patches are fine" line.
In v3:
- Dropped a controversal 'Make find_lock_task_mm() sparse-aware' patch;
- Reword arm and sh commit messages, per Oleg Nesterov's suggestions;
- Added an optimization trick in clear_tasks_mm_cpumask(): take only the rcu read lock, no need for the whole tasklist_lock. Suggested by Peter Zijlstra.
In v2:
- introduced a small helper in cpu.c: most arches duplicate the same [buggy] code snippet, so it's better to fix it and move the logic into a common function.
Thanks,
Signature · 2 lines
Anton Vorontsov Email: cbouatmailru at gmail.com
PATCH v3 1/9 cpu: Introduce clear_tasks_mm_cpumask() helper
Many architectures clear tasks' mm_cpumask like this:
read_lock(&tasklist_lock);
for_each_process(p) {
if (p->mm)
cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
}
read_unlock(&tasklist_lock);Depending on the context, the code above may have several problems, such as:
1. Working with task->mm w/o getting mm or grabing the task lock is dangerous as ->mm might disappear (exit_mm() assigns NULL under task_lock(), so tasklist lock is not enough).
2. Checking for process->mm is not enough because process' main thread may exit or detach its mm via use_mm(), but other threads may still have a valid mm.
This patch implements a small helper function that does things correctly, i.e.:
1. We take the task's lock while whe handle its mm (we can't use get_task_mm()/mmput() pair as mmput() might sleep);
2. To catch exited main thread case, we use find_lock_task_mm(), which walks up all threads and returns an appropriate task (with task lock held).
Also, Per Peter Zijlstra's idea, now we don't grab tasklist_lock in the new helper, instead we take the rcu read lock. We can do this because the function is called after the cpu is taken down and marked offline, so no new tasks will get this cpu set in their mm mask.
--- include/linux/cpu.h | 1 + kernel/cpu.c | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+)
include/linux/cpu.h +1 -0
@@ -179,6 +179,7 @@ extern void put_online_cpus(void);#define hotcpu_notifier (fn, pri) cpu_notifier(fn, pri)#define register_hotcpu_notifier (nb) register_cpu_notifier(nb)#define unregister_hotcpu_notifier (nb) unregister_cpu_notifier(nb) +void clear_tasks_mm_cpumask (int cpu);int cpu_down (unsigned int cpu);#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
kernel/cpu.c +26 -0
@@ -10,6 +10,8 @@#include <linux/sched.h> #include <linux/unistd.h> #include <linux/cpu.h> +#include <linux/oom.h> +#include <linux/rcupdate.h> #include <linux/export.h> #include <linux/kthread.h> #include <linux/stop_machine.h>
@@ -171,6 +173,30 @@ void __ref unregister_cpu_notifier(struct notifier_block *nb) }EXPORT_SYMBOL (unregister_cpu_notifier); +void clear_tasks_mm_cpumask (int cpu) +{ +struct task_struct *p; + +/* +* This function is called after the cpu is taken down and marked +* offline, so its not like new tasks will ever get this cpu set in +* their mm mask. -- Peter Zijlstra +* Thus, we may use rcu_read_lock() here, instead of grabbing +* full-fledged tasklist_lock. +*/ +rcu_read_lock (); +for_each_process (p) { +struct task_struct *t; + + t =find_lock_task_mm (p); +if (!t) +continue ; +cpumask_clear_cpu (cpu,mm_cpumask (t->mm)); +task_unlock (t); + } +rcu_read_unlock (); +} +static inline void check_for_tasks (int cpu) {struct task_struct *p;
Signature · 1 line
1.7.9.2
Andrew Morton
maintainerRe: [PATCH 1/9] cpu: Introduce clear_tasks_mm_cpumask() helper
Quoted 26 lines · On Mon, 23 Apr 2012 00:07:36 -0700 Anton Vorontsov…
Many architectures clear tasks'
mm_cpumasklike this:read_lock(&tasklist_lock); for_each_process(p) { if (p->mm) cpumask_clear_cpu(cpu, mm_cpumask(p->mm)); } read_unlock(&tasklist_lock);Depending on the context, the code above may have several problems, such as:
1. Working with
task->mmw/o getting mm or grabing the task lock is dangerous as ->mm might disappear (exit_mm()assigns NULL undertask_lock(), so tasklist lock is not enough).2. Checking for
process->mmis not enough because process' main thread may exit or detach its mm viause_mm(), but other threads may still have a valid mm.This patch implements a small helper function that does things correctly, i.e.:
1. We take the task's lock while whe handle its mm (we can't use get_task_mm()/mmput() pair as
mmput()might sleep);
Quoted 9 lines
2. To catch exited main thread case, we use
find_lock_task_mm(), which walks up all threads and returns an appropriate task (with task lock held).Also, Per Peter Zijlstra's idea, now we don't grab
tasklist_lockin the new helper, instead we take the rcu read lock. We can do this because the function is called after the cpu is taken down and marked offline, so no new tasks will get this cpu set in their mm mask.
Seems reasonable.
Quoted 24 lines
include/linux/cpu.h +1
-0@@ -179,6 +179,7 @@ extern void put_online_cpus(void);#define hotcpu_notifier (fn, pri) cpu_notifier(fn, pri)#define register_hotcpu_notifier (nb) register_cpu_notifier(nb)#define unregister_hotcpu_notifier (nb) unregister_cpu_notifier(nb) +void clear_tasks_mm_cpumask (int cpu);int cpu_down (unsigned int cpu);#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE kernel/cpu.c +2
-0@@ -10,6 +10,8 @@#include <linux/sched.h> #include <linux/unistd.h> #include <linux/cpu.h> +#include <linux/oom.h> +#include <linux/rcupdate.h> #include <linux/export.h> #include <linux/kthread.h> #include <linux/stop_machine.h>
kernel/cpu.c void __ref unregister_cpu_notifier(struct notifier_block *nb) · line 176
Quoted 4 lines
kernel/cpu.c +1
-0@@ -171,6 +173,30 @@ void __ref unregister_cpu_notifier(struct notifier_block *nb) }EXPORT_SYMBOL (unregister_cpu_notifier); +void clear_tasks_mm_cpumask (int cpu)
The operation of this function was presumably obvious to you at the time you wrote it, but that isn't true of other people at later times.
Please document it?
kernel/cpu.c void __ref unregister_cpu_notifier(struct notifier_block *nb)
Quoted 6 lines
kernel/cpu.c +6
-0+{ +struct task_struct *p; + +/* +* This function is called after the cpu is taken down and marked +* offline,
hm, well. Who said that this function will only ever be called after that CPU was taken down? There is nothing in the function name nor in the (absent) documentation which enforces this precondition.
If someone tries to use this function for a different purpose, or copies-and-modifies it for a different purpose, we just shot them in the foot.
They'd be pretty dumb to do that without reading the local comment, but still...
Quoted 7 lines
kernel/cpu.c +6
-0so its not like new tasks will ever get this cpu set in +* their mm mask. -- Peter Zijlstra +* Thus, we may use rcu_read_lock() here, instead of grabbing +* full-fledged tasklist_lock. +*/ +rcu_read_lock (); +for_each_process (p) {
kernel/cpu.c void __ref unregister_cpu_notifier(struct notifier_block *nb) · line 198
Quoted 10 lines
kernel/cpu.c +10
-0+struct task_struct *t; + + t =find_lock_task_mm (p); +if (!t) +continue ; +cpumask_clear_cpu (cpu,mm_cpumask (t->mm)); +task_unlock (t); + } +rcu_read_unlock (); +}
It is good that this code exists under CONFIG_HOTPLUG_CPU. Did you check that everything works correctly with CONFIG_HOTPLUG_CPU=n?
Re: [PATCH 1/9] cpu: Introduce clear_tasks_mm_cpumask() helper
Quoted 19 lines · Andrew Morton
Quoted 1 line
+void
clear_tasks_mm_cpumask(int cpu)The operation of this function was presumably obvious to you at the time you wrote it, but that isn't true of other people at later times.
Please document it?
kernel/cpu.c void __ref unregister_cpu_notifier(struct notifier_block *nb)
Quoted 6 lines
kernel/cpu.c +6
-0+{ +struct task_struct *p; + +/* +* This function is called after the cpu is taken down and marked +* offline, hm, well. Who said that this function will only ever be called after that CPU was taken down? There is nothing in the function name nor in the (absent) documentation which enforces this precondition.
Quoted 6 lines
If someone tries to use this function for a different purpose, or copies-and-modifies it for a different purpose, we just shot them in the foot.
They'd be pretty dumb to do that without reading the local comment, but still...
Methinks something simple like:
WARN_ON(cpu_online(cpu));
Ought to cure that worry, no? :-)
Quoted 18 lines
Quoted 18 lines
so its not like new tasks will ever get this cpu set in
kernel/cpu.c +16
-0+* their mm mask. -- Peter Zijlstra +* Thus, we may use rcu_read_lock() here, instead of grabbing +* full-fledged tasklist_lock. +*/ +rcu_read_lock (); +for_each_process (p) { +struct task_struct *t; + + t =find_lock_task_mm (p); +if (!t) +continue ; +cpumask_clear_cpu (cpu,mm_cpumask (t->mm)); +task_unlock (t); + } +rcu_read_unlock (); +}
PATCH 1/1 cpu: Document clear_tasks_mm_cpumask()
This patch adds more comments on clear_tasks_mm_cpumask, plus adds a runtime check: the function is only suitable for offlined CPUs, and if called inappropriately, the kernel should scream aloud.
---
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Andrew Morton
Quoted 1 line
+void
clear_tasks_mm_cpumask(int cpu)The operation of this function was presumably obvious to you at the time you wrote it, but that isn't true of other people at later times.
Please document it?
[...]
Quoted 4 lines
Quoted 4 lines
If someone tries to use this function for a different purpose, or copies-and-modifies it for a different purpose, we just shot them in the foot.
Quoted 8 lines
Quoted 2 lines
They'd be pretty dumb to do that without reading the local comment, but still...
Methinks something simple like:
WARN_ON(cpu_online(cpu));Ought to cure that worry, no? :-)
Yeah, this is all good ideas, thanks.
How about the following patch?
kernel/cpu.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
kernel/cpu.c +18 -0
@@ -13,6 +13,7 @@#include <linux/oom.h> #include <linux/rcupdate.h> #include <linux/export.h> +#include <linux/bug.h> #include <linux/kthread.h> #include <linux/stop_machine.h> #include <linux/mutex.h>
@@ -173,6 +174,18 @@ void __ref unregister_cpu_notifier(struct notifier_block *nb) }EXPORT_SYMBOL (unregister_cpu_notifier); +/** +* clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU +* @cpu: a CPU id +* +* This function walks up all processes, finds a valid mm struct for +* each one and then clears a corresponding bit in mm's cpumask. While +* this all sounds trivial, there are various non-obvious corner cases, +* which this function tries to solve in a safe manner. +* +* Also note that the function uses a somewhat relaxed locking scheme, +* so it may be called only for an already offlined CPU. +*/ void clear_tasks_mm_cpumask (int cpu) {struct task_struct *p;
@@ -184,10 +197,15 @@ void clear_tasks_mm_cpumask(int cpu)* Thus, we may use rcu_read_lock() here, instead of grabbing * full-fledged tasklist_lock. */ +WARN_ON (cpu_online (cpu));rcu_read_lock ();for_each_process (p) {struct task_struct *t; +/* +* Main thread might exit, but other threads may still have +* a valid mm. Find one. +*/ t =find_lock_task_mm (p);if (!t)continue ;
Signature · 1 line
1.7.9.2
Re: [PATCH 1/9] cpu: Introduce clear_tasks_mm_cpumask() helper
In reply to Andrew MortonOn Thu, Apr 26, 2012 at 04:59:11PM -0700, Andrew Morton wrote:
[...]
Quoted 12 lines
Quoted 12 lines
so its not like new tasks will ever get this cpu set in
kernel/cpu.c +11
-0+* their mm mask. -- Peter Zijlstra +* Thus, we may use rcu_read_lock() here, instead of grabbing +* full-fledged tasklist_lock. +*/ +rcu_read_lock (); +for_each_process (p) { +struct task_struct *t; + + t =find_lock_task_mm (p); +if (!t) +continue ;
Quoted 8 lines
kernel/cpu.c void __ref unregister_cpu_notifier(struct notifier_block *nb) · line 198
Quoted 5 lines
kernel/cpu.c +5
-0+cpumask_clear_cpu (cpu,mm_cpumask (t->mm)); +task_unlock (t); + } +rcu_read_unlock (); +}It is good that this code exists under
CONFIG_HOTPLUG_CPU. Did you check that everything works correctly withCONFIG_HOTPLUG_CPU=n?
Yeah, only the code under CONFIG_HOTPLUG_CPU calls the function, so it should be all fine.
Thanks!
Signature · 2 lines
Anton Vorontsov Email: cbouatmailru at gmail.com
PATCH v3 2/9 arm: Use clear_tasks_mm_cpumask()
In reply to Anton VorontsovChecking for process->mm is not enough because process' main thread may exit or detach its mm via use_mm(), but other threads may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would walk up all threads and returns an appropriate task (with task lock held).
clear_tasks_mm_cpumask() has this issue fixed, so let's use it.
--- arch/arm/kernel/smp.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
arch/arm/kernel/smp.c +1 -7
@@ -130,7 +130,6 @@ static void percpu_timer_stop(void);int __cpu_disable (void ) {unsigned int cpu =smp_processor_id (); -struct task_struct *p;int ret; ret =platform_cpu_disable (cpu);
@@ -160,12 +159,7 @@ int __cpu_disable(void)flush_cache_all ();local_flush_tlb_all (); -read_lock (&tasklist_lock); -for_each_process (p) { -if (p->mm) -cpumask_clear_cpu (cpu,mm_cpumask (p->mm)); - } -read_unlock (&tasklist_lock); +clear_tasks_mm_cpumask (cpu);return 0 ; }
Signature · 1 line
1.7.9.2
PATCH v3 3/9 powerpc: Use clear_tasks_mm_cpumask()
In reply to Anton VorontsovCurrent CPU hotplug code has some task->mm handling issues:
1. Working with task->mm w/o getting mm or grabing the task lock is dangerous as ->mm might disappear (exit_mm() assigns NULL under task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep, so we must take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main thread may exit or detach its mm via use_mm(), but other threads may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would walk up all threads and returns an appropriate task (with task lock held).
clear_tasks_mm_cpumask() has all the issues fixed, so let's use it.
--- arch/powerpc/mm/mmu_context_nohash.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
arch/powerpc/mm/mmu_context_nohash.c +2 -9
@@ -333,9 +333,7 @@ static int __cpuinit mmu_context_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu) {unsigned int cpu = (unsigned int )(long )hcpu; -#ifdef CONFIG_HOTPLUG_CPU -struct task_struct *p; -#endif +/* We don't touch CPU 0 map, it's allocated at aboot and kept * around forever */
@@ -358,12 +356,7 @@ static int __cpuinit mmu_context_cpu_notify(struct notifier_block *self, stale_map[cpu] =NULL ;/* We also clear the cpu_vm_mask bits of CPUs going away */ -read_lock (&tasklist_lock); -for_each_process (p) { -if (p->mm) -cpumask_clear_cpu (cpu,mm_cpumask (p->mm)); - } -read_unlock (&tasklist_lock); +clear_tasks_mm_cpumask (cpu);break ;#endif /* CONFIG_HOTPLUG_CPU */ }
Signature · 1 line
1.7.9.2
PATCH v3 4/9 sh: Use clear_tasks_mm_cpumask()
In reply to Anton VorontsovChecking for process->mm is not enough because process' main thread may exit or detach its mm via use_mm(), but other threads may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would walk up all threads and returns an appropriate task (with task lock held).
clear_tasks_mm_cpumask() has the issue fixed, so let's use it.
--- arch/sh/kernel/smp.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
arch/sh/kernel/smp.c +1 -6
@@ -123,7 +123,6 @@ void native_play_dead(void)int __cpu_disable (void ) {unsigned int cpu =smp_processor_id (); -struct task_struct *p;int ret; ret = mp_ops->cpu_disable (cpu);
@@ -153,11 +152,7 @@ int __cpu_disable(void)flush_cache_all ();local_flush_tlb_all (); -read_lock (&tasklist_lock); -for_each_process (p) -if (p->mm) -cpumask_clear_cpu (cpu,mm_cpumask (p->mm)); -read_unlock (&tasklist_lock); +clear_tasks_mm_cpumask (cpu);return 0 ; }
Signature · 1 line
1.7.9.2
PATCH v3 5/9 blackfin: A couple of task->mm handling fixes
In reply to Anton VorontsovThe patch fixes two problems:
1. Working with task->mm w/o getting mm or grabing the task lock is dangerous as ->mm might disappear (exit_mm() assigns NULL under task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep, so we have to take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main thread may exit or detach its mm via use_mm(), but other threads may still have a valid mm.
To catch this we use find_lock_task_mm(), which walks up all threads and returns an appropriate task (with task lock held).
--- arch/blackfin/kernel/trace.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
arch/blackfin/kernel/trace.c +13 -13
@@ -10,6 +10,8 @@#include <linux/hardirq.h> #include <linux/thread_info.h> #include <linux/mm.h> +#include <linux/oom.h> +#include <linux/sched.h> #include <linux/uaccess.h> #include <linux/module.h> #include <linux/kallsyms.h>
@@ -28,7 +30,6 @@ void decode_address(char *buf, unsigned long address)struct task_struct *p;struct mm_struct *mm;unsigned long flags, offset; -unsigned char in_atomic = (bfin_read_IPEND () &0x10 ) ||in_atomic ();struct rb_node *n;#ifdef CONFIG_KALLSYMS
@@ -114,15 +115,15 @@ void decode_address(char *buf, unsigned long address)*/ write_lock_irqsave (&tasklist_lock, flags);for_each_process (p) { - mm = (in_atomic ? p->mm :get_task_mm (p)); -if (!mm) -continue ; +struct task_struct *t; -if (!down_read_trylock (&mm->mmap_sem)) { -if (!in_atomic) -mmput (mm); + t =find_lock_task_mm (p); +if (!t)continue ; - } + + mm = t->mm; +if (!down_read_trylock (&mm->mmap_sem)) + goto __continue;for (n =rb_first (&mm->mm_rb); n; n =rb_next (n)) {struct vm_area_struct *vma;
@@ -131,7 +132,7 @@ void decode_address(char *buf, unsigned long address)if (address >= vma->vm_start && address < vma->vm_end) {char _tmpbuf[256 ]; -char *name = p->comm; +char *name = t->comm;struct file *file = vma->vm_file;if (file) {
@@ -164,8 +165,7 @@ void decode_address(char *buf, unsigned long address) name, vma->vm_start, vma->vm_end);up_read (&mm->mmap_sem); -if (!in_atomic) -mmput (mm); +task_unlock (t);if (buf[0 ] =='\0' )sprintf (buf,"[ %s ] dynamic memory" , name);
@@ -175,8 +175,8 @@ void decode_address(char *buf, unsigned long address) }up_read (&mm->mmap_sem); -if (!in_atomic) -mmput (mm); +__continue: +task_unlock (t); }/*
Signature · 1 line
1.7.9.2
Re: [PATCH 5/9] blackfin: A couple of task->mm handling fixes
Quoted 3 lines · On Monday 23 April 2012 03:09:01 Anton Vorontsov wrote:
1. Working with
task->mmw/o getting mm or grabing the task lock is dangerous as ->mm might disappear (exit_mm()assigns NULL undertask_lock(), so tasklist lock is not enough).
that isn't a problem for this code as it specifically checks if it's in an atomic section. if it is, then task->mm can't go away on us.
Quoted 2 lines
We can't use get_task_mm()/mmput() pair as
mmput()might sleep, so we have to take the task lock while handle its mm.
if we're not in an atomic section, then sleeping is fine.
Quoted 3 lines
2. Checking for
process->mmis not enough because process' main thread may exit or detach its mm viause_mm(), but other threads may still have a valid mm.
i don't think it matters for this code (per the reasons above).
Quoted 2 lines
To catch this we use
find_lock_task_mm(), which walks up all threads and returns an appropriate task (with task lock held).
certainly fine for the non-atomic code path. i guess we'll notice in crashes
if it causes a problem in atomic code paths as well.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120601/ff9a5018/attachment.sig>
PATCH v3 6/9 blackfin: Fix possible deadlock in decode_address()
In reply to Anton VorontsovOleg Nesterov found an interesting deadlock possibility:
Quoted 3 lines
sysrq_showregs_othercpus()doessmp_call_function(showacpu)andshowacpu()show_stack()->decode_address(). Now suppose that IPI interrupts the task holdingread_lock(tasklist).
To fix this, blackfin should not grab the write_ variant of the tasklist lock, read_ one is enough.
--- arch/blackfin/kernel/trace.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/blackfin/kernel/trace.c +3 -3
@@ -29,7 +29,7 @@ void decode_address(char *buf, unsigned long address) {struct task_struct *p;struct mm_struct *mm; -unsigned long flags, offset; +unsigned long offset;struct rb_node *n;#ifdef CONFIG_KALLSYMS
@@ -113,7 +113,7 @@ void decode_address(char *buf, unsigned long address)* mappings of all our processes and see if we can't be a whee * bit more specific */ -write_lock_irqsave (&tasklist_lock, flags); +read_lock (&tasklist_lock);for_each_process (p) {struct task_struct *t;
@@ -186,7 +186,7 @@ __continue:sprintf (buf,"/* kernel dynamic memory */" ); done: -write_unlock_irqrestore (&tasklist_lock, flags); +read_unlock (&tasklist_lock); }#define EXPAND_LEN ((1 << CONFIG_DEBUG_BFIN_HWTRACE_EXPAND_LEN) * 256 - 1)
Signature · 1 line
1.7.9.2
PATCH v3 7/9 um: Should hold tasklist_lock while traversing processes
In reply to Anton VorontsovTraversing the tasks requires holding tasklist_lock, otherwise it is unsafe.
p.s. However, I'm not sure that calling os_kill_ptraced_process() in the atomic context is correct. It seem to work, but please take a closer look.
--- arch/um/kernel/reboot.c | 3 +++ 1 file changed, 3 insertions(+)
arch/um/kernel/reboot.c +3 -0
@@ -4,6 +4,7 @@*/ #include "linux/sched.h" +#include "linux/spinlock.h" #include "linux/slab.h" #include "kern_util.h" #include "os.h"
@@ -22,6 +23,7 @@ static void kill_off_processes(void)struct task_struct *p;int pid; +read_lock (&tasklist_lock);for_each_process (p) {if (p->mm ==NULL )continue ;
@@ -29,6 +31,7 @@ static void kill_off_processes(void) pid = p->mm->context.id.u.pid;os_kill_ptraced_process (pid,1 ); } +read_unlock (&tasklist_lock); } }
Signature · 1 line
1.7.9.2
Richard Weinberger
maintainerRe: [PATCH 7/9] um: Should hold tasklist_lock while traversing processes
Quoted 9 lines · Anton Vorontsov
Traversing the tasks requires holding
tasklist_lock, otherwise it is unsafe.p.s. However, I'm not sure that calling
os_kill_ptraced_process()in the atomic context is correct. It seem to work, but please take a closer look.Signed-off-by Anton Vorontsov<[email protected]>---
You forgot my Ack and I've already explained whyos_kill_ptraced_process() is fine.
Thanks,
//richard
Re: [PATCH 7/9] um: Should hold tasklist_lock while traversing processes
Quoted 8 lines · Richard Weinberger
Quoted 7 lines · Anton Vorontsov
Traversing the tasks requires holding
tasklist_lock, otherwise it is unsafe.p.s. However, I'm not sure that calling
os_kill_ptraced_process()in the atomic context is correct. It seem to work, but please take a closer look.
Quoted 5 lines
Quoted 2 lines
Signed-off-by Anton Vorontsov<[email protected]>---
You forgot my Ack and I've already explained why
os_kill_ptraced_process()is fine.
Ouch, sorry!
Signature · 2 lines
Anton Vorontsov Email: cbouatmailru at gmail.com
PATCH v3 8/9 um: Fix possible race on task->mm
In reply to Anton VorontsovChecking for task->mm is dangerous as ->mm might disappear (exit_mm() assigns NULL under task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep, so let's take the task lock while we care about its mm.
Note that we should also use find_lock_task_mm() to check all process' threads for a valid mm, but for uml we'll do it in a separate patch.
--- arch/um/kernel/reboot.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
arch/um/kernel/reboot.c +5 -2
@@ -25,10 +25,13 @@ static void kill_off_processes(void)read_lock (&tasklist_lock);for_each_process (p) { -if (p->mm ==NULL ) +task_lock (p); +if (!p->mm) { +task_unlock (p);continue ; - + } pid = p->mm->context.id.u.pid; +task_unlock (p);os_kill_ptraced_process (pid,1 ); }read_unlock (&tasklist_lock);
Signature · 1 line
1.7.9.2
PATCH v3 9/9 um: Properly check all process' threads for a live mm
In reply to Anton Vorontsovkill_off_processes() might miss a valid process, this is because checking for process->mm is not enough. Process' main thread may exit or detach its mm via use_mm(), but other threads may still have a valid mm.
To catch this we use find_lock_task_mm(), which walks up all threads and returns an appropriate task (with task lock held).
--- arch/um/kernel/reboot.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
arch/um/kernel/reboot.c +7 -6
@@ -6,6 +6,7 @@#include "linux/sched.h" #include "linux/spinlock.h" #include "linux/slab.h" +#include "linux/oom.h" #include "kern_util.h" #include "os.h" #include "skas.h"
@@ -25,13 +26,13 @@ static void kill_off_processes(void)read_lock (&tasklist_lock);for_each_process (p) { -task_lock (p); -if (!p->mm) { -task_unlock (p); +struct task_struct *t; + + t =find_lock_task_mm (p); +if (!t)continue ; - } - pid = p->mm->context.id.u.pid; -task_unlock (p); + pid = t->mm->context.id.u.pid; +task_unlock (t);os_kill_ptraced_process (pid,1 ); }read_unlock (&tasklist_lock);
Signature · 1 line
1.7.9.2