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

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

[Jiri Kosina]

kfree() is happy to accept NULL pointer and does nothing in such case. 
It's reasonable to expect it to behave the same if ERR_PTR is passed to 
it.

Inspired by a9cfcd63e8d ("ext4: avoid trying to kfree an ERR_PTR 
pointer").

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 mm/slab.c | 2 +-
 mm/slob.c | 2 +-
 mm/slub.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index a467b30..1a256ac 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3612,7 +3612,7 @@ void kfree(const void *objp)
 
 	trace_kfree(_RET_IP_, objp);
 
-	if (unlikely(ZERO_OR_NULL_PTR(objp)))
+	if (unlikely(ZERO_OR_NULL_PTR(objp) || IS_ERR(objp)))
 		return;
 	local_irq_save(flags);
 	kfree_debugcheck(objp);
diff --git a/mm/slob.c b/mm/slob.c
index 21980e0..3abc42c 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -489,7 +489,7 @@ void kfree(const void *block)
 
 	trace_kfree(_RET_IP_, block);
 
-	if (unlikely(ZERO_OR_NULL_PTR(block)))
+	if (unlikely(ZERO_OR_NULL_PTR(block) || IS_ERR(objp)))
 		return;
 	kmemleak_free(block);
 
diff --git a/mm/slub.c b/mm/slub.c
index 3e8afcc..46d18ce 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3338,7 +3338,7 @@ void kfree(const void *x)
 
 	trace_kfree(_RET_IP_, x);
 
-	if (unlikely(ZERO_OR_NULL_PTR(x)))
+	if (unlikely(ZERO_OR_NULL_PTR(x) || IS_ERR(objp)))
 		return;
 
 	page = virt_to_head_page(x);

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrew Morton]

On Tue, 9 Sep 2014 23:25:28 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> wrote:

> kfree() is happy to accept NULL pointer and does nothing in such case. 
> It's reasonable to expect it to behave the same if ERR_PTR is passed to 
> it.
> 
> Inspired by a9cfcd63e8d ("ext4: avoid trying to kfree an ERR_PTR 
> pointer").
> 
> ...
>
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3612,7 +3612,7 @@ void kfree(const void *objp)
>  
>  	trace_kfree(_RET_IP_, objp);
>  
> -	if (unlikely(ZERO_OR_NULL_PTR(objp)))
> +	if (unlikely(ZERO_OR_NULL_PTR(objp) || IS_ERR(objp)))
>  		return;

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.

How about a new

kfree_safe(...)
{
	if (IS_ERR(...))
		return;
	if (other-stuff-when-we-think-of-it)
		return;
	kfree(...);
}

?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jiri Kosina]

On Tue, 9 Sep 2014, Andrew Morton wrote:

> > kfree() is happy to accept NULL pointer and does nothing in such case. 
> > It's reasonable to expect it to behave the same if ERR_PTR is passed to 
> > it.
> > 
> > Inspired by a9cfcd63e8d ("ext4: avoid trying to kfree an ERR_PTR 
> > pointer").
> > 
> > ...
> >
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -3612,7 +3612,7 @@ void kfree(const void *objp)
> >  
> >  	trace_kfree(_RET_IP_, objp);
> >  
> > -	if (unlikely(ZERO_OR_NULL_PTR(objp)))
> > +	if (unlikely(ZERO_OR_NULL_PTR(objp) || IS_ERR(objp)))
> >  		return;
> 
> 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.

> How about a new
> 
> kfree_safe(...)
> {
> 	if (IS_ERR(...))
> 		return;
> 	if (other-stuff-when-we-think-of-it)
> 		return;
> 	kfree(...);
> }

I think this unfortunately undermines the whole point of the patch ... if 
the caller knows that he might potentially be feeding ERR_PTR() to 
kfree(), he can as well check the pointer himself.

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrew Morton]

On Wed, 10 Sep 2014 07:05:40 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> wrote:

> > > --- a/mm/slab.c
> > > +++ b/mm/slab.c
> > > @@ -3612,7 +3612,7 @@ void kfree(const void *objp)
> > >  
> > >  	trace_kfree(_RET_IP_, objp);
> > >  
> > > -	if (unlikely(ZERO_OR_NULL_PTR(objp)))
> > > +	if (unlikely(ZERO_OR_NULL_PTR(objp) || IS_ERR(objp)))
> > >  		return;
> > 
> > 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.

