Freezer bugfixes

The reading of PF_BORROWED_MM in is_user_space() without task_lock() is racy. Fix it.

Signed-off-by Rafael J. Wysocki <[email protected]>

Acked-by Pavel Machek <[email protected]>

---
 kernel/power/process.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6/kernel/power/process.c
===================================================================

kernel/power/process.c +1 -0

@@ -8,6 +8,7 @@
 
 #undef DEBUG
 
+#include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/module.h>

@@ -88,7 +89,12 @@ static void cancel_freezing(struct task_
 

kernel/power/process.c static void cancel_freezing(struct task_ · line 98

Quoted 10 lines · On Fri, 11 May 2007 00:36:25 +0200 "Rafael J. Wysocki" <[email protected]> wrote:

static inline int is_user_space(struct task_struct *p) {

kernel/power/process.c +6 -1

-	return p->mm && !(p->flags & PF_BORROWED_MM);
+	int ret;
+
+	task_lock(p);
+	ret = p->mm && !(p->flags & PF_BORROWED_MM);
+	task_unlock(p);
+	return ret;
 }

The whole function is racy, isn't it? I mean, the condition which it is testing can go from true->false or false->true at any instant after this function returns its now-wrong value.

iow, callers of this function need to to something to prevent the expression `p->mm && !(p->flags & PF_BORROWED_MM);' from changing value _anyway_. In which case the new locking is not needed?

Quoted 23 lines · On Fri, 11 May 2007 00:36:25 +0200 "Rafael J. Wysocki" <[email protected]> wrote:

The reading of PF_BORROWED_MM in is_user_space() without task_lock() is racy. Fix it.

Signed-off-by Rafael J. Wysocki <[email protected]>

Acked-by Pavel Machek <[email protected]>

---
 kernel/power/process.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6/kernel/power/process.c
===================================================================

kernel/power/process.c +1 -0

@@ -8,6 +8,7 @@
 
 #undef DEBUG
 
+#include <linux/sched.h>
 #include <linux/interrupt.h>
 #include <linux/suspend.h>
 #include <linux/module.h>

@@ -88,7 +89,12 @@ static void cancel_freezing(struct task_
 

kernel/power/process.c static void cancel_freezing(struct task_ · line 98

Quoted 10 lines

kernel/power/process.c +6 -1

 static inline int is_user_space(struct task_struct *p)
 {
-	return p->mm && !(p->flags & PF_BORROWED_MM);
+	int ret;
+
+	task_lock(p);
+	ret = p->mm && !(p->flags & PF_BORROWED_MM);
+	task_unlock(p);
+	return ret;
 }

The whole function is racy, isn't it? I mean, the condition which it is testing can go from true->false or false->true at any instant after this function returns its now-wrong value.

iow, callers of this function need to to something to prevent the expression `p->mm && !(p->flags & PF_BORROWED_MM);' from changing value _anyway_. In which case the new locking is not needed?

For user space processes this condition is always true.

For kernel threads:
(1) the change of tsk->mm from NULL to a nonzero value is only made in fs/aio.c:use_mm() along with the setting of PF_BORROWED_MM under the task_lock(),
(2) the change of tsk->mm from a nonzero value to NULL is only made in fs/aio.c:unuse_mm() along with the resetting of PF_BORROWED_MM under the task_lock().
Therefore, by taking the task_lock() here we make sure that the condition is alyways false when we check it for kernel threads.

Quoted 12 lines · Rafael J. Wysocki

For user space processes this condition is always true.

For kernel threads:
(1) the change of tsk->mm from NULL to a nonzero value is only made in fs/aio.c:use_mm() along with the setting of PF_BORROWED_MM under the task_lock(),
(2) the change of tsk->mm from a nonzero value to NULL is only made in fs/aio.c:unuse_mm() along with the resetting of PF_BORROWED_MM under the task_lock().
Therefore, by taking the task_lock() here we make sure that the condition is alyways false when we check it for kernel threads.

Why *test* it then and return anything?

Why not just doa "task_lock(p); task_unlock(p);" with no return value?

As it is, it sounds like either the code is buggy, or it's pointless.

Quoted 15 lines · Linus Torvalds

Quoted 12 lines · Rafael J. Wysocki

For user space processes this condition is always true.

For kernel threads:
(1) the change of tsk->mm from NULL to a nonzero value is only made in fs/aio.c:use_mm() along with the setting of PF_BORROWED_MM under the task_lock(),
(2) the change of tsk->mm from a nonzero value to NULL is only made in fs/aio.c:unuse_mm() along with the resetting of PF_BORROWED_MM under the task_lock().
Therefore, by taking the task_lock() here we make sure that the condition is alyways false when we check it for kernel threads.

Quoted 5 lines

Why *test* it then and return anything?

Why not just doa "task_lock(p); task_unlock(p);" with no return value?

As it is, it sounds like either the code is buggy, or it's pointless.

I'm not sure what you mean.

We use this function (ie. kernel/power/process.c:is_user_space()) to distinguish kernel threads from user space processes. Therefore we make it always return true for user space processes and always return false for kernel threads. In the latter case we need to use the task_lock() to ensure that the result is as desired (ie. false), because otherwise it might be racing with either fs/aio.c:use_mm() or fs/aio.c:unuse_mm().

Quoted 22 lines · On Sat, 12 May 2007 01:22:06 +0200 "Rafael J. Wysocki" <[email protected]> wrote:

Quoted 15 lines · Linus Torvalds

Quoted 12 lines · Rafael J. Wysocki

For user space processes this condition is always true.

For kernel threads:
(1) the change of tsk->mm from NULL to a nonzero value is only made in fs/aio.c:use_mm() along with the setting of PF_BORROWED_MM under the task_lock(),
(2) the change of tsk->mm from a nonzero value to NULL is only made in fs/aio.c:unuse_mm() along with the resetting of PF_BORROWED_MM under the task_lock().
Therefore, by taking the task_lock() here we make sure that the condition is alyways false when we check it for kernel threads.

Quoted 5 lines

Why *test* it then and return anything?

Why not just doa "task_lock(p); task_unlock(p);" with no return value?

As it is, it sounds like either the code is buggy, or it's pointless.

Quoted 9 lines

I'm not sure what you mean.

We use this function (ie. kernel/power/process.c:is_user_space()) to distinguish kernel threads from user space processes. Therefore we make it always return true for user space processes and always return false for kernel threads. In the latter case we need to use the task_lock() to ensure that the result is as desired (ie. false), because otherwise it might be racing with either fs/aio.c:use_mm() or fs/aio.c:unuse_mm().

ah, OK.

static void use_mm(struct mm_struct *mm)

{
	struct mm_struct *active_mm;
	struct task_struct *tsk = current;

	task_lock(tsk);
	tsk->flags |= PF_BORROWED_MM;
	active_mm = tsk->active_mm;
	atomic_inc(&mm->mm_count);
	tsk->mm = mm;
	tsk->active_mm = mm;
	/*
	 * Note that on UML this *requires* PF_BORROWED_MM to be set, otherwise
	 * it won't work. Update it accordingly if you change it here
	 */
	switch_mm(active_mm, mm, tsk);
	task_unlock(tsk);

So is_user_space() requires that the state of p->mm and p->flags be
consistent: it doesn't want to be looking at those two things in that three-statement window above.

Good changelogging and commenting save quite a bit of time and email.

We use this function (ie. kernel/power/process.c:is_user_space()) to distinguish kernel threads from user space processes. Therefore we make it always return true for user space processes and always return false for kernel threads. In the latter case we need to use the task_lock() to ensure that the result is as desired (ie. false), because otherwise it might be racing with either fs/aio.c:use_mm() or fs/aio.c:unuse_mm().

Quoted 7 lines · Rafael J. Wysocki

We use this function (ie. kernel/power/process.c:is_user_space()) to distinguish kernel threads from user space processes. Therefore we make it always return true for user space processes and always return false for kernel threads. In the latter case we need to use the task_lock() to ensure that the result is as desired (ie. false), because otherwise it might be racing with either fs/aio.c:use_mm() or fs/aio.c:unuse_mm().

But there is no race protection in the *caller*, so if it can ever return one or the other, what protects it from changing once the caller returns?

And if the value can change (because some thread uses "use_mm()"), then the caller cannot rely on the value that got returned.

So you migt as well not return any value at all, since the returned value is apparently meaningless once the lock has been released.

Quoted 3 lines

So you migt as well not return any value at all, since the returned value is apparently meaningless once the lock has been released.

No, it is not meaningless.

Rafael

Quoted 5 lines · Rafael J. Wysocki

Quoted 3 lines

So you migt as well not return any value at all, since the returned value is apparently meaningless once the lock has been released.

No, it is not meaningless.

Right.

Agreed that the returned value might not necessarily reflect the current state of thread in question. Can it pose any problems?

Case : is_user_space() returns 0:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
We think the thread is a kernel thread.
So we don't count this thread when the case is FREEZER_USER_SPACE.

Now this thread can perform an execve() and enter the userspace. It might not freeze.
So we would be declaring that all the userspace threads have been frozen, when actually we might have this one li'l unfrozen devil from the userspace.

Do we care?
For cpu hotplug, I don't think so.

Because, like Oleg pointed out, we know we'll anyway freeze all the leftover threads while handling the case FREEZER_KERNEL_THREADS.

But I am not sure if this is the case with suspend/hibernate, since we need to do a sys_sync() between try_freeze_tasks(FREEZE_USER_SPACE) and try_to_freeze_tasks(FREEZE_KERNEL_THREADS).

From the point of view of syncing it's only necessary to make sure that we

Case:is_user_space() returns 1:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
We think the thread is from userspace.
We count this thread when the case is FREEZER_USER_SPACE. Now the thread enters the kernel space by either doing a daemonize() or exit().

The freezer code loops *atleast* twice before declaring the system frozen. So if this metamorphosis occurs between iteration i and iteration i+1, in i+1, we note that this thread is now a kernel thread, and don't count it anymore.

However, between the 'i'th and 'i+1'th interation, if this thread calls try_to_freeze(), it'll enter the refrigerator. We now have a cold kernel thread when we were trying to freeze only userspace.

So, the question should be, are we clearing the TIF_FREEZING flag in places where the thread can become a kernel thread and eventually call try_to_freeze. I won't expect an exiting thread to call try_to_freeze, but a daemonize()ed thread can.

So should we perform that check in reparent_to_kthreadd() ? We are protected by the tasklist_lock there, no?

Quoted 4 lines · Gautham R Shenoy

But I am not sure if this is the case with suspend/hibernate, since we need to do a sys_sync() between try_freeze_tasks(FREEZE_USER_SPACE) and try_to_freeze_tasks(FREEZE_KERNEL_THREADS).

From the point of view of syncing it's only necessary to make sure that we won't freeze a kernel thread that's needed for the syncing. We can have an additional user space task running at this point.

Quoted 3 lines

So should we perform that check in reparent_to_kthreadd() ? We are protected by the tasklist_lock there, no?

Yes. Still, I think the daemonize()ed threads should clear their TIF_FREEZE flag unconditionally right after they have called exit_mm(). So that would be in daemonize().

Or, perhaps, it's better to clear TIF_FREEZE (unconditionally) in exit_mm(), after we've done tsk->mm = NULL? Oleg, what do you think?

Greetings,
Rafael

Quoted 9 lines · Rafael J. Wysocki

Quoted 4 lines · Gautham R Shenoy

But I am not sure if this is the case with suspend/hibernate, since we need to do a sys_sync() between try_freeze_tasks(FREEZE_USER_SPACE) and try_to_freeze_tasks(FREEZE_KERNEL_THREADS).

From the point of view of syncing it's only necessary to make sure that we won't freeze a kernel thread that's needed for the syncing. We can have an additional user space task running at this point.

Ok. Say we're might have an additional user space task which is not frozen (say A).

Quoted 11 lines

Quoted 3 lines

So should we perform that check in reparent_to_kthreadd() ? We are protected by the tasklist_lock there, no?

Yes. Still, I think the daemonize()ed threads should clear their TIF_FREEZE flag unconditionally right after they have called exit_mm(). So that would be in daemonize().

Or, perhaps, it's better to clear TIF_FREEZE (unconditionally) in exit_mm(), after we've done tsk->mm = NULL? Oleg, what do you think?

Is the following scenario possible?

FREEZE_KERNEL_THREADS:
1) Mark all leftover threads as freezeable. That would include 'A'.
2) 'A' is now daemonised and we clear TIF_FREEZE in exit_mm().
3) 'A' calls try_to_freeze() but doesn't enter the refrigerator.

