Messages in this thread |  | | | Date | Fri, 3 Feb 2012 17:30:56 +0100 | | From | Oleg Nesterov <> | | Subject | Re: [PATCH] staging: android/lowmemorykiller: Don't grab tasklist_lock |
| |
On 02/02, Anton Vorontsov wrote: > > On Thu, Feb 02, 2012 at 01:54:41PM +0100, Oleg Nesterov wrote: > > > > This has the same problem, force_sig() becomes unsafe. > > Ouch, I think I finally got it. So, lock_task_sighand() is trying to > gracefully grab the lock, checking if the sighand is not NULL (which means, > per __exit_signal(), that the task is halfway into the grave).
Yes.
> Would the following fix work for the sysrq? > > - - - - > From: Anton Vorontsov <anton.vorontsov@linaro.org> > Subject: [PATCH] sysrq: Fix unsafe operations on tasks > > sysrq should grab the tasklist lock, otherwise calling force_sig() is > not safe, as it might race with exiting task, which ->sighand might be > set to NULL already.
And there are other reasons for tasklist. It is not clear why for_each_process() itself is safe. OK, currently (afaics) the caller disables irqs, but in theory this doesn't imply rcu_lock.
And it can race with fork() and miss the task, although mostly in theory.
> --- a/drivers/tty/sysrq.c > +++ b/drivers/tty/sysrq.c > @@ -322,11 +322,13 @@ static void send_sig_all(int sig) > { > struct task_struct *p; > > + read_lock(&tasklist_lock); > for_each_process(p) { > if (p->mm && !is_global_init(p)) > /* Not swapper, init nor kernel thread */ > force_sig(sig, p); > } > + read_unlock(&tasklist_lock);
Agreed, but force_sig() should not be used anyway. It sends the signal to the thread, we need to kill the process. The main thread can exit.
> > With or without this patch, sig == NULL is not possible but !mm is not right, > > there could be other other threads with mm != NULL. > > I'm not sure I completely follow. In the current LMK code, we check for !mm > because otherwise the potential victim is useless for us (i.e. killing it > will not free much memory anyway).
This is the common mistake. Consider this program:
void *thread_func(...) { use_a_lot_of_memory(); } int main(void) { pthread_create(..., thread_func, ...); pthread_exit(); } lowmem_shrink() will skip this task because p->mm == NULL.
Oleg.
|  |