Re: [PATCH 1/7] Freezer: Read PF_BORROWED_MM in a nonracy way

[Linus Torvalds <torvalds@linux-foundation.org>]

On Sat, 12 May 2007, Oleg Nesterov wrote:
> 
> things change, ->mm is not stable if the kernel thread does use_mm/unuse_mm.

->mm is not stable *regardless*!

Trivial examples:
 - kernel thread does execve()
 - user thread does exit().

The use "use_mm()" and "unuse_mm()" things are total red herrings.

If the freezer depends on the difference between user and kernel threads, 
then THAT PATCH IS BUGGY. It's that simple. It tests something that simply 
isn't stable outside the lock, and then returns that value after having 
unlocked it.

It might as well return a random number.

> However, the return value == 0 does not change in that particular case,
> exactly because is_user_space() takes task_lock().

As does exit_mm() etc.

That's NOT THE POINT. You cannot use the end result after releasing the 
task lock, because the moment you release the task lock, it becomes 
totally irrelevant, and may not be true any more.

Example (a):
 - you ask "is_user_space(p)", it returns 1.
 - before you actually have time to do anything about it, the task exists, 
   and (since you don't hold the lock any more) will now have a NULL 
   tsk->mm again (and would now return 0 if you called it again).

Example (b):
 - you ask "is_user_space(p)" and it returns 0, because it's a kernel 
   thread
 - before you actually do anything about it (but after you released the 
   task lock), the kernel thread does an "execve(/sbin/hotplug)" and is no 
   longer a kernel thread.

In both cases will the caller have a return value THAT IS NO LONGER TRUE.

See? The locking was pointless. Exactly because you release the lock 
before the user can actually do anything about the return value!

The fact that the locking protects against the very specific case of AIO 
where the threads _stay_ user tasks and don't really change is pretty much 
irrelevant, as far as I can see. 

Anyway, I think the whole freezer thing is broken. There's no reason to 
freeze kernel threads. 

If you want to freeze user processes, go ahead. But then you need a lock 
to make sure that new processes don't *become* user processes (ie you need 
to disable hotplug). 

And if you want to protect against cpufreq, do so. But don't try to say 
that you want to freeze all kernel threads. Just protect against cpufreq 
threads. Don't make all the other threads that have *zero* interest in 
freezing have to worry about it.

			Linus