fix RCU-callback-after-kmem_cache_destroy problem in sl[aou]b

Hello!

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result in RCU callbacks accessing a kmem_cache after it had been destroyed.

The following untested (might not even compile) patch proposes a fix.

Reported-by Jesper Dangaard Brouer <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---

 slab.c |    2 +-
 slob.c |    2 ++
 slub.c |    2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

mm/slab.c +1 -1

index e74a16e..5241b65 100644
@@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
 	}
 
 	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
-		synchronize_rcu();
+		rcu_barrier();
 
 	__kmem_cache_destroy(cachep);
 	mutex_unlock(&cache_chain_mutex);

mm/slob.c +2 -0

index c78742d..9641da3 100644
@@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
 void kmem_cache_destroy(struct kmem_cache *c)
 {
 	kmemleak_free(c);
+	if (c->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	slob_free(c, sizeof(struct kmem_cache));
 }
 EXPORT_SYMBOL(kmem_cache_destroy);

mm/slub.c +2 -0

index 819f056..a9201d8 100644
@@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)
  */
 void kmem_cache_destroy(struct kmem_cache *s)
 {
+	if (s->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	down_write(&slub_lock);
 	s->refcount--;
 	if (!s->refcount) {

Quoted 6 lines · Paul E. McKenney

Hello!

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result in RCU callbacks accessing a kmem_cache after it had been destroyed.

Quoted 1 line

The following untested (might not even compile) patch proposes a fix.

Acked-by Matt Mackall <[email protected]>

Nick, you'll want to make sure you get this in SLQB.

Quoted 1 line

Reported-by Jesper Dangaard Brouer <[email protected]>

Quoted 39 lines

Signed-off-by Paul E. McKenney <[email protected]>

---

 slab.c |    2 +-
 slob.c |    2 ++
 slub.c |    2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

mm/slab.c +1 -1

index e74a16e..5241b65 100644
@@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
 	}
 
 	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
-		synchronize_rcu();
+		rcu_barrier();
 
 	__kmem_cache_destroy(cachep);
 	mutex_unlock(&cache_chain_mutex);

mm/slob.c +2 -0

index c78742d..9641da3 100644
@@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
 void kmem_cache_destroy(struct kmem_cache *c)
 {
 	kmemleak_free(c);
+	if (c->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	slob_free(c, sizeof(struct kmem_cache));
 }
 EXPORT_SYMBOL(kmem_cache_destroy);

mm/slub.c +0 -0

index 819f056..a9201d8 100644
@@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)

Quoted 8 lines

mm/slub.c +2 -0

  */
 void kmem_cache_destroy(struct kmem_cache *s)
 {
+	if (s->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	down_write(&slub_lock);
 	s->refcount--;
 	if (!s->refcount) {

Signature · 1 line

http://selenic.com : development and support for Mercurial and Linux

Quoted 11 lines · Matt Mackall

Quoted 7 lines · Paul E. McKenney

Hello!

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result in RCU callbacks accessing a kmem_cache after it had been destroyed.

The following untested (might not even compile) patch proposes a fix.

Acked-by Matt Mackall <[email protected]>

Quoted 3 lines

Nick, you'll want to make sure you get this in SLQB.

Quoted 1 line

Reported-by Jesper Dangaard Brouer <[email protected]>

And I seem to have blown Jesper's email address (apologies, Jesper!), so:

Reported-by Jesper Dangaard Brouer <[email protected]>

Quoted 6 lines · Paul E. McKenney

Hello!

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result in RCU callbacks accessing a kmem_cache after it had been destroyed.

Quoted 1 line

The following untested (might not even compile) patch proposes a fix.

Acked-by Matt Mackall <[email protected]>

Nick, you'll want to make sure you get this in SLQB.

Quoted 41 lines

Reported-by Jesper Dangaard Brouer <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---

 slab.c |    2 +-
 slob.c |    2 ++
 slub.c |    2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

mm/slab.c +1 -1

index e74a16e..5241b65 100644
@@ -2547,7 +2547,7 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
 	}
 
 	if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
-		synchronize_rcu();
+		rcu_barrier();
 
 	__kmem_cache_destroy(cachep);
 	mutex_unlock(&cache_chain_mutex);

mm/slob.c +2 -0

index c78742d..9641da3 100644
@@ -595,6 +595,8 @@ EXPORT_SYMBOL(kmem_cache_create);
 void kmem_cache_destroy(struct kmem_cache *c)
 {
 	kmemleak_free(c);
+	if (c->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	slob_free(c, sizeof(struct kmem_cache));
 }
 EXPORT_SYMBOL(kmem_cache_destroy);

mm/slub.c +0 -0

index 819f056..a9201d8 100644
@@ -2595,6 +2595,8 @@ static inline int kmem_cache_close(struct kmem_cache *s)

Quoted 8 lines

mm/slub.c +2 -0

  */
 void kmem_cache_destroy(struct kmem_cache *s)
 {
+	if (s->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
 	down_write(&slub_lock);
 	s->refcount--;
 	if (!s->refcount) {

Signature · 1 line

http://selenic.com : development and support for Mercurial and Linux

Quoted 7 lines · Matt Mackall

Quoted 6 lines · Paul E. McKenney

Hello!

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result in RCU callbacks accessing a kmem_cache after it had been destroyed.

Quoted 5 lines

Quoted 1 line

The following untested (might not even compile) patch proposes a fix.

Acked-by Matt Mackall <[email protected]>

Nick, you'll want to make sure you get this in SLQB.

Thanks Matt. Paul, I think this should be appropriate for [email protected] too?

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result in RCU callbacks accessing a kmem_cache after it had been destroyed.

The following untested (might not even compile) patch proposes a fix.

Quoted 5 lines · Paul E. McKenney

Jesper noted that kmem_cache_destroy() invokes synchronize_rcu() rather than rcu_barrier() in the SLAB_DESTROY_BY_RCU case, which could result in RCU callbacks accessing a kmem_cache after it had been destroyed.

The following untested (might not even compile) patch proposes a fix.

It could be seen to be the responsibility of the caller of kmem_cache_destroy to insure that no accesses are pending.

If the caller specified destroy by rcu on cache creation then he also needs to be aware of not destroying the cache itself until all rcu actions are complete. This is similar to the caution that has to be execised then accessing cache data itself.

This is a reasonable point, and in keeping with the design principle 'callers should handle their own special cases'. However, I think it would be more than a little surprising for kmem_cache_free() to do the right thing, but not kmem_cache_destroy().

Quoted 4 lines · Matt Mackall

This is a reasonable point, and in keeping with the design principle 'callers should handle their own special cases'. However, I think it would be more than a little surprising for kmem_cache_free() to do the right thing, but not kmem_cache_destroy().

kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU. The freed object can be accessed after free until the rcu interval expires (well sortof, it may even be reallocated within the interval).

There are special RCU considerations coming already with the use of kmem_cache_free().

Adding RCU operations to the kmem_cache_destroy() logic may result in unnecessary RCU actions for slabs where the coder is ensuring that the RCU interval has passed by other means.

Quoted 14 lines · Christoph Lameter

Quoted 4 lines · Matt Mackall

This is a reasonable point, and in keeping with the design principle 'callers should handle their own special cases'. However, I think it would be more than a little surprising for kmem_cache_free() to do the right thing, but not kmem_cache_destroy().

kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU. The freed object can be accessed after free until the rcu interval expires (well sortof, it may even be reallocated within the interval).

There are special RCU considerations coming already with the use of kmem_cache_free().

Quoted 3 lines

Adding RCU operations to the kmem_cache_destroy() logic may result in unnecessary RCU actions for slabs where the coder is ensuring that the RCU interval has passed by other means.

Do we care? Cache destruction shouldn't be in anyone's fast path. Correctness is more important and users are more liable to be correct with this patch.

Quoted 19 lines · Matt Mackall

Quoted 17 lines · Christoph Lameter

Quoted 4 lines · Matt Mackall

This is a reasonable point, and in keeping with the design principle 'callers should handle their own special cases'. However, I think it would be more than a little surprising for kmem_cache_free() to do the right thing, but not kmem_cache_destroy().

kmem_cache_free() must be used carefully when using SLAB_DESTROY_BY_RCU. The freed object can be accessed after free until the rcu interval expires (well sortof, it may even be reallocated within the interval).

There are special RCU considerations coming already with the use of kmem_cache_free().

Adding RCU operations to the kmem_cache_destroy() logic may result in unnecessary RCU actions for slabs where the coder is ensuring that the RCU interval has passed by other means.

Quoted 3 lines

Do we care? Cache destruction shouldn't be in anyone's fast path. Correctness is more important and users are more liable to be correct with this patch.

I am with Matt on this one -- if we are going to hand the users of SLAB_DESTROY_BY_RCU a hand grenade, let's at least leave the pin in.

I don't even claim to understand all the RCU details here but I don't see why we should care about _kmem_cache_destroy()_ performance at this level. Christoph, hmmm?

Quoted 3 lines

I don't even claim to understand all the RCU details here but I don't see why we should care about _kmem_cache_destroy()_ performance at this level. Christoph, hmmm?

Well it was surprising to me that kmem_cache_destroy() would perform rcu actions in the first place. RCU is usually handled externally and not within the slab allocator. The only reason that SLAB_DESTROY_BY_RCU exists is because the user cannot otherwise control the final release of memory to the page allocator.