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.