[RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Sergey Senozhatsky]

Hello,

RFC

Proposed by Andrew Morton: https://lkml.org/lkml/2015/6/8/583

The existing pools' destroy() functions do not allow NULL pool pointers;
instead, every destructor() caller forced to check if pool is not NULL,
which:
 a) requires additional attention from developers/reviewers
 b) may lead to a NULL pointer dereferences if (a) didn't work


First 3 patches tweak
- kmem_cache_destroy()
- mempool_destroy()
- dma_pool_destroy()

to handle NULL pointers.
Basically, this patch set will:

1) Can prevent us from still undiscovered NULL pointer dereferences.
 (like the one that was addressed in https://lkml.org/lkml/2015/6/5/262)

2) Make a cleanup possible. Things like:
 [..]
         if (xhci->segment_pool)
                 dma_pool_destroy(xhci->segment_pool);
 	..
         if (xhci->device_pool)
                 dma_pool_destroy(xhci->device_pool);
 	..
         if (xhci->small_streams_pool)
                 dma_pool_destroy(xhci->small_streams_pool);
 	..
         if (xhci->medium_streams_pool)
                 dma_pool_destroy(xhci->medium_streams_pool);
 [..]
 
 or
 
 [..]
 fail_dma_pool:
         if (IS_QLA82XX(ha) || ql2xenabledif) {
                 dma_pool_destroy(ha->fcp_cmnd_dma_pool);
                 ha->fcp_cmnd_dma_pool = NULL;
         }
 fail_dl_dma_pool:
         if (IS_QLA82XX(ha) || ql2xenabledif) {
                 dma_pool_destroy(ha->dl_dma_pool);
                 ha->dl_dma_pool = NULL;
         }
 fail_s_dma_pool:
         dma_pool_destroy(ha->s_dma_pool);
         ha->s_dma_pool = NULL;
 [..]

 may now be simplified.


