Re: [PATCH] mm/sl[aou]b: make kfree() aware of error pointers

[Theodore Ts'o]

On Wed, Sep 10, 2014 at 07:05:40AM +0200, Jiri Kosina wrote:
> > kfree() is quite a hot path to which this will add overhead.  And we
> > have (as far as we know) no code which will actually use this at
> > present.
> 
> We obviously don't, as such code will be causing explosions. This is meant 
> as a prevention of problems such as the one that has just been fixed in 
> ext4.

These sorts of things can happen a lot, unfortunately.  We had a
number of bugs in ext4 where ext4 would explode if a GFP_NOFS kmalloc
would fail.  These bugs have been around for a long time, and
apparently *none* of the RHEL/SLES/OUL enterprise linux
certification/QA efforts found them.

I only found them when an a Google-internal-only patch introduced an
mm behavioural change that caused GFP_NOFS allocations to fail under
extreme memory pressure.  And that's exactly the sort of thing that
would cause disasgters when you might have some function such as:

	ptr = function_that_does_an_kmalloc(...)
	if (IS_ERR(ptr)) {
		ret = PTR_ERR(ptr);
		goto cleanup);
	}
	bh = function_that_does_a_getblk(...)
	if (IS_ERR(bh)) {
		ret = PTR_ERR(bh);
		goto cleanup);
	}
	....

cleanup:
	if (bh)
		brelse(bh);
	if (ptr)
		kfree(ptr);


Normally, kfree and bh would be allocated, and so kfree() and brelse()
does the right thing.  But if something changes that causes functions
that in practice, never returned an allocation failure, suddenly
*does* start failing, then you get an explosion.  And/or, previous to
recent mainline patches, the kernel might BUG, and/or declare the file
system corrupt, forcing an fsck --- and when *large* number of systems
get stuck in an fsck at the same time, it tends to distress the system
administrators, far worse than if the kernel had merely exploded.  :-/

So I wouldn't be so sure that we don't have these sorts of bugs hiding
somewhere; and it's extremely easy for them to sneak in.  That being
said, I'm not in favor of making changes to kfree; I'd much rather
depending on better testing and static checkers to fix them, since
kfree *is* a hot path.

Cheers,

					- Ted
--
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/