Hmm, on second thought, this shouldn't matter .
The subsequent iteration will set A's TIF_FREEZE flag anyway, right? So I think it should be ok to unconditionally clear the TIF_FREEZE flag in exit_mm() after tsk->mm = NULL.

Still, the following scenario is possible while we're freezing users space tasks:

(1) user space task calls daemonize()
(2) freezer checks if this is a user space task and the test returns 'true'
(3) task calls exit_mm() and clears its TIF_FREEZE
(4) freezer sets TIF_FREEZE for the task
(5) task calls try_to_freeze() and freezes itself (bad!)

To prevent this from happening, I think, we should acquire task_lock() around the entire block in which the test is made and TIF_FREEZE is set for the task, so something more sophisticated than freezer-read-pf_borrowed_mm-in-a-nonracy-way.patch is needed.

Well, I think we should ask Andrew to drop this patch and try to address the issue in the next series of patches.

Greetings,
Rafael

Quoted 10 lines · Rafael J. Wysocki

Still, the following scenario is possible while we're freezing users space tasks:

(1) user space task calls daemonize()
(2) freezer checks if this is a user space task and the test returns 'true'
(3) task calls exit_mm() and clears its TIF_FREEZE
(4) freezer sets TIF_FREEZE for the task
(5) task calls try_to_freeze() and freezes itself (bad!)

