Re: [PATCH v2 3/3] pppoatm: protect against freeing of vcc

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 4457 bytes --]

On Thu, 2012-11-29 at 16:09 +0100, Krzysztof Mazur wrote:
> 
> I don't like two thinks about this patch:
> 
>         - if allos_skb(sizeof(*header), GFP_ATOMIC) at beginning of
>           pclose() fails we will crash
> 
>         - if card wakes up after this timeout we will probably crash too
> 
> That's why proposed different approach, but it has other problems.

How about this variant on what you suggested. Yes, we can definitely
remove everything that's in the queue... as long as we use
skb_queue_walk_safe() instead of skb_queue_walk().

We can use GFP_KERNEL instead of GFP_ATOMIC, which at least reduces the
likelihood of failing to close the vcc.

We end up waiting *only* if there is a packet which is *currently* being
DMA'd to the card. And if the card doesn't take that within 5 seconds,
it almost certainly never will. So I can live with that.

I'd definitely want someone with a DMA-capable FPGA to test this
properly, adding printks to it to make sure the interesting path is
being exercised. Nathan, you should be able to trigger it with the same
test that used to just crash the system entirely — send a flood of
packets while you kill br2684ctl.

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 6258961..2006a39 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -92,7 +92,6 @@ struct pkt_hdr {
 };
 
 struct solos_skb_cb {
-	struct completion c;
 	struct atm_vcc *vcc;
 	uint32_t dma_addr;
 };
@@ -868,10 +867,11 @@ static int popen(struct atm_vcc *vcc)
 static void pclose(struct atm_vcc *vcc)
 {
 	struct solos_card *card = vcc->dev->dev_data;
-	struct sk_buff *skb;
+	struct sk_buff *skb, *tmpskb;
 	struct pkt_hdr *header;
+	unsigned char port = SOLOS_CHAN(vcc->dev);
 
-	skb = alloc_skb(sizeof(*header), GFP_ATOMIC);
+	skb = alloc_skb(sizeof(*header), GFP_KERNEL);
 	if (!skb) {
 		dev_warn(&card->dev->dev, "Failed to allocate sk_buff in pclose()\n");
 		return;
@@ -883,22 +883,50 @@ static void pclose(struct atm_vcc *vcc)
 	header->vci = cpu_to_le16(vcc->vci);
 	header->type = cpu_to_le16(PKT_PCLOSE);
 
-	init_completion(&SKB_CB(skb)->c);
-
 	fpga_queue(card, SOLOS_CHAN(vcc->dev), skb, NULL);
 
 	clear_bit(ATM_VF_ADDR, &vcc->flags);
 	clear_bit(ATM_VF_READY, &vcc->flags);
 
-	if (!wait_for_completion_timeout(&SKB_CB(skb)->c,
-					 msecs_to_jiffies(5000)))
-		dev_warn(&card->dev->dev, "Timeout waiting for VCC close on port %d\n",
-			 SOLOS_CHAN(vcc->dev));
+	/* Remove any yet-to-be-transmitted packets from the pending queue */
+	spin_lock(&card->tx_queue_lock);
+	skb_queue_walk_safe(&card->tx_queue[port], skb, tmpskb) {
+		if (SKB_CB(skb)->vcc == vcc) {
+			skb_unlink(skb, &card->tx_queue[port]);
+			solos_pop(vcc, skb);
+		}
+	}
+	spin_unlock(&card->tx_queue_lock);
 
 	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
 	   tasklet has finished processing any incoming packets (and, more to
 	   the point, using the vcc pointer). */
 	tasklet_unlock_wait(&card->tlet);
+
+	/*
+	 * If we're in DMA mode and a skb on this VCC is *currently* being
+	 * submitted, wait for it to finish (using param_wq)
+	 */
+	if (card->using_dma) {
+		DEFINE_WAIT(wait);
+
+		spin_lock(&card->tx_lock);
+		while (card->tx_skb[port] && SKB_CB(card->tx_skb[port])->vcc == vcc) {
+			prepare_to_wait(&card->param_wq, &wait, TASK_UNINTERRUPTIBLE);
+			spin_unlock(&card->tx_lock);
+
+			if (schedule_timeout(5 * HZ)) {
+				dev_warn(&card->dev->dev,
+					 "Timeout waiting for VCC close on port %d\n",
+					 port);
+				goto done_waiting;
+			}
+			spin_lock(&card->tx_lock);
+		}
+		spin_unlock(&card->tx_lock);
+	done_waiting:
+		finish_wait(&card->param_wq, &wait);
+	}
 	return;
 }
 
@@ -1020,12 +1048,15 @@ static uint32_t fpga_tx(struct solos_card *card)
 			if (vcc) {
 				atomic_inc(&vcc->stats->tx);
 				solos_pop(vcc, oldskb);
-			} else {
-				struct pkt_hdr *header = (void *)oldskb->data;
-				if (le16_to_cpu(header->type) == PKT_PCLOSE)
-					complete(&SKB_CB(oldskb)->c);
+
+				/*
+				 * If it's a TX skb on a closed VCC, pclose()
+				 * may be waiting for it...
+				 */
+				if (!test_bit(ATM_VF_READY, &vcc->flags))
+					wake_up(&card->param_wq);
+			} else
 				dev_kfree_skb_irq(oldskb);
-			}
 		}
 	}
 	/* For non-DMA TX, write the 'TX start' bit for all four ports simultaneously */


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]