Re: [PATCH] kref: Avoid null pointer dereference after WARN

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Jun 26, 2017 at 08:52:15PM -0700, Kees Cook wrote:
> From: Daniel Micay <danielmicay@gmail.com>
> 
> The WARN_ON() checking for a NULL release pointer should be a BUG()
> since continuing with a NULL release pointer will lead to a NULL
> pointer dereference anyway.

The reason I enforced the release pointer not being NULL in the first
place, was because to not do so is a logic bug, not a run-time bug.

By putting the WARN_ON there, developers get to see the traceback and
know to fix their code.  The driver core is even harsher printing out a
message if the release function is not set, and doing a full WARN_ON().

When you just crash the machine, with BUG(), you are preventing the
developer to have a chance to know what to fix.  With WARN_ON, the
machine still runs, and they can fix up their code.  Odds are, when
someone doesn't provide a release function, their logic is such that it
will never be called anyway as their reference counting is totally
broken anyway :)

So this is a helper for the developer, let's not turn it into something
that is then a "crash the machine" issue.

Now if we could also come up with a way for the check to see if the
release function was something foolish like:
	void my_release(kref *kref) { }
that would be ideal, as the documentation explicitly says not to do
this, and I will be able to make fun of you in public if you do, yet I
constantly see _that_ happen all the time..

So let's leave this as WARN_ON(), it's not a security issue / protection
issue in any sense of the word.  It's a "let's help provide an API that
tells the developer when they use it incorrectly" type thing.

Also, you will note, all calls to kref_put() in the kernel tree do not
have this issue, so really, one could argue that this check is doing its
job :)

So I'll just drop this patch for now, thanks,

greg k-h