Quoted 5 lines

To prevent this from happening, I think, we should acquire task_lock() around the entire block in which the test is made and TIF_FREEZE is set for the task, so something more sophisticated than freezer-read-pf_borrowed_mm-in-a-nonracy-way.patch is needed.

Hmmm, turns out Linus was right, after all! The caller needs to acquire the task_lock().

Quoted 2 lines

Well, I think we should ask Andrew to drop this patch and try to address the issue in the next series of patches.

I think it's a good idea.

I would want to review the patches again. The more I look at them, the better I seem to understand the subtleties in the freezer code.

Hmm, I'm not sure if the task can call try_to_freeze() after doing exit(). May it happen?

From the freezer POV an exiting task has 2 "interesting" events

Or, perhaps, it's better to clear TIF_FREEZE (unconditionally) in exit_mm(), after we've done tsk->mm = NULL? Oleg, what do you think?

	exit_notify()   ->exit_state != 0, and perhaps the task disappears
	                  from global process list

Quoted 3 lines · Rafael J. Wysocki

Therefore, by taking the task_lock() here we make sure that the condition is alyways false when we check it for kernel threads.

Why *test* it then and return anything?

Why not just doa "task_lock(p); task_unlock(p);" with no return value?

without task_lock() we can see "p->mm != NULL" but not PF_BORROWED_MM.

