proto_unregister sleeps while atomic

proto_unregister sleeps while atomic

[Daniele Orlandi]

I'm looking at proto_unregister() in linux-2.6.13:

Il calls kmem_cache_destroy() while holding proto_list_lock:

void proto_unregister(struct proto *prot)
{
        write_lock(&proto_list_lock);

        if (prot->slab != NULL) {
                kmem_cache_destroy(prot->slab);


However, kmem_cache_destroy() may sleep:

int kmem_cache_destroy (kmem_cache_t * cachep)
{
         int i;
 
         if (!cachep || in_interrupt())
                BUG();
 
         /* Don't let CPUs to come and go */
         lock_cpu_hotplug();

        /* Find the cache in the chain of caches. */
        down(&cache_chain_sem);
        ^^^^^^^^^^^^^^^^^^^^^^^

Am I seeing it right?

Bye,

-- 
  Daniele Orlandi

Re: proto_unregister sleeps while atomic

[Patrick McHardy]

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

Daniele Orlandi wrote:
> I'm looking at proto_unregister() in linux-2.6.13:
> 
> Il calls kmem_cache_destroy() while holding proto_list_lock:
> 
> void proto_unregister(struct proto *prot)
> {
>         write_lock(&proto_list_lock);
> 
>         if (prot->slab != NULL) {
>                 kmem_cache_destroy(prot->slab);
> 
> 
> However, kmem_cache_destroy() may sleep:
> 
>         /* Find the cache in the chain of caches. */
>         down(&cache_chain_sem);
>         ^^^^^^^^^^^^^^^^^^^^^^^
> 
> Am I seeing it right?

You're right, good catch. This patch fixes it by moving the lock
down to the list-operation which it is supposed to protect.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1111 bytes --]

[NET]: proto_unregister: fix sleeping while atomic

proto_unregister holds a lock while calling kmem_cache_destroy, which can
sleep.

Noticed by Daniele Orlandi <daniele@orlandi.com>.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 90fdf8bc78b52d40190766c5496a3d8c24903be8
tree 54048218d981ab263b37e0de09ad52f4bd49a000
parent 591bd554f58b7d363167760a606d2a84696772da
author Patrick McHardy <kaber@trash.net> Wed, 07 Sep 2005 01:00:00 +0200
committer Patrick McHardy <kaber@trash.net> Wed, 07 Sep 2005 01:00:00 +0200

 net/core/sock.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1528,8 +1528,6 @@ EXPORT_SYMBOL(proto_register);
 
 void proto_unregister(struct proto *prot)
 {
-	write_lock(&proto_list_lock);
-
 	if (prot->slab != NULL) {
 		kmem_cache_destroy(prot->slab);
 		prot->slab = NULL;
@@ -1551,6 +1549,7 @@ void proto_unregister(struct proto *prot
 		prot->twsk_slab = NULL;
 	}
 
+	write_lock(&proto_list_lock);
 	list_del(&prot->node);
 	write_unlock(&proto_list_lock);
 }

Re: proto_unregister sleeps while atomic

[David S. Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 07 Sep 2005 01:02:01 +0200

> You're right, good catch. This patch fixes it by moving the lock
> down to the list-operation which it is supposed to protect.

I think we need to unlink from the list first if you're
going to do it this way.  Otherwise someone can find the
protocol via lookup, and then bogusly try to use the SLAB
cache we're freeing up.

Or does something else prevent this?

Re: proto_unregister sleeps while atomic

[Patrick McHardy]

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

David S. Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Wed, 07 Sep 2005 01:02:01 +0200
> 
> 
>>You're right, good catch. This patch fixes it by moving the lock
>>down to the list-operation which it is supposed to protect.
> 
> 
> I think we need to unlink from the list first if you're
> going to do it this way.  Otherwise someone can find the
> protocol via lookup, and then bogusly try to use the SLAB
> cache we're freeing up.
> 
> Or does something else prevent this?

The only other user of proto_list besides proto_register, which
doesn't care, are the seqfs functions. They use the slab pointer,
but in a harmless way:

                    proto->slab == NULL ? "no" : "yes",

Anyway, I've moved it up to the top.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1168 bytes --]

[NET]: proto_unregister: fix sleeping while atomic

proto_unregister holds a lock while calling kmem_cache_destroy, which can
sleep.

Noticed by Daniele Orlandi <daniele@orlandi.com>.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit d68b08edb26dfb58d18ab6c555d011572f9115a6
tree 1d14cf91ca5db6878b6af3953f85a34a6fe12a91
parent 591bd554f58b7d363167760a606d2a84696772da
author Patrick McHardy <kaber@trash.net> Wed, 07 Sep 2005 01:35:19 +0200
committer Patrick McHardy <kaber@trash.net> Wed, 07 Sep 2005 01:35:19 +0200

 net/core/sock.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1529,6 +1529,8 @@ EXPORT_SYMBOL(proto_register);
 void proto_unregister(struct proto *prot)
 {
 	write_lock(&proto_list_lock);
+	list_del(&prot->node);
+	write_unlock(&proto_list_lock);
 
 	if (prot->slab != NULL) {
 		kmem_cache_destroy(prot->slab);
@@ -1550,9 +1552,6 @@ void proto_unregister(struct proto *prot
 		kfree(name);
 		prot->twsk_slab = NULL;
 	}
-
-	list_del(&prot->node);
-	write_unlock(&proto_list_lock);
 }
 
 EXPORT_SYMBOL(proto_unregister);

Re: proto_unregister sleeps while atomic

[David S. Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 07 Sep 2005 01:42:48 +0200

> The only other user of proto_list besides proto_register, which
> doesn't care, are the seqfs functions. They use the slab pointer,
> but in a harmless way:
> 
>                     proto->slab == NULL ? "no" : "yes",
> 
> Anyway, I've moved it up to the top.

Ok thanks, patch applied.