linux-kernel - Re: v3.4-rc2 out-of-memory problems (was Re: 3.4-rc1 sticks-and-crashs)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFwa40cv_TYdU3dKHPPJQo2omLpi8y6k6KBhJ5uP12f29A@mail.gmail.com>
Date:	Mon, 9 Apr 2012 16:55:23 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	werner <w.landgraf@...ru>, Rik van Riel <riel@...hat.com>,
	Hugh Dickins <hughd@...gle.com>, linux-kernel@...r.kernel.org,
	Oleg Nesterov <oleg@...hat.com>,
	Rabin Vincent <rabin.vincent@...ricsson.com>,
	Christian Bejram <christian.bejram@...ricsson.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Anton Vorontsov <anton.vorontsov@...aro.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	stable@...r.kernel.org, Ingo Molnar <mingo@...nel.org>
Subject: Re: v3.4-rc2 out-of-memory problems (was Re: 3.4-rc1 sticks-and-crashs)

On Mon, Apr 9, 2012 at 4:25 PM, David Rientjes <rientjes@...gle.com> wrote:
>
> You could that if you also turned the check for "ret == NOTIFY_OK" in
> profile_handoff_task() into "ret & NOTIFY_OK" in your patch, otherwise you
> get a double free from __put_task_struct() and oprofile.

Why? NOTIFY_DONE is zero.

I do agree that we *also* could do the "& NOTIFY_OK" and make it
clearer that we're oring bits together. And we could document the
stupid notifier interfaces to do this all, and just make the rules be
*sane* when you have multiple notifiers.

And sane rules would be either:

 - you always return an error return, and notifiers all return either
0 or a negative error number, and we stop on the first error and
return that.

 - you return a bitmask, and we or all bits together (and we can
certainly continue to have a "stop here" bit)

But the current notifier semantics are just insane. The whole "we
return the last return value" is crazy. It's by definition a random
number, since the whole point of notifiers is that there can be
multiple, and they aren't "ordered". So the whole "last return value"
is something I just look at and say: "Whoever designed that is a
f*cking moron".

(And if that happens to be some younger version of me, I am happy that
I got over it. But I'm pretty sure I have never touched that broken
notifier code in my life)

> It works fine if the callbacks are correctly implemented, it's just that
> the task handoff in kernel/profile.c is broken because it assumes only one
> callback will return NOTIFY_OK, meaning it will eventually free, and its
> only checking the return value of the last notifier called to see if
> __put_task_struct() should immediately free.

We can easily document it as "only oprofile is allowed to return
NOTIFY_OK, this notifier is a big mess, don't even *think* about
returning anything but NOTIFY_DONE".

>  (1)  fix the lowmemorykiller so it doesn't need to use these notifiers at
>      all, which isn't difficult, for 3.4, then

I do think that that makes sense. Fixing people to not use notifiers
is always a good idea. Why would anybody sane even care about the
process going away anyway? If some lowmemorykiller decides to kill off
a process that no longer exists, kill() should happily return ENOSRCH,
and we're all good

So it could just use a "pid", and test for existence with send_sig()
or lookup_pid() or something.

>  (2a) change the task handoff to a refcount on task->usage after the final
>      put_task_struct() using the notifier and then allow it to be freed
>      after everybody does a put_handoff_task_struct() for 3.5

The task handoff code runs too late right now. I guess we could easily
move it up, though.

At the same time, the *only* user of that stupid handoff thing is
oprofile, afaik, and if we use a refcount, why the hell doesn't
oprofile just use a refcount to begin with, instead of using that
notifier?: IOW, *both* users of the notifier seem to be just retarded.

So I'd rather just kill the stupid notifier entirely. In the meantime,
making lowmemorykiller just return zero instead just "fixes" it
(assuming we make the notifier semantics for multiple return codes
sane, which they clearly aren't).

Again, almost every notifier user has always been total crap. It's
just a stupid abstraction. "Something happened". "Oh, ok".

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