Quoted 2 lines · Oleg Nesterov

without task_lock() we can see "p->mm != NULL" but not PF_BORROWED_MM.

Let me explain it one more time:
- shouldn't the *caller* protect this?

Afaik, there's two situations:
- either things don't change (in which case you don't need locking at all, since things are statically one way or the other)
- or things change (in which case the caller can't rely on the return value anyway, since they might change *after* you release the lock)

Quoted 5 lines · Linus Torvalds

Quoted 2 lines · Oleg Nesterov

without task_lock() we can see "p->mm != NULL" but not PF_BORROWED_MM.

Quoted 8 lines

Let me explain it one more time:
- shouldn't the *caller* protect this?

Afaik, there's two situations:
- either things don't change (in which case you don't need locking at all, since things are statically one way or the other)
- or things change (in which case the caller can't rely on the return value anyway, since they might change *after* you release the lock)

things change, ->mm is not stable if the kernel thread does use_mm/unuse_mm.

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

things change, ->mm is not stable if the kernel thread does use_mm/unuse_mm.

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

Quoted 2 lines · Oleg Nesterov

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.

Quoted 2 lines

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.

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

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

Do we need freezer? Should we freeze kernel threads? I can't judge. I tried to read a long thread about suspend, and failed to understand it.

I personally think we can simplify things if CPU-hotplug use freezer, at least.

However, in my opininon THAT PATCH has nothing to do with this problem. It just improves the code that we already have.

Quoted 3 lines · Oleg Nesterov

However, in my opininon THAT PATCH has nothing to do with this problem. It just improves the code that we already have.

Sure.

However, I think it does it THE WRONG WAY, and doesn't actually fix the much deeper problems with the freezer, as shown by the fact that the lock is *still* broken for other cases.

So, here's a summary:

- we should not take the lock inside the function, because taking it there is fundamentally wrong, and leaves all the *other* races in place.

- if you actually want to solve the other races, the lock needs to be taken by the caller, in which case taking it in the callee is obviously (again) wrong.

- or then, we accept that the race wasn't fixed AT ALL, and you add other code to _other_ places to handle the case where you froze the wrong thread (or didn't freeze the right one).

