On Fri, 2012-11-30 at 12:10 +0000, David Woodhouse wrote: > In that case I think we're fine. I'll just do the same thing in > br2684_push(), fix up the comment you just corrected, and we're all > good.
OK, here's an update to me my patch 8/17 'br2684: don't send frames on
not-ready vcc'. It takes the socket lock and does fairly much the same
thing as your pppoatm version. It returns NETDEV_TX_BUSY and stops the
queue if the socket is locked, and it gets woken from the ->release_cb
callback.
I've dropped your Acked-By: since it's mostly new, but feel free to give
me a fresh one. With this I think we're done.
Unless Chas has any objections, I'll ask Dave to pull it...
From 47d5ad4c98452bcddfd00da1c659dac85202f213 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dw...@infradead.org>
Date: Tue, 27 Nov 2012 23:28:36 +0000
Subject: [PATCH] br2684: don't send frames on not-ready vcc
Avoid submitting packets to a vcc which is being closed. Things go badly
wrong when the ->pop method gets later called after everything's been
torn down.
Use the ATM socket lock for synchronisation with vcc_destroy_socket(),
which clears the ATM_VF_READY bit under the same lock. Otherwise, we
could end up submitting a packet to the device driver even after its
->ops->close method has been called. And it could call the vcc's ->pop
method after the protocol has been shut down. Which leads to a panic.
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
net/atm/br2684.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 45 insertions(+), 3 deletions(-)
diff --git a/net/atm/br2684.c b/net/atm/br2684.c
index 8eb6fbe..5ff145f 100644
--- a/net/atm/br2684.c
+++ b/net/atm/br2684.c
@@ -68,6 +68,7 @@ struct br2684_vcc {
/* keep old push, pop functions for chaining */
void (*old_push)(struct atm_vcc *vcc, struct sk_buff *skb);
void (*old_pop)(struct atm_vcc *vcc, struct sk_buff *skb);
+ void (*old_release_cb)(struct atm_vcc *vcc);
enum br2684_encaps encaps;
struct list_head brvccs;
#ifdef CONFIG_ATM_BR2684_IPFILTER
@@ -269,6 +270,22 @@ static int br2684_xmit_vcc(struct sk_buff *skb, struct
net_device *dev,
return !atmvcc->send(atmvcc, skb);
}
+static void br2684_release_cb(struct atm_vcc *atmvcc)
+{
+ struct br2684_vcc *brvcc = BR2684_VCC(atmvcc);
+
+ /*
+ * A race with br2684_xmit_vcc() might cause a spurious wakeup just
+ * after that function *stops* the queue, and qspace might actually
+ * go negative before the queue stops again. We cope with that.
+ */
+ if (atomic_read(&brvcc->qspace) > 0)
+ netif_wake_queue(brvcc->device);
+
+ if (brvcc->old_release_cb)
+ brvcc->old_release_cb(atmvcc);
+}
+
static inline struct br2684_vcc *pick_outgoing_vcc(const struct sk_buff *skb,
const struct br2684_dev
*brdev)
{
@@ -280,6 +297,8 @@ static netdev_tx_t br2684_start_xmit(struct sk_buff *skb,
{
struct br2684_dev *brdev = BRPRIV(dev);
struct br2684_vcc *brvcc;
+ struct atm_vcc *atmvcc;
+ netdev_tx_t ret = NETDEV_TX_OK;
pr_debug("skb_dst(skb)=%p\n", skb_dst(skb));
read_lock(&devs_lock);
@@ -290,9 +309,26 @@ static netdev_tx_t br2684_start_xmit(struct sk_buff *skb,
dev->stats.tx_carrier_errors++;
/* netif_stop_queue(dev); */
dev_kfree_skb(skb);
- read_unlock(&devs_lock);
- return NETDEV_TX_OK;
+ goto out_devs;
+ }
+ atmvcc = brvcc->atmvcc;
+
+ bh_lock_sock(sk_atm(atmvcc));
+
+ if (test_bit(ATM_VF_RELEASED, &atmvcc->flags) ||
+ test_bit(ATM_VF_CLOSE, &atmvcc->flags) ||
+ !test_bit(ATM_VF_READY, &atmvcc->flags)) {
+ dev->stats.tx_dropped++;
+ dev_kfree_skb(skb);
+ goto out;
}
+
+ if (sock_owned_by_user(sk_atm(atmvcc))) {
+ netif_stop_queue(brvcc->device);
+ ret = NETDEV_TX_BUSY;
+ goto out;
+ }
+
if (!br2684_xmit_vcc(skb, dev, brvcc)) {
/*
* We should probably use netif_*_queue() here, but that
@@ -304,8 +340,11 @@ static netdev_tx_t br2684_start_xmit(struct sk_buff *skb,
dev->stats.tx_errors++;
dev->stats.tx_fifo_errors++;
}
+ out:
+ bh_unlock_sock(sk_atm(atmvcc));
+ out_devs:
read_unlock(&devs_lock);
- return NETDEV_TX_OK;
+ return ret;
}
/*
@@ -378,6 +417,7 @@ static void br2684_close_vcc(struct br2684_vcc *brvcc)
list_del(&brvcc->brvccs);
write_unlock_irq(&devs_lock);
brvcc->atmvcc->user_back = NULL; /* what about vcc->recvq ??? */
+ brvcc->atmvcc->release_cb = brvcc->old_release_cb;
brvcc->old_push(brvcc->atmvcc, NULL); /* pass on the bad news */
kfree(brvcc);
module_put(THIS_MODULE);
@@ -554,9 +594,11 @@ static int br2684_regvcc(struct atm_vcc *atmvcc, void
__user * arg)
brvcc->encaps = (enum br2684_encaps)be.encaps;
brvcc->old_push = atmvcc->push;
brvcc->old_pop = atmvcc->pop;
+ brvcc->old_release_cb = atmvcc->release_cb;
barrier();
atmvcc->push = br2684_push;
atmvcc->pop = br2684_pop;
+ atmvcc->release_cb = br2684_release_cb;
/* initialize netdev carrier state */
if (atmvcc->dev->signal == ATM_PHY_SIG_LOST)
--
1.8.0
--
dwmw2
smime.p7s
Description: S/MIME cryptographic signature