Well.  I bet there exist sites which can pass an ERR_PTR to kfree but
haven't been know to do so yet because errors are rare.  Your patch
would fix all those by magic, but is it worth the overhead?

This is the sort of error which a static checker could find.  I wonder
if any of them do so.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Dan Carpenter]

On Tue, Sep 09, 2014 at 10:11:38PM -0700, Andrew Morton wrote:
> On Wed, 10 Sep 2014 07:05:40 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> wrote:
> This is the sort of error which a static checker could find.  I wonder
> if any of them do so.

Yes.  Ted asked me to add this to Smatch and that's how we found the
problems in ext4.  I'll push it out later this week.  It won't find
every single bug.

We have fixed the 8 bugs that Smatch found.

regards,
dan carpenter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Theodore Ts'o]

On Wed, Sep 10, 2014 at 09:36:30AM +0300, Dan Carpenter wrote:
> On Tue, Sep 09, 2014 at 10:11:38PM -0700, Andrew Morton wrote:
> > On Wed, 10 Sep 2014 07:05:40 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> wrote:
> > This is the sort of error which a static checker could find.  I wonder
> > if any of them do so.
> 
> Yes.  Ted asked me to add this to Smatch and that's how we found the
> problems in ext4.  I'll push it out later this week.  It won't find
> every single bug.
> 
> We have fixed the 8 bugs that Smatch found.

The ironic thing is that I asked Dan to add the feature to smatch
because I found two such bugs in ext4, and I suspected there would be
more.  Sure enough, it found four more such bugs, including two in a
recent commit where I had found the first two bugs --- and I had
missed the other two even though I was specifically looking for such
instances.  Oops.  :-)

Maybe we can add a debugging config option?  I think having static
checkers plus some kmalloc failure testing should be sufficient to
prevent these sorts of problem from showing up.

It would seem to me that this is the sort of thing that a static
checker should find reliably; Coverity has found things that were more
complex than what this should require, I think.  I don't know if they
would be willing to add something this kernel-specific, though.  (I've
added Dave Jones to the thread since he's been working a lot with
Coverity; Dave, what do you think?)

      	 	    		       - Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Dave Jones]

On Wed, Sep 10, 2014 at 09:56:49AM -0400, Theodore Ts'o wrote:

 > The ironic thing is that I asked Dan to add the feature to smatch
 > because I found two such bugs in ext4, and I suspected there would be
 > more.  Sure enough, it found four more such bugs, including two in a
 > recent commit where I had found the first two bugs --- and I had
 > missed the other two even though I was specifically looking for such
 > instances.  Oops.  :-)
 > 
 > Maybe we can add a debugging config option?  I think having static
 > checkers plus some kmalloc failure testing should be sufficient to
 > prevent these sorts of problem from showing up.
 > 
 > It would seem to me that this is the sort of thing that a static
 > checker should find reliably; Coverity has found things that were more
 > complex than what this should require, I think.  I don't know if they
 > would be willing to add something this kernel-specific, though.  (I've
 > added Dave Jones to the thread since he's been working a lot with
 > Coverity; Dave, what do you think?)

It *might* be possible to rig up something using their modelling 
functionality, but I've not managed to make that work to my ends in the past.

I suspect a runtime check would be more fruitful faster than they could
implement kernel specific checkers & roll them out.

	Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jiri Kosina]

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 <jkosina@suse.cz>
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 <jkosina@suse.cz>
---
 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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrey Ryabinin]

2014-09-10 18:24 GMT+04:00 Jiri Kosina <jkosina@suse.cz>:
> 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 <jkosina@suse.cz>
> 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.
>

This won't work cause CONFIG_DEBUG_SLAB  is only for CONFIG_SLAB=y

How about just VM_BUG_ON(IS_ERR(ptr)); ?


> Inspired by a9cfcd63e8d ("ext4: avoid trying to kfree an ERR_PTR pointer").
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  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, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Best regards,
Andrey Ryabinin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jiri Kosina]