And I'm not making that up. Look at most of the other patches in that
series: they are _exactly_ about the scenario I'm outlining.

- the whole "kernel thread vs user thread" thing is the wrong thing to check in the first place, since we just should never touch kernel threads in the first place, and anything that wants to freeze user space should have disabled exec_usermodehelper() at a higher level

That's why I'm so unhappy. The "fix" is going in the wrong direction. Each fix on their own may be an "improvement", but the end result of many of the fixes is a total mess!

We can continue to add bandaids to something broken, until it "works". But the end result, while "working", is not actually any better. Quite the reverse - the end result of something like that is that you add all these magic rules and special cases.

So in the end one ugly design decision leads to broken locking, which in turn leads to other cases where you add more broken code, which just leads to a situation where nobody actually understands what the *design* is, because there simply *isn't* any design - it's just a hodge-podge of "but this fixes a bug" ad-hoc "fixes".

Quoted 11 lines · Linus Torvalds

Quoted 3 lines · Oleg Nesterov

However, in my opininon THAT PATCH has nothing to do with this problem. It just improves the code that we already have.

Sure.

However, I think it does it THE WRONG WAY, and doesn't actually fix the much deeper problems with the freezer, as shown by the fact that the lock is *still* broken for other cases.

The other cases don't lead to the specific issue this patch is meant to prevent. Namely, that if a kernel thread is identified as a user space task by the freezer it may be frozen prematurely.

And yes, this only is a problem because we freeze kernel threads, which may be avoidable, but we've been doing that for more than two years and it's a big change to stop doing so. We can't just say overnight that we won't be freezing kernel threads from now on without al least checking if that doesn't lead to user-visible problems.

Thus IMO it's reasonable to fix the potential issue with the current code and think about changing the design *later*. Still, I'm not so attached to this patch and if you think that it should be dropped (which IMO is wrong), then so be it.

Quoted 5 lines · That

So, here's a summary:

- we should not take the lock inside the function, because taking it there is fundamentally wrong, and leaves all the *other* races in place.

The other races don't lead to the same (wrong) result.

Quoted 4 lines

- if you actually want to solve the other races, the lock needs to be taken by the caller, in which case taking it in the callee is obviously (again) wrong.

Quoted 3 lines

- or then, we accept that the race wasn't fixed AT ALL, and you add other code to _other_ places to handle the case where you froze the wrong thread (or didn't freeze the right one).

There are no other cases, AFAICS, in which we can freeze a wrong thead.

... user space tasks that call deamonize() can also be frozen prematurely. We didn't take this possibility into consideration before, which was obviously wrong.

Quoted 4 lines · Rafael J. Wysocki

... user space tasks that call deamonize() can also be frozen prematurely. We didn't take this possibility into consideration before, which was obviously wrong.

No, no, sorry for the confusion. User space tasks never call deamonize().

Kernel threads call daemonize, because when we are doing kernel_thread() on behalf of user-space task, the new kernel thread (child) shares its ->mm with the caller (parent). So it is considered as "is_user_space()" until it does daemonize().

Definitely, is_user_space() should have another name.

