From patchwork Thu Nov 29 15:47:57 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: David Woodhouse X-Patchwork-Id: 202770 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 028902C0092 for ; Fri, 30 Nov 2012 02:48:47 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754909Ab2K2PsK (ORCPT ); Thu, 29 Nov 2012 10:48:10 -0500 Received: from merlin.infradead.org ([205.233.59.134]:42921 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754836Ab2K2PsI (ORCPT ); Thu, 29 Nov 2012 10:48:08 -0500 Received: from shinybook.infradead.org ([2001:8b0:10b:1:e6ce:8fff:fe1f:f2c0]) by merlin.infradead.org with esmtpsa (Exim 4.76 #1 (Red Hat Linux)) id 1Te6LI-0005Ay-E0; Thu, 29 Nov 2012 15:48:01 +0000 Message-ID: <1354204077.21562.172.camel@shinybook.infradead.org> Subject: Re: [PATCH v2 3/3] pppoatm: protect against freeing of vcc From: David Woodhouse To: Krzysztof Mazur Cc: David Laight , chas williams - CONTRACTOR , davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, nathan@traverse.com.au Date: Thu, 29 Nov 2012 15:47:57 +0000 In-Reply-To: <20121129150945.GB16478@shrek.podlesie.net> References: <1350926091-12642-1-git-send-email-krzysiek@podlesie.net> <1350926091-12642-3-git-send-email-krzysiek@podlesie.net> <1354036592.2534.6.camel@shinybook.infradead.org> <20121127173906.GA11390@shrek.podlesie.net> <1354039349.2534.11.camel@shinybook.infradead.org> <20121127135434.0728cd4f@thirdoffive.cmf.nrl.navy.mil> <1354141115.21562.101.camel@shinybook.infradead.org> <20121129150945.GB16478@shrek.podlesie.net> X-Mailer: Evolution 3.6.1 (3.6.1-1.fc18) Mime-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 */