On Wed, 10 Sep 2014, Andrey Ryabinin wrote:

> > 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 <jkosina@suse.cz>
> > 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.
> >
> 
> This won't work cause CONFIG_DEBUG_SLAB  is only for CONFIG_SLAB=y
> 
> How about just VM_BUG_ON(IS_ERR(ptr)); ?

VM_BUG_ON() makes very little sense to me, as we are going to oops anyway 
later, so it's a lose-lose situation.

VM_WARN_ON() + return seems like much more reasonable choice.

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Christoph Lameter]

On Wed, 10 Sep 2014, Jiri Kosina wrote:

> Still, I believe that kernel shouldn't be just ignoring kfree(ERR_PTR)
> happening. Would something like the below be more acceptable?

CONFIG_DEBUG_SLAB is the wrong debugging option since it is used for
object debugging. This kind of patch would need CONFIG_DEBUG_VM I think.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jiri Kosina]

On Wed, 10 Sep 2014, Theodore Ts'o wrote:

> I'd much rather depending on better testing and static checkers to fix 
> them, since kfree *is* a hot path.

BTW if we stretch this argument a little bit more, we should also kill the 
ZERO_OR_NULL_PTR() check from kfree() and make it callers responsibility 
to perform the checking only if applicable ... we are currently doing a 
lot of pointless checking in cases where caller would be able to guarantee 
that the pointer is going to be non-NULL.

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Dan Carpenter]

On Wed, Sep 10, 2014 at 04:26:46PM +0200, Jiri Kosina wrote:
> On Wed, 10 Sep 2014, Theodore Ts'o wrote:
> 
> > I'd much rather depending on better testing and static checkers to fix 
> > them, since kfree *is* a hot path.
> 
> BTW if we stretch this argument a little bit more, we should also kill the 
> ZERO_OR_NULL_PTR() check from kfree() and make it callers responsibility 
> to perform the checking only if applicable ... we are currently doing a 
> lot of pointless checking in cases where caller would be able to guarantee 
> that the pointer is going to be non-NULL.

What you're saying is that we should remove the ZERO_SIZE_PTR
completely.  ZERO_SIZE_PTR is a very useful idiom and also it's too late
to remove it because everything depends on it.

Returning ZERO_SIZE_PTR is not an error.  Callers shouldn't test for it.
It works like this:
1) User space says "copy zero items to somewhere."
2) The kernel says "here is a zero size pointer"
3) We do some stuff like:
	copy_from_user(zero_pointer, src, 0)
   or:
	for (i = 0; i < 0; i++)
4) The caller frees the ZERO_SIZE_PTR.
5) We return success.

If we get rid of it then we're start returning -ENOMEM all over the
place and that breaks userspace.  Or we introduce zero as a special case
for every kmalloc.

You would think there would be a lot of bugs with ZERO_SIZE_POINTERs
but they seem fairly rare to me.  There are some where we allocate a
zero length string and then put a NUL terminator at the end.

regards,
dan carpenter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Jiri Kosina]

On Wed, 10 Sep 2014, Dan Carpenter wrote:

> > BTW if we stretch this argument a little bit more, we should also kill the 
> > ZERO_OR_NULL_PTR() check from kfree() and make it callers responsibility 
> > to perform the checking only if applicable ... we are currently doing a 
> > lot of pointless checking in cases where caller would be able to guarantee 
> > that the pointer is going to be non-NULL.
> 
> What you're saying is that we should remove the ZERO_SIZE_PTR
> completely.  ZERO_SIZE_PTR is a very useful idiom and also it's too late
> to remove it because everything depends on it.

I was just argumenting that if we care about single additional test in 
this path, the ZERO_OR_NULL_PTR() should have never been added at the 
first place, and the responsibility for checking should have been kept at 
callers.

Too late for this now, yes.

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Dan Carpenter]

On Wed, Sep 10, 2014 at 05:28:11PM +0200, Jiri Kosina wrote:
> 
> Too late for this now, yes.

We could still introduce a __kfree_fast_path() which doesn't have
checking.

regards,
dan carpenter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Theodore Ts'o]

