[PATCH] smc91x: plug race between TX tasklet and driver reset

The race causes a kernel oops when smc_hardware_send_pkt() tries to
dereference pending_tx_skb which would have been freed from one of the
driver reset paths just after the tx_task tasklet has been scheduled.
This race is possible on SMP but was uncovered by the kernel RT work.

Signed-off-by: Nicolas Pitre <nico@cam.org>

1 files changed, 24 insertions, 19 deletions

diff --git a/drivers/net/smc91x.c b/drivers/net/smc91x.c
index fd80048..cfb9d3c 100644
--- a/drivers/net/smc91x.c
+++ b/drivers/net/smc91x.c
@@ -315,15 +315,25 @@ static void smc_reset(struct net_device *dev)
 	struct smc_local *lp = netdev_priv(dev);
 	void __iomem *ioaddr = lp->base;
 	unsigned int ctl, cfg;
+	struct sk_buff *pending_skb;
 
 	DBG(2, "%s: %s\n", dev->name, __FUNCTION__);
 
-	/* Disable all interrupts */
+	/* Disable all interrupts, block TX tasklet */
 	spin_lock(&lp->lock);
 	SMC_SELECT_BANK(2);
 	SMC_SET_INT_MASK(0);
+	pending_skb = lp->pending_tx_skb;
+	lp->pending_tx_skb = NULL;
 	spin_unlock(&lp->lock);
 
+	/* free any pending tx skb */
+	if (pending_skb) {
+		dev_kfree_skb(pending_skb);
+		lp->stats.tx_errors++;
+		lp->stats.tx_aborted_errors++;
+	}
+
 	/*
 	 * This resets the registers mostly to defaults, but doesn't
 	 * affect EEPROM.  That seems unnecessary
@@ -389,14 +399,6 @@ static void smc_reset(struct net_device *dev)
 	SMC_SELECT_BANK(2);
 	SMC_SET_MMU_CMD(MC_RESET);
 	SMC_WAIT_MMU_BUSY();
-
-	/* clear anything saved */
-	if (lp->pending_tx_skb != NULL) {
-		dev_kfree_skb (lp->pending_tx_skb);
-		lp->pending_tx_skb = NULL;
-		lp->stats.tx_errors++;
-		lp->stats.tx_aborted_errors++;
-	}
 }
 
 /*
@@ -440,6 +442,7 @@ static void smc_shutdown(struct net_device *dev)
 {
 	struct smc_local *lp = netdev_priv(dev);
 	void __iomem *ioaddr = lp->base;
+	struct sk_buff *pending_skb;
 
 	DBG(2, "%s: %s\n", CARDNAME, __FUNCTION__);
 
@@ -447,7 +450,11 @@ static void smc_shutdown(struct net_device *dev)
 	spin_lock(&lp->lock);
 	SMC_SELECT_BANK(2);
 	SMC_SET_INT_MASK(0);
+	pending_skb = lp->pending_tx_skb;
+	lp->pending_tx_skb = NULL;
 	spin_unlock(&lp->lock);
+	if (pending_skb)
+		dev_kfree_skb(pending_skb);
 
 	/* and tell the card to stay away from that nasty outside world */
 	SMC_SELECT_BANK(0);
@@ -627,7 +634,12 @@ static void smc_hardware_send_pkt(unsigned long data)
 	}
 
 	skb = lp->pending_tx_skb;
+	if (unlikely(!skb)) {
+		smc_special_unlock(&lp->lock);
+		return;
+	}
 	lp->pending_tx_skb = NULL;
+
 	packet_no = SMC_GET_AR();
 	if (unlikely(packet_no & AR_FAILED)) {
 		printk("%s: Memory allocation failed.\n", dev->name);
@@ -702,7 +714,6 @@ static int smc_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	DBG(3, "%s: %s\n", dev->name, __FUNCTION__);
 
 	BUG_ON(lp->pending_tx_skb != NULL);
-	lp->pending_tx_skb = skb;
 
 	/*
 	 * The MMU wants the number of pages to be the number of 256 bytes
@@ -718,7 +729,6 @@ static int smc_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	numPages = ((skb->len & ~1) + (6 - 1)) >> 8;
 	if (unlikely(numPages > 7)) {
 		printk("%s: Far too big packet error.\n", dev->name);
-		lp->pending_tx_skb = NULL;
 		lp->stats.tx_errors++;
 		lp->stats.tx_dropped++;
 		dev_kfree_skb(skb);
@@ -745,6 +755,7 @@ static int smc_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	smc_special_unlock(&lp->lock);
 
+	lp->pending_tx_skb = skb;
    	if (!poll_count) {
 		/* oh well, wait until the chip finds memory later */
 		netif_stop_queue(dev);
@@ -1062,7 +1073,7 @@ static void smc_phy_powerdown(struct net_device *dev)
 	   above). linkwatch_event() also wants the netlink semaphore.
 	*/
 	while(lp->work_pending)
-		schedule();
+		yield();
 
 	bmcr = smc_phy_read(dev, phy, MII_BMCR);
 	smc_phy_write(dev, phy, MII_BMCR, bmcr | BMCR_PDOWN);
@@ -1606,14 +1617,8 @@ static int smc_close(struct net_device *dev)
 
 	/* clear everything */
 	smc_shutdown(dev);
-
+	tasklet_kill(&lp->tx_task);
 	smc_phy_powerdown(dev);
-
-	if (lp->pending_tx_skb) {
-		dev_kfree_skb(lp->pending_tx_skb);
-		lp->pending_tx_skb = NULL;
-	}
-
 	return 0;
 }