[PATCH] nets: fix problem of using lock

[PATCH] nets: fix problem of using lock

[Jianjun Kong]

net/core/skbuff.c: void skb_queue_purge(struct sk_buff_head *list)

This function should takes the the list lock, because the operation to
this list shoule be atomic. And __skb_queue_purge()  (in
include/linux/skbuff.c) real delete the buffers in the list.

Signed-off-by: Jianjun Kong <jianjun@zeuux.org>
---
 net/core/skbuff.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ebb6b94..3b89fb1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1834,9 +1834,11 @@ struct sk_buff *skb_dequeue_tail(struct sk_buff_head *list)
  */
 void skb_queue_purge(struct sk_buff_head *list)
 {
-	struct sk_buff *skb;
-	while ((skb = skb_dequeue(list)) != NULL)
-		kfree_skb(skb);
+	unsigned long flags;
+
+	spin_lock_irqsave(&list->lock, flags);
+	__skb_queue_purge(list);
+	spin_unlock_irqrestore(&list->lock, flags);
 }
 
 /**
-- 
1.5.6.3

Re: [PATCH] nets: fix problem of using lock

[David Miller]

From: Jianjun Kong <jianjun@zeuux.org>
Date: Thu, 6 Nov 2008 18:41:03 +0800

> net/core/skbuff.c: void skb_queue_purge(struct sk_buff_head *list)
> 
> This function should takes the the list lock, because the operation to
> this list shoule be atomic. And __skb_queue_purge()  (in
> include/linux/skbuff.c) real delete the buffers in the list.
> 
> Signed-off-by: Jianjun Kong <jianjun@zeuux.org>

No, this function is fine.  skb_dequeue() takes the lock so
there cannot be any list corruption.

And this function is called in contexts where the caller knows
that no new packets can be added to the list (closing a socket,
shutting down a device, etc.)  And even if new packets could
appear, taking the lock over the entire function would not
help that problem.

In fact, I suspect that many if not all skb_queue_purge() callers
can be converted to use __skb_queue_purge().

Re: [PATCH] nets: fix problem of using lock

[Jianjun Kong]

On Mon, Nov 10, 2008 at 01:35:15PM -0800, David Miller wrote:
>From: Jianjun Kong <jianjun@zeuux.org>
>Date: Thu, 6 Nov 2008 18:41:03 +0800
>
>> net/core/skbuff.c: void skb_queue_purge(struct sk_buff_head *list)
>> 
>> This function should takes the the list lock, because the operation to
>> this list shoule be atomic. And __skb_queue_purge()  (in
>> include/linux/skbuff.c) real delete the buffers in the list.
>> 
>> Signed-off-by: Jianjun Kong <jianjun@zeuux.org>
>
>No, this function is fine.  skb_dequeue() takes the lock so
>there cannot be any list corruption.
>
>And this function is called in contexts where the caller knows
>that no new packets can be added to the list (closing a socket,
>shutting down a device, etc.)  And even if new packets could
>appear, taking the lock over the entire function would not
>help that problem.
>
>In fact, I suspect that many if not all skb_queue_purge() callers
>can be converted to use __skb_queue_purge().

Thanks, I've known :-)

-- 
Jianjun Kong @_@ Happy Hacking
Homepage: http://kongove.cn/
Gtalk: kongjianjun@gmail.com