The loopback send path reaches netif_rx() without net_bh_lock: start_bh_atomic() is a no-op, so an application send or a TCP retransmit runs dev_queue_xmit() -> loopback_xmit() holding only global_lock, while the ethernet and tunnel receive paths enqueue under net_bh_lock. The skb queue spinlocks are no-ops on Hurd, so the two enqueues can corrupt the receive backlog. net_bh_lock is the lock meant to protect the backlog. loopback_xmit() and dev_loopback_xmit() must take it around netif_rx().
net_bh() never holds both locks, and the receive threads are not blocked while the protocol handlers run. dev_clear_backlog() also touches the backlog, so protect it with net_bh_lock. Correct the loopback.c and sched.c comments. * pfinet/sched.c (net_bh_worker): Do not take global_lock around net_bh(); release net_bh_lock for the duration of net_bh(). * pfinet/loopback.c (loopback_xmit): Take net_bh_lock around netif_rx(). * pfinet/linux-src/net/core/dev.c (dev_loopback_xmit): Likewise. (net_bh): Run protocol handlers under global_lock, take net_bh_lock only for the backlog dequeue, and never hold both locks at once. (dev_clear_backlog): Protect the backlog with net_bh_lock. * pfinet/glue-include/linux/interrupt.h: Update the start_bh_atomic() comment. --- v2: Drop the separate backlog_lock. Reuse net_bh_lock: loopback_xmit() and dev_loopback_xmit() take it around netif_rx(), net_bh_worker() no longer takes global_lock, and net_bh() alternates net_bh_lock (dequeue) and global_lock (protocol handling) without ever holding both. v1: https://lists.gnu.org/archive/html/bug-hurd/2026-10/msg00090.html --- pfinet/glue-include/linux/interrupt.h | 2 +- pfinet/linux-src/net/core/dev.c | 57 ++++++++++++++++----------- pfinet/loopback.c | 6 +-- pfinet/sched.c | 9 +++-- 4 files changed, 43 insertions(+), 31 deletions(-) diff --git a/pfinet/glue-include/linux/interrupt.h b/pfinet/glue-include/linux/interrupt.h index 49eb23d..a335e57 100644 --- a/pfinet/glue-include/linux/interrupt.h +++ b/pfinet/glue-include/linux/interrupt.h @@ -10,7 +10,7 @@ #define synchronize_bh() ((void) 0) /* XXX ? */ /* The code that can call these are already entered holding - global_lock, which locks out the net_bh worker thread. */ + global_lock. */ #define start_bh_atomic() ((void) 0) #define end_bh_atomic() ((void) 0) /* diff --git a/pfinet/linux-src/net/core/dev.c b/pfinet/linux-src/net/core/dev.c index b47c502..fa672b5 100644 --- a/pfinet/linux-src/net/core/dev.c +++ b/pfinet/linux-src/net/core/dev.c @@ -577,7 +577,9 @@ void dev_loopback_xmit(struct sk_buff *skb) newskb->ip_summed = CHECKSUM_UNNECESSARY; if (newskb->dst==NULL) printk(KERN_DEBUG "BUG: packet without dst looped back 1\n"); + pthread_mutex_lock (&net_bh_lock); netif_rx(newskb); + pthread_mutex_unlock (&net_bh_lock); } int dev_queue_xmit(struct sk_buff *skb) @@ -718,7 +720,6 @@ static void netdev_wakeup(void) static void dev_clear_backlog(struct device *dev) { struct sk_buff *curr; - unsigned long flags; /* * @@ -727,30 +728,35 @@ static void dev_clear_backlog(struct device *dev) * 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 we must grab the - * skb_queue_lock. + * net_bh_lock. */ + pthread_mutex_lock(&net_bh_lock); if (backlog.qlen) { 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); + pthread_mutex_unlock(&net_bh_lock); kfree_skb(curr); + pthread_mutex_lock(&net_bh_lock); goto repeat; } - spin_unlock_irqrestore(&skb_queue_lock, flags); #ifdef CONFIG_NET_HW_FLOWCONTROL if (netdev_dropping) + { + pthread_mutex_unlock(&net_bh_lock); netdev_wakeup(); + return; + } #else netdev_dropping = 0; #endif } + pthread_mutex_unlock(&net_bh_lock); } /* @@ -861,6 +867,8 @@ void net_bh(void) * latency on a transmit interrupt bh. */ + pthread_mutex_lock (&global_lock); + if (qdisc_head.forw != &qdisc_head) qdisc_run_queues(); @@ -870,15 +878,7 @@ void net_bh(void) * that from the device which does a mark_bh() just after */ - /* - * While the queue is not empty.. - * - * Note that the queue never shrinks due to - * an interrupt, so we can do this test without - * disabling interrupts. - */ - - while (!skb_queue_empty(&backlog)) + for (;;) { struct sk_buff * skb; @@ -888,10 +888,26 @@ void net_bh(void) goto net_bh_break; #endif - /* - * We have a packet. Therefore the queue has shrunk - */ + pthread_mutex_unlock (&global_lock); + pthread_mutex_lock (&net_bh_lock); skb = skb_dequeue(&backlog); + if (skb == NULL) + { +#ifdef CONFIG_NET_HW_FLOWCONTROL + int dropping = netdev_dropping; +#else + netdev_dropping = 0; +#endif + pthread_mutex_unlock (&net_bh_lock); + pthread_mutex_lock (&global_lock); +#ifdef CONFIG_NET_HW_FLOWCONTROL + if (dropping) + netdev_wakeup(); +#endif + break; + } + pthread_mutex_unlock (&net_bh_lock); + pthread_mutex_lock (&global_lock); #ifndef _HURD_ #ifdef CONFIG_CPU_IS_SLOW @@ -1028,12 +1044,7 @@ void net_bh(void) } #endif #endif -#ifdef CONFIG_NET_HW_FLOWCONTROL - if (netdev_dropping) - netdev_wakeup(); -#else - netdev_dropping = 0; -#endif + pthread_mutex_unlock (&global_lock); NET_PROFILE_LEAVE(net_bh); return; diff --git a/pfinet/loopback.c b/pfinet/loopback.c index e15e426..5804461 100644 --- a/pfinet/loopback.c +++ b/pfinet/loopback.c @@ -73,12 +73,12 @@ static int loopback_xmit(struct sk_buff *skb, struct device *dev) #endif /* - * Calling netif_rx() requires locking net_bh_lock, which - * has already been done since this function is called by - * the net_bh worker thread. + * Calling netif_rx() requires locking net_bh_lock. */ + pthread_mutex_lock (&net_bh_lock); netif_rx(skb); + pthread_mutex_unlock (&net_bh_lock); stats->rx_bytes+=skb->len; stats->tx_bytes+=skb->len; diff --git a/pfinet/sched.c b/pfinet/sched.c index 5a3dd3e..6508c3d 100644 --- a/pfinet/sched.c +++ b/pfinet/sched.c @@ -49,8 +49,9 @@ sock_wake_async (struct socket *sock, int how) netif_rx either drops the packet, or enqueues it and wakes us up via mark_bh which is really condition_broadcast on net_bh_wakeup. The packet receiver thread holds net_bh_lock while calling netif_rx. - We wake up and take global_lock, which locks out RPC service threads. - We then also take net_bh_lock running net_bh. + We wake up and run net_bh, which takes net_bh_lock only to dequeue + from the backlog and global_lock, which locks out RPC service threads, + to run the protocol handlers. Thus, only this thread running net_bh locks out the packet receiver thread (which takes only net_bh_lock while calling netif_rx), so packets are quickly moved from the Mach port's message queue to the `backlog' @@ -70,9 +71,9 @@ net_bh_worker (void *arg) net_bh_raised = 0; - pthread_mutex_lock (&global_lock); + pthread_mutex_unlock (&net_bh_lock); net_bh (); - pthread_mutex_unlock (&global_lock); + pthread_mutex_lock (&net_bh_lock); } /*NOTREACHED*/ return 0; -- 2.47.3
