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

[Linus Torvalds]

On Sat, 12 May 2007, Oleg Nesterov wrote:
> 
> 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".

			Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/