When a user space task exits, it does exit_mm() and becomes "a kernel thread" from the freezer POV. In its current from, freezer can do nothing with this. The exiting task won't call try_to_freeze() after that, so try_to_freeze_tasks() will wait until it dissapears (actually, until it calls exit_notify(), note the ->exit_state check in freezeable()).

I do not think we can improve things if exit_mm() clears TIF_FREEZING.

We should clear TIF_FREEZING when we set PF_NOFREEZE, I think. This was discussed before iirc, but I forgot the result.

Ah, I see. We spawn a kernel thread from a code path that belongs to a user space task and we need to call deamonize() to make it become a 'real' kernel thread.

Still, this means that is_user_space() may return 'true' for this thread before it calls daemonize() and then the scenario described by me in the previous message may occur. It seems.

It's in freezer-fix-pf_nofreeze-vs-freezeable-race.patch (appended for convenience, white space may be broken).

---

include/linux/freezer.h renamed from include/linux/freezer.h~freezer-fix-pf_nofreeze-vs-freezeable-race +0 -0

@@ -63,8 +63,10 @@ static inline int thaw_process(struct ta
   */

Quoted 5 lines · Rafael J. Wysocki

Ah, I see. We spawn a kernel thread from a code path that belongs to a user space task and we need to call deamonize() to make it become a 'real' kernel thread.

Quoted 3 lines

Still, this means that is_user_space() may return 'true' for this thread before it calls daemonize() and then the scenario described by me in the previous message may occur. It seems.

Yes sure. Probably not so bad in practice. Most likely this fresh thread is not "important" and could be freezed, I dunno.

Quoted 8 lines

It's in freezer-fix-pf_nofreeze-vs-freezeable-race.patch (appended for convenience, white space may be broken).

---

include/linux/freezer.h renamed from include/linux/freezer.h~freezer-fix-pf_nofreeze-vs-freezeable-race +0 -0

@@ -63,8 +63,10 @@ static inline int thaw_process(struct ta
   */

include/linux/freezer.h static inline int thaw_process(struct ta · line 71

Quoted 10 lines

include/linux/freezer.h renamed from include/linux/freezer.h~freezer-fix-pf_nofreeze-vs-freezeable-race +4 -2

  static inline void frozen_process(struct task_struct *p)
  {
-       p->flags |= PF_FROZEN;
-       wmb();
+       if (!unlikely(p->flags & PF_NOFREEZE)) {
+               p->flags |= PF_FROZEN;
+               wmb();
+       }
         clear_tsk_thread_flag(p, TIF_FREEZE);
  }

This is OK if a kernel thread does try_to_freeze() eventually.

But what if it does not, because it marks itself as PF_NOFREEZE? This means it may run with signal_pending() forever. That is why I think we should clear TIF_FREEZE when we set PF_NOFREEZE.

So, here's a summary:

- we should not take the lock inside the function, because taking it there is fundamentally wrong, and leaves all the *other* races in place.

- if you actually want to solve the other races, the lock needs to be taken by the caller, in which case taking it in the callee is obviously (again) wrong.

- or then, we accept that the race wasn't fixed AT ALL, and you add other code to _other_ places to handle the case where you froze the wrong thread (or didn't freeze the right one).

And I'm not making that up. Look at most of the other patches in that
series: they are _exactly_ about the scenario I'm outlining.

- the whole "kernel thread vs user thread" thing is the wrong thing to check in the first place, since we just should never touch kernel threads in the first place, and anything that wants to freeze user space should have disabled exec_usermodehelper() at a higher level

That's why I'm so unhappy. The "fix" is going in the wrong direction. Each fix on their own may be an "improvement", but the end result of many of the fixes is a total mess!

We can continue to add bandaids to something broken, until it "works". But the end result, while "working", is not actually any better. Quite the reverse - the end result of something like that is that you add all these magic rules and special cases.

So in the end one ugly design decision leads to broken locking, which in turn leads to other cases where you add more broken code, which just leads to a situation where nobody actually understands what the *design* is, because there simply *isn't* any design - it's just a hodge-podge of "but this fixes a bug" ad-hoc "fixes".

Quoted 2 lines · Oleg Nesterov

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.

Quoted 2 lines

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.