>skb_queue_lock is really an internal lock to the skb module. That dev.c plays
>with it is already not nice, but holding it for longer times while calling
^^^^^^^^^^^^^^^^
You have to grab it. You are walking the list. It's completly legal (it's
nice).
>other subsystems (kfree_skb -> skb->destruct etc.) is just nasty and violates
>the abstraction barriers. And it is dangerous because it makes every skb_*
Agreed.
>Maybe the fix is simple, but the implications on the rest of the code
>are not :)
:)
>You never know, there are drivers doing clever things with it.
I don't know about drivers not in the stock kernel, true.
Here it is a new (untested) patch that will solve the destructor issue
by harming some more the complexity of the slow path.
--- 2.2.13pre14/net/core/dev.c Tue Sep 28 18:32:40 1999
+++ /tmp/dev.c Mon Oct 4 02:49:12 1999
@@ -57,6 +57,8 @@
* A network device unload needs to purge
* the backlog queue.
* Paul Rusty Russel : SIOCSIFNAME
+ * Andrea Arcangeli : dev_clear_backlog() needs the
+ * skb_queue_lock held.
*/
#include <asm/uaccess.h>
@@ -711,7 +713,8 @@
static void dev_clear_backlog(struct device *dev)
{
- struct sk_buff *prev, *curr;
+ struct sk_buff *curr;
+ unsigned long flags;
/*
*
@@ -719,27 +722,24 @@
*
* We are competing here both with netif_rx() and net_bh().
* We don't want either of those to mess with skb ptrs
- * while we work on them, thus cli()/sti().
- *
- * It looks better to use net_bh trick, at least
- * to be sure, that we keep interrupt latency really low. --ANK (980727)
+ * while we work on them, thus we must grab the
+ * skb_queue_lock.
*/
if (backlog.qlen) {
- start_bh_atomic();
- curr = backlog.next;
- while ( curr != (struct sk_buff *)(&backlog) ) {
- unsigned long flags;
- curr=curr->next;
- if ( curr->prev->dev == dev ) {
- prev = curr->prev;
- spin_lock_irqsave(&skb_queue_lock, flags);
- __skb_unlink(prev, &backlog);
+ repeat:
+ spin_lock_irqsave(&skb_queue_lock, flags);
+ for (curr = backlog.next;
+ curr != (struct sk_buff *)(&backlog);
+ curr = curr->next)
+ if (curr->dev == dev)
+ {
+ __skb_unlink(curr, &backlog);
spin_unlock_irqrestore(&skb_queue_lock, flags);
- kfree_skb(prev);
+ kfree_skb(curr);
+ goto repeat;
}
- }
- end_bh_atomic();
+ spin_unlock_irqrestore(&skb_queue_lock, flags);
#ifdef CONFIG_NET_HW_FLOWCONTROL
if (netdev_dropping)
netdev_wakeup();
Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/