0004 and 0005 are not so necessary, simply because there are not
so many users of these two (added for pool's destroy() functions consistency):
-- zpool_destroy_pool()
-- zs_destroy_pool()

So, 0004 and 0005 can be dropped.


- zbud does kfree() in zbud_destroy_pool(), so I didn't touch it.


Sergey Senozhatsky (5):
  mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()
  mm/mempool: allow NULL `pool' pointer in mempool_destroy()
  mm/dmapool: allow NULL `pool' pointer in dma_pool_destroy()
  mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()
  mm/zsmalloc: allow NULL `pool' pointer in zs_destroy_pool()

 mm/dmapool.c     | 3 +++
 mm/mempool.c     | 3 +++
 mm/slab_common.c | 3 +++
 mm/zpool.c       | 3 +++
 mm/zsmalloc.c    | 3 +++
 5 files changed, 15 insertions(+)

-- 
2.4.3.368.g7974889

[RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Sergey Senozhatsky]

kmem_cache_destroy() does not tolerate a NULL kmem_cache pointer
argument and performs a NULL-pointer dereference. This requires
additional attention and effort from developers/reviewers and
forces all kmem_cache_destroy() callers (200+ as of 4.1) to do
a NULL check

	if (cache)
		kmem_cache_destroy(cache);

Or, otherwise, be invalid kmem_cache_destroy() users.

Tweak kmem_cache_destroy() and NULL-check the pointer there.

Proposed by Andrew Morton.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
 mm/slab_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 8873985..ea69b13 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -641,6 +641,9 @@ void kmem_cache_destroy(struct kmem_cache *s)
 	bool need_rcu_barrier = false;
 	bool busy = false;
 
+	if (unlikely(!s))
+		return;
+
 	BUG_ON(!is_root_cache(s));
 
 	get_online_cpus();
-- 
2.4.3.368.g7974889

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[David Rientjes]

On Tue, 9 Jun 2015, Sergey Senozhatsky wrote:

> kmem_cache_destroy() does not tolerate a NULL kmem_cache pointer
> argument and performs a NULL-pointer dereference. This requires
> additional attention and effort from developers/reviewers and
> forces all kmem_cache_destroy() callers (200+ as of 4.1) to do
> a NULL check
> 
> 	if (cache)
> 		kmem_cache_destroy(cache);
> 
> Or, otherwise, be invalid kmem_cache_destroy() users.
> 
> Tweak kmem_cache_destroy() and NULL-check the pointer there.
> 
> Proposed by Andrew Morton.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by: David Rientjes <rientjes@google.com>

kmem_cache_destroy() isn't a fastpath, this is long overdue.  Now where's 
the patch to remove the NULL checks from the callers? ;)

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Sergey Senozhatsky]

On (06/17/15 16:14), David Rientjes wrote:
[..]
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> 
> Acked-by: David Rientjes <rientjes@google.com>
> 
> kmem_cache_destroy() isn't a fastpath, this is long overdue.  Now where's 
> the patch to remove the NULL checks from the callers? ;)
> 

Thanks.

Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.

	-ss

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Julia Lawall]

On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:

> On (06/17/15 16:14), David Rientjes wrote:
> [..]
> > >
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> >
> > Acked-by: David Rientjes <rientjes@google.com>
> >
> > kmem_cache_destroy() isn't a fastpath, this is long overdue.  Now where's
> > the patch to remove the NULL checks from the callers? ;)
> >
>
> Thanks.
>
> Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.

I'll refresh it and send it shortly.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Sergey Senozhatsky]

On (06/19/15 08:50), Julia Lawall wrote:
> On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:
> 
> > On (06/17/15 16:14), David Rientjes wrote:
> > [..]
> > > >
> > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > >
> > > Acked-by: David Rientjes <rientjes@google.com>
> > >
> > > kmem_cache_destroy() isn't a fastpath, this is long overdue.  Now where's
> > > the patch to remove the NULL checks from the callers? ;)
> > >
> >
> > Thanks.
> >
> > Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
> 
> I'll refresh it and send it shortly.
> 

I'll re-up this thread.

Julia, do you want to wait until these 3 patches will be merged to
Linus's tree (just to be on a safe side, so someone's tree (out of sync
with linux-next) will not go crazy)?

	-ss

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Julia Lawall]

On Thu, 6 Aug 2015, Sergey Senozhatsky wrote:

> On (06/19/15 08:50), Julia Lawall wrote:
> > On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:
> >
> > > On (06/17/15 16:14), David Rientjes wrote:
> > > [..]
> > > > >
> > > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > > >
> > > > Acked-by: David Rientjes <rientjes@google.com>
> > > >
> > > > kmem_cache_destroy() isn't a fastpath, this is long overdue.  Now where's
> > > > the patch to remove the NULL checks from the callers? ;)
> > > >
> > >
> > > Thanks.
> > >
> > > Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
> >
> > I'll refresh it and send it shortly.
> >
>
> I'll re-up this thread.
>
> Julia, do you want to wait until these 3 patches will be merged to
> Linus's tree (just to be on a safe side, so someone's tree (out of sync
> with linux-next) will not go crazy)?

I think it would be safer.  Code may crash if the test is removed before
the function can tolerate it.

julia

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Sergey Senozhatsky]

On (08/06/15 16:27), Julia Lawall wrote:
[..]
> > Julia, do you want to wait until these 3 patches will be merged to
> > Linus's tree (just to be on a safe side, so someone's tree (out of sync
> > with linux-next) will not go crazy)?
> 
> I think it would be safer.  Code may crash if the test is removed before
> the function can tolerate it.
> 

Agree. I'll re-up this thread later (once 4.3 merge window is closed).
Thank you.

	-ss

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Julia Lawall]

On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:

> On (06/17/15 16:14), David Rientjes wrote:
> [..]
> > > 
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > 
> > Acked-by: David Rientjes <rientjes@google.com>
> > 
> > kmem_cache_destroy() isn't a fastpath, this is long overdue.  Now where's 
> > the patch to remove the NULL checks from the callers? ;)
> > 
> 
> Thanks.
> 
> Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.

The patch for making these functions able to tolerate NULL doesn't seem to 
be in linux-next yet, so I will wait until it appears.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 1/5] mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()

[Sergey Senozhatsky]

On (06/20/15 18:25), Julia Lawall wrote:
> > On (06/17/15 16:14), David Rientjes wrote:
> > [..]
> > > > 
> > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > > 
> > > Acked-by: David Rientjes <rientjes@google.com>
> > > 
> > > kmem_cache_destroy() isn't a fastpath, this is long overdue.  Now where's 
> > > the patch to remove the NULL checks from the callers? ;)
> > > 
> > 
> > Thanks.
> > 
> > Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
> 
> The patch for making these functions able to tolerate NULL doesn't seem to 
> be in linux-next yet, so I will wait until it appears.

Hello Julia,

The patches are in -next now.

mm/dmapool: 8bf49946ed8fa01a0b5e7d0de94655c072525344
mm/mempool: eb54bc8469e2977bcef4e284d24cbf3578ce9cd9
mm/slab_common: e88672f95907c14cf8ab2cce592c41bbb9cefc5f

	-ss

[RFC][PATCH 2/5] mm/mempool: allow NULL `pool' pointer in mempool_destroy()

[Sergey Senozhatsky]

mempool_destroy() does not tolerate a NULL mempool_t pointer
argument and performs a NULL-pointer dereference. This requires
additional attention and effort from developers/reviewers and
forces all mempool_destroy() callers to do a NULL check

	if (pool)
		mempool_destroy(pool);

Or, otherwise, be invalid mempool_destroy() users.

Tweak mempool_destroy() and NULL-check the pointer there.

Proposed by Andrew Morton.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
 mm/mempool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/mempool.c b/mm/mempool.c
index 2cc08de..4c533bc 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -150,6 +150,9 @@ static void *remove_element(mempool_t *pool)
  */
 void mempool_destroy(mempool_t *pool)
 {
+	if (unlikely(!pool))
+		return;
+
 	while (pool->curr_nr) {
 		void *element = remove_element(pool);
 		pool->free(element, pool->pool_data);
-- 
2.4.3.368.g7974889

Re: [RFC][PATCH 2/5] mm/mempool: allow NULL `pool' pointer in mempool_destroy()

[David Rientjes]

On Tue, 9 Jun 2015, Sergey Senozhatsky wrote:

> mempool_destroy() does not tolerate a NULL mempool_t pointer
> argument and performs a NULL-pointer dereference. This requires
> additional attention and effort from developers/reviewers and
> forces all mempool_destroy() callers to do a NULL check
> 
> 	if (pool)
> 		mempool_destroy(pool);
> 
> Or, otherwise, be invalid mempool_destroy() users.
> 
> Tweak mempool_destroy() and NULL-check the pointer there.
> 
> Proposed by Andrew Morton.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by: David Rientjes <rientjes@google.com>

I like how your patch series is enabling us to remove many lines from the 
source code.  But doing s/Reported-by/Suggested-by/ can also make your 
changelog two lines shorter ;)

Re: [RFC][PATCH 2/5] mm/mempool: allow NULL `pool' pointer in mempool_destroy()

[Sergey Senozhatsky]

On (06/17/15 16:21), David Rientjes wrote:
[..]
> > Tweak mempool_destroy() and NULL-check the pointer there.
> > 
> > Proposed by Andrew Morton.
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> 
> Acked-by: David Rientjes <rientjes@google.com>
> 
> I like how your patch series is enabling us to remove many lines from the 
> source code.  But doing s/Reported-by/Suggested-by/ can also make your 
> changelog two lines shorter ;)
> 

Thanks.

Oh, s/Reported-by/Suggested-by/ looks better, indeed.

	-ss

[RFC][PATCH 3/5] mm/dmapool: allow NULL `pool' pointer in dma_pool_destroy()

[Sergey Senozhatsky]

dma_pool_destroy() does not tolerate a NULL dma_pool pointer
argument and performs a NULL-pointer dereference. This requires
additional attention and effort from developers/reviewers and
forces all dma_pool_destroy() callers to do a NULL check

	if (pool)
		dma_pool_destroy(pool);

Or, otherwise, be invalid dma_pool_destroy() users.

Tweak dma_pool_destroy() and NULL-check the pointer there.

Proposed by Andrew Morton.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
 mm/dmapool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index fd5fe43..5f2cffc 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -271,6 +271,9 @@ void dma_pool_destroy(struct dma_pool *pool)
 {
 	bool empty = false;
 
+	if (unlikely(!pool))
+		return;
+
 	mutex_lock(&pools_reg_lock);
 	mutex_lock(&pools_lock);
 	list_del(&pool->pools);
-- 
2.4.3.368.g7974889

Re: [RFC][PATCH 3/5] mm/dmapool: allow NULL `pool' pointer in dma_pool_destroy()

[David Rientjes]

On Tue, 9 Jun 2015, Sergey Senozhatsky wrote:

> dma_pool_destroy() does not tolerate a NULL dma_pool pointer
> argument and performs a NULL-pointer dereference. This requires
> additional attention and effort from developers/reviewers and
> forces all dma_pool_destroy() callers to do a NULL check
> 
> 	if (pool)
> 		dma_pool_destroy(pool);
> 
> Or, otherwise, be invalid dma_pool_destroy() users.
> 
> Tweak dma_pool_destroy() and NULL-check the pointer there.
> 
> Proposed by Andrew Morton.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by: David Rientjes <rientjes@google.com>

[RFC][PATCH 4/5] mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()

[Sergey Senozhatsky]

zpool_destroy_pool() does not tolerate a NULL zpool pointer
argument and performs a NULL-pointer dereference. Although
there is only one zpool_destroy_pool() user (as of 4.1),
still update it to be coherent with the corresponding
destroy() functions of the remainig pool-allocators (slab,
mempool, etc.), which now allow NULL pool-pointers.

For consistency, tweak zpool_destroy_pool() and NULL-check the
pointer there.

Proposed by Andrew Morton.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
 mm/zpool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/zpool.c b/mm/zpool.c
index bacdab6..2f59b90 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -202,6 +202,9 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
  */
 void zpool_destroy_pool(struct zpool *zpool)
 {
+	if (unlikely(!zpool))
+		return;
+
 	pr_info("destroying pool type %s\n", zpool->type);
 
 	spin_lock(&pools_lock);
-- 
2.4.3.368.g7974889

Re: [RFC][PATCH 4/5] mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()

[Dan Streetman]

On Tue, Jun 9, 2015 at 8:04 AM, Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
> zpool_destroy_pool() does not tolerate a NULL zpool pointer
> argument and performs a NULL-pointer dereference. Although
> there is only one zpool_destroy_pool() user (as of 4.1),
> still update it to be coherent with the corresponding
> destroy() functions of the remainig pool-allocators (slab,
> mempool, etc.), which now allow NULL pool-pointers.
>
> For consistency, tweak zpool_destroy_pool() and NULL-check the
> pointer there.
>
> Proposed by Andrew Morton.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reported-by: Andrew Morton <akpm@linux-foundation.org>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by: Dan Streetman <ddstreet@ieee.org>

> ---
>  mm/zpool.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/zpool.c b/mm/zpool.c
> index bacdab6..2f59b90 100644
> --- a/mm/zpool.c
> +++ b/mm/zpool.c
> @@ -202,6 +202,9 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
>   */
>  void zpool_destroy_pool(struct zpool *zpool)
>  {
> +       if (unlikely(!zpool))
> +               return;
> +
>         pr_info("destroying pool type %s\n", zpool->type);
>
>         spin_lock(&pools_lock);
> --
> 2.4.3.368.g7974889
>
> --
> 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: [RFC][PATCH 4/5] mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()

[Sergey Senozhatsky]

On (06/10/15 16:59), Dan Streetman wrote:
> On Tue, Jun 9, 2015 at 8:04 AM, Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com> wrote:
> > zpool_destroy_pool() does not tolerate a NULL zpool pointer
> > argument and performs a NULL-pointer dereference. Although
> > there is only one zpool_destroy_pool() user (as of 4.1),
> > still update it to be coherent with the corresponding
> > destroy() functions of the remainig pool-allocators (slab,
> > mempool, etc.), which now allow NULL pool-pointers.
> >
> > For consistency, tweak zpool_destroy_pool() and NULL-check the
> > pointer there.
> >
> > Proposed by Andrew Morton.
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> 
> Acked-by: Dan Streetman <ddstreet@ieee.org>

Thanks.

Shall we ask Joe to add zpool_destroy_pool() to the
"$func(NULL) is safe and this check is probably not required" list?

	-ss

> > ---
> >  mm/zpool.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/zpool.c b/mm/zpool.c
> > index bacdab6..2f59b90 100644
> > --- a/mm/zpool.c
> > +++ b/mm/zpool.c
> > @@ -202,6 +202,9 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
> >   */
> >  void zpool_destroy_pool(struct zpool *zpool)
> >  {
> > +       if (unlikely(!zpool))
> > +               return;
> > +
> >         pr_info("destroying pool type %s\n", zpool->type);
> >
> >         spin_lock(&pools_lock);
> > --
> > 2.4.3.368.g7974889
> >
> > --
> > 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: [RFC][PATCH 4/5] mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()

[Joe Perches]

On Thu, 2015-06-11 at 08:58 +0900, Sergey Senozhatsky wrote:
> On (06/10/15 16:59), Dan Streetman wrote:
> > On Tue, Jun 9, 2015 at 8:04 AM, Sergey Senozhatsky
> > <sergey.senozhatsky@gmail.com> wrote:
> > > zpool_destroy_pool() does not tolerate a NULL zpool pointer
> > > argument and performs a NULL-pointer dereference. Although
> > > there is only one zpool_destroy_pool() user (as of 4.1),
> > > still update it to be coherent with the corresponding
> > > destroy() functions of the remainig pool-allocators (slab,
> > > mempool, etc.), which now allow NULL pool-pointers.
> > >
> > > For consistency, tweak zpool_destroy_pool() and NULL-check the
> > > pointer there.
> > >
> > > Proposed by Andrew Morton.
> > >
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > 
> > Acked-by: Dan Streetman <ddstreet@ieee.org>
> 
> Thanks.
> 
> Shall we ask Joe to add zpool_destroy_pool() to the
> "$func(NULL) is safe and this check is probably not required" list?

[]

Is it really worth it?

There isn't any use of zpool_destroy_pool preceded by an if
There is one and only one use of zpool_destroy_pool.

Re: [RFC][PATCH 4/5] mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()

[Sergey Senozhatsky]

On (06/10/15 17:48), Joe Perches wrote:
[..]
> > > > For consistency, tweak zpool_destroy_pool() and NULL-check the
> > > > pointer there.
> > > >
> > > > Proposed by Andrew Morton.
> > > >
> > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > > 
> > > Acked-by: Dan Streetman <ddstreet@ieee.org>
> > 
> > Thanks.
> > 
> > Shall we ask Joe to add zpool_destroy_pool() to the
> > "$func(NULL) is safe and this check is probably not required" list?
> 
> []
> 
> Is it really worth it?
> 
> There isn't any use of zpool_destroy_pool preceded by an if
> There is one and only one use of zpool_destroy_pool.
> 

Yes, that's why I asked. I don't think that zpool_destroy_pool()
will gain any significant amount of users soon (well, who knows),
so I'm fine with keeping it out of checkpatch checks. Just checked
your opinion.

	-ss

Re: [RFC][PATCH 4/5] mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()

[Dan Streetman]

On Wed, Jun 10, 2015 at 8:59 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> On (06/10/15 17:48), Joe Perches wrote:
> [..]
>> > > > For consistency, tweak zpool_destroy_pool() and NULL-check the
>> > > > pointer there.
>> > > >
>> > > > Proposed by Andrew Morton.
>> > > >
>> > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>> > > > Reported-by: Andrew Morton <akpm@linux-foundation.org>
>> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
>> > >
>> > > Acked-by: Dan Streetman <ddstreet@ieee.org>
>> >
>> > Thanks.
>> >
>> > Shall we ask Joe to add zpool_destroy_pool() to the
>> > "$func(NULL) is safe and this check is probably not required" list?
>>
>> []
>>
>> Is it really worth it?
>>
>> There isn't any use of zpool_destroy_pool preceded by an if
>> There is one and only one use of zpool_destroy_pool.
>>
>
> Yes, that's why I asked. I don't think that zpool_destroy_pool()
> will gain any significant amount of users soon (well, who knows),
> so I'm fine with keeping it out of checkpatch checks. Just checked
> your opinion.

I really doubt if zpool will be used by anyone other than zswap anytime soon.

>
>         -ss

[RFC][PATCH 5/5] mm/zsmalloc: allow NULL `pool' pointer in zs_destroy_pool()

[Sergey Senozhatsky]

zpool_destroy_pool() does not tolerate a NULL zs_pool pointer
argument and performs a NULL-pointer dereference. Although
there are quite a few zs_destroy_pool() users, still update
it to be coherent with the corresponding destroy() functions
of the remainig pool-allocators (slab, mempool, etc.), which
now allow NULL pool-pointers.

For consistency, tweak zpool_destroy_pool() and NULL-check the
pointer there.

Proposed by Andrew Morton.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
 mm/zsmalloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index c766240..80964d2 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1868,6 +1868,9 @@ void zs_destroy_pool(struct zs_pool *pool)
 {
 	int i;
 
+	if (unlikely(!pool))
+		return;
+
 	zs_pool_stat_destroy(pool);
 
 	for (i = 0; i < zs_size_classes; i++) {
-- 
2.4.3.368.g7974889

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Andrew Morton]

On Tue,  9 Jun 2015 21:04:48 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> The existing pools' destroy() functions do not allow NULL pool pointers;
> instead, every destructor() caller forced to check if pool is not NULL,
> which:
>  a) requires additional attention from developers/reviewers
>  b) may lead to a NULL pointer dereferences if (a) didn't work
> 
> 
> First 3 patches tweak
> - kmem_cache_destroy()
> - mempool_destroy()
> - dma_pool_destroy()
> 
> to handle NULL pointers.

Well I like it, even though it's going to cause a zillion little cleanup
patches.

checkpatch already has a "kfree(NULL) is safe and this check is
probably not required" test so I guess Joe will need to get busy ;)

I'll park these patches until after 4.1 is released - it's getting to
that time...

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Joe Perches]

On Tue, 2015-06-09 at 14:25 -0700, Andrew Morton wrote:
> On Tue,  9 Jun 2015 21:04:48 +0900 Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:
> 
> > The existing pools' destroy() functions do not allow NULL pool pointers;
> > instead, every destructor() caller forced to check if pool is not NULL,
> > which:
> >  a) requires additional attention from developers/reviewers
> >  b) may lead to a NULL pointer dereferences if (a) didn't work
> > 
> > 
> > First 3 patches tweak
> > - kmem_cache_destroy()
> > - mempool_destroy()
> > - dma_pool_destroy()
> > 
> > to handle NULL pointers.
> 
> Well I like it, even though it's going to cause a zillion little cleanup
> patches.
> 
> checkpatch already has a "kfree(NULL) is safe and this check is
> probably not required" test so I guess Joe will need to get busy ;)

Maybe it'll be Julia's crew.

The checkpatch change is pretty trivial
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 69c4716..3d6e34d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4801,7 +4801,7 @@ sub process {
 # check for needless "if (<foo>) fn(<foo>)" uses
 		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
 			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
-			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
 				WARN('NEEDLESS_IF',
 				     "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
 			}

[PATCH] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Joe Perches]

Sergey Senozhatsky has modified several destroy functions that can
now be called with NULL values.

 - kmem_cache_destroy()
 - mempool_destroy()
 - dma_pool_destroy()

Update checkpatch to warn when those functions are preceded by an if.

Update checkpatch to --fix all the calls too only when the code style
form is using leading tabs.

from:
	if (foo)
		<func>(foo);
to:
	<func>(foo);

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 69c4716..2eff013 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4800,10 +4800,37 @@ sub process {
 
 # check for needless "if (<foo>) fn(<foo>)" uses
 		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
-			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
-			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
-				WARN('NEEDLESS_IF',
-				     "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
+			my $tested = quotemeta($1);
+			my $expr = '\s*\(\s*' . quotemeta($tested) . '\s*\)\s*;';
+			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
+				my $func = $1;
+				if (WARN('NEEDLESS_IF',
+					 "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
+				    $fix) {
+					my $do_fix = 1;
+					my $leading_tabs = "";
+					my $new_leading_tabs = "";
+					if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
+						$leading_tabs = $1;
+					} else {
+						$do_fix = 0;
+						print("here1: <$lines[$linenr - 2]>\n")
+					}
+					if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
+						$new_leading_tabs = $1;
+						if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
+							$do_fix = 0;
+							print("here2: <$lines[$linenr - 1]>\n")
+						}
+					} else {
+						$do_fix = 0;
+						print("here3: <$lines[$linenr - 1]>\n")
+					}
+					if ($do_fix) {
+						fix_delete_line($fixlinenr - 1, $prevrawline);
+						$fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
+					}
+				}
 			}
 		}
 

[PATCH V2] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Joe Perches]

Sergey Senozhatsky has modified several destroy functions that can
now be called with NULL values.

 - kmem_cache_destroy()
 - mempool_destroy()
 - dma_pool_destroy()

Update checkpatch to warn when those functions are preceded by an if.

Update checkpatch to --fix all the calls too only when the code style
form is using leading tabs.

from:
	if (foo)
		<func>(foo);
to:
	<func>(foo);

Signed-off-by: Joe Perches <joe@perches.com>
---
V2: Remove useless debugging print messages and multiple quotemetas

 scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 69c4716..87d3bf1aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4800,10 +4800,34 @@ sub process {
 
 # check for needless "if (<foo>) fn(<foo>)" uses
 		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
-			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
-			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
-				WARN('NEEDLESS_IF',
-				     "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
+			my $tested = quotemeta($1);
+			my $expr = '\s*\(\s*' . $tested . '\s*\)\s*;';
+			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
+				my $func = $1;
+				if (WARN('NEEDLESS_IF',
+					 "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
+				    $fix) {
+					my $do_fix = 1;
+					my $leading_tabs = "";
+					my $new_leading_tabs = "";
+					if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
+						$leading_tabs = $1;
+					} else {
+						$do_fix = 0;
+					}
+					if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
+						$new_leading_tabs = $1;
+						if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
+							$do_fix = 0;
+						}
+					} else {
+						$do_fix = 0;
+					}
+					if ($do_fix) {
+						fix_delete_line($fixlinenr - 1, $prevrawline);
+						$fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
+					}
+				}
 			}
 		}
 

Re: [PATCH V2] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Sergey Senozhatsky]

On (06/09/15 22:52), Joe Perches wrote:
> Sergey Senozhatsky has modified several destroy functions that can
> now be called with NULL values.
> 
>  - kmem_cache_destroy()
>  - mempool_destroy()
>  - dma_pool_destroy()
> 
> Update checkpatch to warn when those functions are preceded by an if.
> 
> Update checkpatch to --fix all the calls too only when the code style
> form is using leading tabs.
> 
> from:
> 	if (foo)
> 		<func>(foo);
> to:
> 	<func>(foo);
> 
> Signed-off-by: Joe Perches <joe@perches.com>

nice.

works fine to me. you can add
Tested-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

if needed.

	-ss

> ---
> V2: Remove useless debugging print messages and multiple quotemetas
> 
>  scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 69c4716..87d3bf1aa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4800,10 +4800,34 @@ sub process {
>  
>  # check for needless "if (<foo>) fn(<foo>)" uses
>  		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> -			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> -			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> -				WARN('NEEDLESS_IF',
> -				     "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
> +			my $tested = quotemeta($1);
> +			my $expr = '\s*\(\s*' . $tested . '\s*\)\s*;';
> +			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
> +				my $func = $1;
> +				if (WARN('NEEDLESS_IF',
> +					 "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
> +				    $fix) {
> +					my $do_fix = 1;
> +					my $leading_tabs = "";
> +					my $new_leading_tabs = "";
> +					if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
> +						$leading_tabs = $1;
> +					} else {
> +						$do_fix = 0;
> +					}
> +					if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
> +						$new_leading_tabs = $1;
> +						if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
> +							$do_fix = 0;
> +						}
> +					} else {
> +						$do_fix = 0;
> +					}
> +					if ($do_fix) {
> +						fix_delete_line($fixlinenr - 1, $prevrawline);
> +						$fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
> +					}
> +				}
>  			}
>  		}
>  
> 
> 

Re: [PATCH V2] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Julia Lawall]

On Tue, 9 Jun 2015, Joe Perches wrote:

> Sergey Senozhatsky has modified several destroy functions that can
> now be called with NULL values.
>
>  - kmem_cache_destroy()
>  - mempool_destroy()
>  - dma_pool_destroy()

I don't actually see any null test in the definition of dma_pool_destroy,
in the linux-next 54896f27dd5 (20150610).  So I guess it would be
premature to send patches to remove the null tests.

julia

> Update checkpatch to warn when those functions are preceded by an if.
>
> Update checkpatch to --fix all the calls too only when the code style
> form is using leading tabs.
>
> from:
> 	if (foo)
> 		<func>(foo);
> to:
> 	<func>(foo);
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> V2: Remove useless debugging print messages and multiple quotemetas
>
>  scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 69c4716..87d3bf1aa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4800,10 +4800,34 @@ sub process {
>
>  # check for needless "if (<foo>) fn(<foo>)" uses
>  		if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> -			my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> -			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> -				WARN('NEEDLESS_IF',
> -				     "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
> +			my $tested = quotemeta($1);
> +			my $expr = '\s*\(\s*' . $tested . '\s*\)\s*;';
> +			if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
> +				my $func = $1;
> +				if (WARN('NEEDLESS_IF',
> +					 "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
> +				    $fix) {
> +					my $do_fix = 1;
> +					my $leading_tabs = "";
> +					my $new_leading_tabs = "";
> +					if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
> +						$leading_tabs = $1;
> +					} else {
> +						$do_fix = 0;
> +					}
> +					if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
> +						$new_leading_tabs = $1;
> +						if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
> +							$do_fix = 0;
> +						}
> +					} else {
> +						$do_fix = 0;
> +					}
> +					if ($do_fix) {
> +						fix_delete_line($fixlinenr - 1, $prevrawline);
> +						$fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
> +					}
> +				}
>  			}
>  		}
>
>
>
>

Re: [PATCH V2] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Sergey Senozhatsky]

On (06/11/15 11:41), Julia Lawall wrote:
> On Tue, 9 Jun 2015, Joe Perches wrote:
> 
> > Sergey Senozhatsky has modified several destroy functions that can
> > now be called with NULL values.
> >
> >  - kmem_cache_destroy()
> >  - mempool_destroy()
> >  - dma_pool_destroy()
> 
> I don't actually see any null test in the definition of dma_pool_destroy,
> in the linux-next 54896f27dd5 (20150610).  So I guess it would be
> premature to send patches to remove the null tests.
> 

yes,

Andrew Morton:
: I'll park these patches until after 4.1 is released - it's getting to
: that time...


	-ss

Re: [PATCH V2] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Julia Lawall]

On Thu, 11 Jun 2015, Sergey Senozhatsky wrote:

> On (06/11/15 11:41), Julia Lawall wrote:
> > On Tue, 9 Jun 2015, Joe Perches wrote:
> >
> > > Sergey Senozhatsky has modified several destroy functions that can
> > > now be called with NULL values.
> > >
> > >  - kmem_cache_destroy()
> > >  - mempool_destroy()
> > >  - dma_pool_destroy()
> >
> > I don't actually see any null test in the definition of dma_pool_destroy,
> > in the linux-next 54896f27dd5 (20150610).  So I guess it would be
> > premature to send patches to remove the null tests.
> >
>
> yes,
>
> Andrew Morton:
> : I'll park these patches until after 4.1 is released - it's getting to
> : that time...

OK, thanks,

julia

Re: [PATCH V2] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Andrew Morton]

On Tue, 09 Jun 2015 22:52:29 -0700 Joe Perches <joe@perches.com> wrote:

> Sergey Senozhatsky has modified several destroy functions that can
> now be called with NULL values.
> 
>  - kmem_cache_destroy()
>  - mempool_destroy()
>  - dma_pool_destroy()
> 
> Update checkpatch to warn when those functions are preceded by an if.
> 
> Update checkpatch to --fix all the calls too only when the code style
> form is using leading tabs.
> 
> from:
> 	if (foo)
> 		<func>(foo);
> to:
> 	<func>(foo);

There's also zpool_destroy_pool() and zs_destroy_pool().  Did we decide
they're not worth bothering about?

Re: [PATCH V2] checkpatch: Add some <foo>_destroy functions to NEEDLESS_IF tests

[Sergey Senozhatsky]

On (07/14/15 16:03), Andrew Morton wrote:
> > Sergey Senozhatsky has modified several destroy functions that can
> > now be called with NULL values.
> > 
> >  - kmem_cache_destroy()
> >  - mempool_destroy()
> >  - dma_pool_destroy()
> > 
> > Update checkpatch to warn when those functions are preceded by an if.
> > 
> > Update checkpatch to --fix all the calls too only when the code style
> > form is using leading tabs.
> > 
> > from:
> > 	if (foo)
> > 		<func>(foo);
> > to:
> > 	<func>(foo);
> 
> There's also zpool_destroy_pool() and zs_destroy_pool().  Did we decide
> they're not worth bothering about?

Correct. Those two are very unlikely will see any significant number
of users so, I think, we can drop the patches that touch zspool and
zsmalloc destructors.

	-ss

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Julia Lawall]

> > Well I like it, even though it's going to cause a zillion little cleanup
> > patches.

Actually only at most 87.  There are some functions that look quite a bit 
nicer with the change, like:

 void jffs2_destroy_slab_caches(void)
 {
-       if(full_dnode_slab)
-               kmem_cache_destroy(full_dnode_slab);
-       if(raw_dirent_slab)
-               kmem_cache_destroy(raw_dirent_slab);
-       if(raw_inode_slab)
-               kmem_cache_destroy(raw_inode_slab);
-       if(tmp_dnode_info_slab)
-               kmem_cache_destroy(tmp_dnode_info_slab);
-       if(raw_node_ref_slab)
-               kmem_cache_destroy(raw_node_ref_slab);
-       if(node_frag_slab)
-               kmem_cache_destroy(node_frag_slab);
-       if(inode_cache_slab)
-               kmem_cache_destroy(inode_cache_slab);
+       kmem_cache_destroy(full_dnode_slab);
+       kmem_cache_destroy(raw_dirent_slab);
+       kmem_cache_destroy(raw_inode_slab);
+       kmem_cache_destroy(tmp_dnode_info_slab);
+       kmem_cache_destroy(raw_node_ref_slab);
+       kmem_cache_destroy(node_frag_slab);
+       kmem_cache_destroy(inode_cache_slab);
 #ifdef CONFIG_JFFS2_FS_XATTR
-       if (xattr_datum_cache)
-               kmem_cache_destroy(xattr_datum_cache);
-       if (xattr_ref_cache)
-               kmem_cache_destroy(xattr_ref_cache);
+       kmem_cache_destroy(xattr_datum_cache);
+       kmem_cache_destroy(xattr_ref_cache);
 #endif
 }

I can try to check and submit the patches.

julia

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Sergey Senozhatsky]

On (06/10/15 07:46), Julia Lawall wrote:
> > > Well I like it, even though it's going to cause a zillion little cleanup
> > > patches.
> 
> Actually only at most 87.  There are some functions that look quite a bit 
> nicer with the change, like:
> 
>  void jffs2_destroy_slab_caches(void)
>  {
> -       if(full_dnode_slab)
> -               kmem_cache_destroy(full_dnode_slab);
> -       if(raw_dirent_slab)
> -               kmem_cache_destroy(raw_dirent_slab);
> -       if(raw_inode_slab)
> -               kmem_cache_destroy(raw_inode_slab);
> -       if(tmp_dnode_info_slab)
> -               kmem_cache_destroy(tmp_dnode_info_slab);
> -       if(raw_node_ref_slab)
> -               kmem_cache_destroy(raw_node_ref_slab);
> -       if(node_frag_slab)
> -               kmem_cache_destroy(node_frag_slab);
> -       if(inode_cache_slab)
> -               kmem_cache_destroy(inode_cache_slab);
> +       kmem_cache_destroy(full_dnode_slab);
> +       kmem_cache_destroy(raw_dirent_slab);
> +       kmem_cache_destroy(raw_inode_slab);
> +       kmem_cache_destroy(tmp_dnode_info_slab);
> +       kmem_cache_destroy(raw_node_ref_slab);
> +       kmem_cache_destroy(node_frag_slab);
> +       kmem_cache_destroy(inode_cache_slab);
>  #ifdef CONFIG_JFFS2_FS_XATTR
> -       if (xattr_datum_cache)
> -               kmem_cache_destroy(xattr_datum_cache);
> -       if (xattr_ref_cache)
> -               kmem_cache_destroy(xattr_ref_cache);
> +       kmem_cache_destroy(xattr_datum_cache);
> +       kmem_cache_destroy(xattr_ref_cache);
>  #endif
>  }
> 

and some goto labels can go away either. like

[..]

err_percpu_counter_init:
	kmem_cache_destroy(sctp_chunk_cachep);
err_chunk_cachep:
	kmem_cache_destroy(sctp_bucket_cachep);

[..]

and others.

	-ss

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Julia Lawall]

On Wed, 10 Jun 2015, Sergey Senozhatsky wrote:

> On (06/10/15 07:46), Julia Lawall wrote:
> > > > Well I like it, even though it's going to cause a zillion little cleanup
> > > > patches.
> > 
> > Actually only at most 87.  There are some functions that look quite a bit 
> > nicer with the change, like:
> > 
> >  void jffs2_destroy_slab_caches(void)
> >  {
> > -       if(full_dnode_slab)
> > -               kmem_cache_destroy(full_dnode_slab);
> > -       if(raw_dirent_slab)
> > -               kmem_cache_destroy(raw_dirent_slab);
> > -       if(raw_inode_slab)
> > -               kmem_cache_destroy(raw_inode_slab);
> > -       if(tmp_dnode_info_slab)
> > -               kmem_cache_destroy(tmp_dnode_info_slab);
> > -       if(raw_node_ref_slab)
> > -               kmem_cache_destroy(raw_node_ref_slab);
> > -       if(node_frag_slab)
> > -               kmem_cache_destroy(node_frag_slab);
> > -       if(inode_cache_slab)
> > -               kmem_cache_destroy(inode_cache_slab);
> > +       kmem_cache_destroy(full_dnode_slab);
> > +       kmem_cache_destroy(raw_dirent_slab);
> > +       kmem_cache_destroy(raw_inode_slab);
> > +       kmem_cache_destroy(tmp_dnode_info_slab);
> > +       kmem_cache_destroy(raw_node_ref_slab);
> > +       kmem_cache_destroy(node_frag_slab);
> > +       kmem_cache_destroy(inode_cache_slab);
> >  #ifdef CONFIG_JFFS2_FS_XATTR
> > -       if (xattr_datum_cache)
> > -               kmem_cache_destroy(xattr_datum_cache);
> > -       if (xattr_ref_cache)
> > -               kmem_cache_destroy(xattr_ref_cache);
> > +       kmem_cache_destroy(xattr_datum_cache);
> > +       kmem_cache_destroy(xattr_ref_cache);
> >  #endif
> >  }
> > 
> 
> and some goto labels can go away either. like
> 
> [..]
> 
> err_percpu_counter_init:
> 	kmem_cache_destroy(sctp_chunk_cachep);
> err_chunk_cachep:
> 	kmem_cache_destroy(sctp_bucket_cachep);
> 
> [..]
> 
> and others.

This I find much less appealing.  The labels make clear what is needed at 
what point.  At least from a tool point of view, this is useful 
infomation.

julia

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Sergey Senozhatsky]

On (06/10/15 08:44), Julia Lawall wrote:
> > 
> > [..]
> > 
> > err_percpu_counter_init:
> > 	kmem_cache_destroy(sctp_chunk_cachep);
> > err_chunk_cachep:
> > 	kmem_cache_destroy(sctp_bucket_cachep);
> > 
> > [..]
> > 
> > and others.
> 
> This I find much less appealing.  The labels make clear what is needed
>

hm, agree.

	-ss

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Christoph Lameter]

On Tue, 9 Jun 2015, Andrew Morton wrote:

> Well I like it, even though it's going to cause a zillion little cleanup
> patches.
>
> checkpatch already has a "kfree(NULL) is safe and this check is
> probably not required" test so I guess Joe will need to get busy ;)
>
> I'll park these patches until after 4.1 is released - it's getting to
> that time...

Why do this at all? I understand that kfree/kmem_cache_free can take a
null pointer but this is the destruction of a cache and it usually
requires multiple actions to clean things up and these actions have to be
properly sequenced. All other processors have to stop referencing this
cache before it can be destroyed. I think failing if someone does
something strange like doing cache destruction with a NULL pointer is
valuable.

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Andrew Morton]

On Tue, 9 Jun 2015 20:11:25 -0500 (CDT) Christoph Lameter <cl@linux.com> wrote:

> On Tue, 9 Jun 2015, Andrew Morton wrote:
> 
> > Well I like it, even though it's going to cause a zillion little cleanup
> > patches.
> >
> > checkpatch already has a "kfree(NULL) is safe and this check is
> > probably not required" test so I guess Joe will need to get busy ;)
> >
> > I'll park these patches until after 4.1 is released - it's getting to
> > that time...
> 
> Why do this at all?

For the third time: because there are approx 200 callsites which are
already doing it.

> I understand that kfree/kmem_cache_free can take a
> null pointer but this is the destruction of a cache and it usually
> requires multiple actions to clean things up and these actions have to be
> properly sequenced. All other processors have to stop referencing this
> cache before it can be destroyed. I think failing if someone does
> something strange like doing cache destruction with a NULL pointer is
> valuable.

More than half of the kmem_cache_destroy() callsites are declining that
value by open-coding the NULL test.  That's reality and we should recognize
it.

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Christoph Lameter]

On Tue, 9 Jun 2015, Andrew Morton wrote:

> > Why do this at all?
>
> For the third time: because there are approx 200 callsites which are
> already doing it.

Did some grepping and I did see some call sites that do this but the
majority has to do other processing as well.

200 call sites? Do we have that many uses of caches? Typical prod system
have ~190 caches active and the merging brings that down to half of that.

> More than half of the kmem_cache_destroy() callsites are declining that
> value by open-coding the NULL test.  That's reality and we should recognize
> it.

Well that may just indicate that we need to have a look at those
callsites and the reason there to use a special cache at all. If the cache
is just something that kmalloc can provide then why create a special
cache. On the other hand if something special needs to be accomplished
then it would make sense to have special processing on kmem_cache_destroy.

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Andrew Morton]

On Tue, 9 Jun 2015 21:00:58 -0500 (CDT) Christoph Lameter <cl@linux.com> wrote:

> On Tue, 9 Jun 2015, Andrew Morton wrote:
> 
> > > Why do this at all?
> >
> > For the third time: because there are approx 200 callsites which are
> > already doing it.
> 
> Did some grepping and I did see some call sites that do this but the
> majority has to do other processing as well.
> 
> 200 call sites? Do we have that many uses of caches? Typical prod system
> have ~190 caches active and the merging brings that down to half of that.

I didn't try terribly hard.

z:/usr/src/linux-4.1-rc7> grep -r -C1 kmem_cache_destroy .  | grep "if [(]" | wc -l
158

It's a lot, anyway.

> > More than half of the kmem_cache_destroy() callsites are declining that
> > value by open-coding the NULL test.  That's reality and we should recognize
> > it.
> 
> Well that may just indicate that we need to have a look at those
> callsites and the reason there to use a special cache at all.

This makes no sense.  Go look at the code. 
drivers/staging/lustre/lustre/llite/super25.c, for example.  It's all
in the basic unwind/recover/exit code.

> If the cache
> is just something that kmalloc can provide then why create a special
> cache. On the other hand if something special needs to be accomplished
> then it would make sense to have special processing on kmem_cache_destroy.

This has nothing to do with anything.  We're talking about a basic "if
I created this cache then destroy it" operation.

It's a common pattern.  mm/ exists to serve client code and as a lot of
client code is doing this, we should move it into mm/ so as to serve
client code better.

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Joe Perches]

On Tue, 2015-06-09 at 19:17 -0700, Andrew Morton wrote:
> On Tue, 9 Jun 2015 21:00:58 -0500 (CDT) Christoph Lameter <cl@linux.com> wrote:
> > On Tue, 9 Jun 2015, Andrew Morton wrote:
> > > > Why do this at all?
> > Did some grepping and I did see some call sites that do this but the
> > majority has to do other processing as well.
> > 
> > 200 call sites? Do we have that many uses of caches? Typical prod system
> > have ~190 caches active and the merging brings that down to half of that.
> I didn't try terribly hard.
> z:/usr/src/linux-4.1-rc7> grep -r -C1 kmem_cache_destroy .  | grep "if [(]" | wc -l
> 158
> 
> It's a lot, anyway.

Yeah.

$ git grep -E -B1 -w "(kmem_cache|mempool|dma_pool)_destroy" *| \
  grep -P "\bif\s*\(" | wc -l
268

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Christoph Lameter]

On Tue, 9 Jun 2015, Andrew Morton wrote:

> > > More than half of the kmem_cache_destroy() callsites are declining that
> > > value by open-coding the NULL test.  That's reality and we should recognize
> > > it.
> >
> > Well that may just indicate that we need to have a look at those
> > callsites and the reason there to use a special cache at all.
>
> This makes no sense.  Go look at the code.
> drivers/staging/lustre/lustre/llite/super25.c, for example.  It's all
> in the basic unwind/recover/exit code.

That is screwed up code. I'd do that without the checks simply with a
series of kmem_cache_destroys().

> > If the cache
> > is just something that kmalloc can provide then why create a special
> > cache. On the other hand if something special needs to be accomplished
> > then it would make sense to have special processing on kmem_cache_destroy.
>
> This has nothing to do with anything.  We're talking about a basic "if
> I created this cache then destroy it" operation.

As you see in this code snipped you cannot continue if a certain operation
during setup fails. At that point it is known which caches exist and
therefore kmem_cache_destroy() can be called without the checks.

> It's a common pattern.  mm/ exists to serve client code and as a lot of
> client code is doing this, we should move it into mm/ so as to serve
> client code better.

Doing this seems to encourage sloppy coding practices.

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Andrew Morton]

On Thu, 11 Jun 2015 12:26:11 -0500 (CDT) Christoph Lameter <cl@linux.com> wrote:

> On Tue, 9 Jun 2015, Andrew Morton wrote:
> 
> > > > More than half of the kmem_cache_destroy() callsites are declining that
> > > > value by open-coding the NULL test.  That's reality and we should recognize
> > > > it.
> > >
> > > Well that may just indicate that we need to have a look at those
> > > callsites and the reason there to use a special cache at all.
> >
> > This makes no sense.  Go look at the code.
> > drivers/staging/lustre/lustre/llite/super25.c, for example.  It's all
> > in the basic unwind/recover/exit code.
> 
> That is screwed up code. I'd do that without the checks simply with a
> series of kmem_cache_destroys().

So go and review some of the many other callers which do this.

Re: [RFC][PATCH 0/5] do not dereference NULL pools in pools' destroy() functions

[Sergey Senozhatsky]

On (06/09/15 20:11), Christoph Lameter wrote:
> On Tue, 9 Jun 2015, Andrew Morton wrote:
> 
> > Well I like it, even though it's going to cause a zillion little cleanup
> > patches.
> >
> > checkpatch already has a "kfree(NULL) is safe and this check is
> > probably not required" test so I guess Joe will need to get busy ;)
> >
> > I'll park these patches until after 4.1 is released - it's getting to
> > that time...
> 
> Why do this at all?

this makes things less fragile.

> I understand that kfree/kmem_cache_free can take a
> null pointer but this is the destruction of a cache and it usually
> requires multiple actions to clean things up and these actions have to be
> properly sequenced. All other processors have to stop referencing this
> cache before it can be destroyed. 

>I think failing

well, it's not just `failing', it's a NULL pointer deref.

> if someone does something strange like doing cache destruction with a
> NULL pointer is valuable.
> 

a missing check is not `something strange'. it's just happening.

(a very quick google search)
http://help.lockergnome.com/linux/PATCH-dlm-NULL-dereference-failure-kmem_cache_create--ftopict555436.html
http://linux-kernel.2935.n7.nabble.com/PATCH-2-6-30-rc6-Remove-kmem-cache-destroy-in-s3c24xx-dma-init-td460417.html
etc.

	-ss