do not dereference NULL pools in pools' destroy() functions

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 <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Quoted 4 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by David Rientjes <[email protected]>

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

On (06/17/15 16:14), David Rientjes wrote:
[..]

Quoted 7 lines

Quoted 4 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by David Rientjes <[email protected]>

Quoted 3 lines

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.

Quoted 13 lines · Sergey Senozhatsky

On (06/17/15 16:14), David Rientjes wrote:
[..]

Quoted 10 lines

Quoted 4 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by David Rientjes <[email protected]>

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

Quoted 3 lines

Thanks.

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

I'll refresh it and send it shortly.

Quoted 19 lines · Julia Lawall

Quoted 16 lines · Sergey Senozhatsky

On (06/17/15 16:14), David Rientjes wrote:
[..]

Quoted 10 lines

Quoted 4 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by David Rientjes <[email protected]>

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.

Quoted 2 lines

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)?

Quoted 3 lines

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.

On (06/17/15 16:14), David Rientjes wrote:
[..]

Quoted 7 lines

Quoted 4 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by David Rientjes <[email protected]>

Quoted 3 lines

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.

Quoted 13 lines · David Rientjes

[..]

Quoted 10 lines

Quoted 4 lines

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by David Rientjes <[email protected]>

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

Quoted 3 lines

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.

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 <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Quoted 7 lines

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

Proposed by Andrew Morton.

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by David Rientjes <[email protected]>

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 ;)

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 <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

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 <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

---
 mm/zpool.c | 3 +++
 1 file changed, 3 insertions(+)

mm/zpool.c +3 -1

index bacdab6..2f59b90 100644
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 10 lines · 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.

Quoted 5 lines

Proposed by Andrew Morton.

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Acked-by Dan Streetman <[email protected]>

Quoted 21 lines

---
 mm/zpool.c | 3 +++
 1 file changed, 3 insertions(+)

mm/zpool.c +3 -1

index bacdab6..2f59b90 100644
@@ -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

Quoted 5 lines

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

Quoted 18 lines · Dan Streetman

Quoted 15 lines · 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 <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Quoted 1 line

Acked-by Dan Streetman <[email protected]>

Thanks.

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

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

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

Proposed by Andrew Morton.

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Quoted 6 lines

Quoted 1 line

Acked-by Dan Streetman <[email protected]>

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.

On (06/10/15 17:48), Joe Perches wrote:
[..]

Quoted 20 lines

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

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

Proposed by Andrew Morton.

Signed-off-by Sergey Senozhatsky <[email protected]>

Reported-by Andrew Morton <[email protected]>

LKML-reference: https://lkml.org/lkml/2015/6/8/583

Quoted 6 lines

Quoted 1 line

Acked-by Dan Streetman <[email protected]>

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?

Quoted 3 lines

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

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.

Quoted 12 lines · On Tue, 9 Jun 2015 21:04:48 +0900 Sergey Senozhatsky…

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()

Quoted 1 line

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 ;)

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 <[email protected]>

---
V2: Remove useless debugging print messages and multiple quotemetas

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

scripts/checkpatch.pl +21 -4

index 69c4716..87d3bf1aa 100755
@@ -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 {

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 <[email protected]>

---
V2: Remove useless debugging print messages and multiple quotemetas

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

scripts/checkpatch.pl +21 -4

index 69c4716..87d3bf1aa 100755
@@ -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 {

Quoted 3 lines · Joe Perches

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

Quoted 3 lines

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

Quoted 5 lines · Julia Lawall

Quoted 3 lines · Joe Perches

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

Quoted 8 lines

Quoted 3 lines

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

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);

Quoted 12 lines

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.

Quoted 5 lines

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?

Quoted 2 lines

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

Quoted 2 lines

Quoted 2 lines

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)
{

(quoted diff) +6 -14

-       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);

Quoted 29 lines · Julia Lawall

Quoted 2 lines

Quoted 2 lines

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)
{

(quoted diff) +6 -14

-       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);

Review

Quoted 10 lines

(quoted diff) +3 -4

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

Quoted 8 lines

[..]

err_percpu_counter_init:

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

Quoted 3 lines

[..]

and others.

This I find much less appealing. The labels make clear what is needed

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

Quoted 3 lines · Andrew Morton

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

Quoted 5 lines

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.

Quoted 1 line

Why do this at all?

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

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.

Quoted 2 lines · Andrew Morton

Quoted 1 line

Why do this at all?

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 5 lines · On Tue, 9 Jun 2015 21:00:58 -0500 (CDT) Christoph Lameter <[email protected]> wrote:

Quoted 1 line · Andrew Morton

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 7 lines · Andrew Morton

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 3 lines · Andrew Morton

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

Quoted 5 lines

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.