On Wed, 10 Sep 2014, Theodore Ts'o wrote: > 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.
I of course have no objections to this check being added to whatever static checker, that would be very welcome improvement. Still, I believe that kernel shouldn't be just ignoring kfree(ERR_PTR) happening. Would something like the below be more acceptable? From: Jiri Kosina <[email protected]> Subject: [PATCH] mm/sl[aou]b: make kfree() aware of error pointers Freeing if ERR_PTR is not covered by ZERO_OR_NULL_PTR() check already present in kfree(), but it happens in the wild and has disastrous effects. Issue a warning and don't proceed trying to free the memory if CONFIG_DEBUG_SLAB is set. Inspired by a9cfcd63e8d ("ext4: avoid trying to kfree an ERR_PTR pointer"). Signed-off-by: Jiri Kosina <[email protected]> --- mm/slab.c | 6 ++++++ mm/slob.c | 7 ++++++- mm/slub.c | 7 ++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index a467b30..6f49d6b 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3612,6 +3612,12 @@ void kfree(const void *objp) trace_kfree(_RET_IP_, objp); +#ifdef CONFIG_DEBUG_SLAB + if (unlikely(IS_ERR(objp))) { + WARN(1, "trying to free ERR_PTR\n"); + return; + } +#endif if (unlikely(ZERO_OR_NULL_PTR(objp))) return; local_irq_save(flags); diff --git a/mm/slob.c b/mm/slob.c index 21980e0..66422a0 100644 --- a/mm/slob.c +++ b/mm/slob.c @@ -488,7 +488,12 @@ void kfree(const void *block) struct page *sp; trace_kfree(_RET_IP_, block); - +#ifdef CONFIG_DEBUG_SLAB + if (unlikely(IS_ERR(block))) { + WARN(1, "trying to free ERR_PTR\n"); + return; + } +#endif if (unlikely(ZERO_OR_NULL_PTR(block))) return; kmemleak_free(block); diff --git a/mm/slub.c b/mm/slub.c index 3e8afcc..21155ae 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3337,7 +3337,12 @@ void kfree(const void *x) void *object = (void *)x; trace_kfree(_RET_IP_, x); - +#ifdef CONFIG_DEBUG_SLAB + if (unlikely(IS_ERR(x))) { + WARN(1, "trying to free ERR_PTR\n"); + return; + } +#endif if (unlikely(ZERO_OR_NULL_PTR(x))) return; -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
