Messages in this thread Patch in this message | | | Date | Mon, 4 Oct 1999 02:55:41 +0200 (CEST) | From | Andrea Arcangeli <> | Subject | Re: Network-related Oopses on 2.2.13pre14 |
| |
On Mon, 4 Oct 1999, Andi Kleen wrote:
>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/
| |