On Wed, Sep 10, 2014 at 06:53:56PM +0300, Dan Carpenter wrote:
> On Wed, Sep 10, 2014 at 05:28:11PM +0200, Jiri Kosina wrote:
> > 
> > Too late for this now, yes.
> 
> We could still introduce a __kfree_fast_path() which doesn't have
> checking.

Well, there certainly is precedence for that sort of thing.  There is
a bunch of code which uses __brelse(bh) instead of brelse(bh) when the
caller is sure that bh is a valid non-NULL pointer.

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Rasmus Villemoes]

On Wed, Sep 10 2014, Jiri Kosina <jkosina@suse.cz> wrote:

> On Wed, 10 Sep 2014, Dan Carpenter wrote:
>
>> > BTW if we stretch this argument a little bit more, we should also kill the 
>> > ZERO_OR_NULL_PTR() check from kfree() and make it callers responsibility 
>> > to perform the checking only if applicable ... we are currently doing a 
>> > lot of pointless checking in cases where caller would be able to guarantee 
>> > that the pointer is going to be non-NULL.
>> 
>> What you're saying is that we should remove the ZERO_SIZE_PTR
>> completely.  ZERO_SIZE_PTR is a very useful idiom and also it's too late
>> to remove it because everything depends on it.
>
> I was just argumenting that if we care about single additional test in 
> this path, the ZERO_OR_NULL_PTR() should have never been added at the 
> first place, and the responsibility for checking should have been kept at 
> callers.

I think it makes a lot of sense to have the domain of kfree() be exactly
the codomain of kmalloc() and friends. That is, what is acceptable to
pass to kfree() is exactly the set of values that might be returned from
kmalloc() et al. Those include NULL and the very useful unique
zero-sized "object" ZERO_SIZE_PTR, but does not include any ERR_PTR().

Having every caller of kfree() check for NULL would bloat the code size
considerably, and it seems that these checks are being actively removed.

Rasmus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Christoph Lameter]

On Wed, 10 Sep 2014, Jiri Kosina wrote:

> 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.

So we actually think that it is okay to pass an error pointer to kfree
and silently ignore that?

Are we thinking about just accepting any pointer in kfree and ignore
invalid ones?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]

On Tue, 09 Sep 2014 16:21:14 -0700, Andrew Morton said:
> On Tue, 9 Sep 2014 23:25:28 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> 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 already do a check for ZERO_SIZE_PTR, and given that dereferencing *that* is
instant death for the kernel, and we see it very rarely, I'm going to guess
that IS_ERR(ptr) *has* to be true more often than ZERO_SIZE_PTR, and thus even
more advantageous to short-circuit.

I guess it depends on a few things:

1) How many instances of 'if (!IS_ERR(foo)) kfree(foo);' are in the tree, and
what percent of kfree() calls executed have the guard on them

2) How many of the hot calls can/will get the guard removed.

3) How many cycles, if any, this adds to the path (a non-trivial question on
superscalar architectures), compared with doing a test before calling kfree()

I unfortunately have no earthly clue what the values of any of those
three quantities are....

[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]

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

[Dan Carpenter]

On Wed, Sep 10, 2014 at 01:15:15AM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Tue, 09 Sep 2014 16:21:14 -0700, Andrew Morton said:
> > On Tue, 9 Sep 2014 23:25:28 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> 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 already do a check for ZERO_SIZE_PTR, and given that dereferencing *that* is
> instant death for the kernel, and we see it very rarely, I'm going to guess
> that IS_ERR(ptr) *has* to be true more often than ZERO_SIZE_PTR, and thus even
> more advantageous to short-circuit.

ZERO_SIZE_PTR is sort of common.

ZERO_SIZE_PTR is an mm abstraction and kfree() and ksize() are basically
the only places where we need to test for it.  Also friends of kfree()
like jbd2_journal_free_transaction().

regards,
dan carpenter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Christoph Lameter]

On Tue, 9 Sep 2014, Andrew Morton wrote:

> >
> > -	if (unlikely(ZERO_OR_NULL_PTR(objp)))
> > +	if (unlikely(ZERO_OR_NULL_PTR(objp) || IS_ERR(objp)))
> >  		return;
>
> 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 could come up with a macro that does both. Basically if objp < 4086 or
so (signed comparison) then just return.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>