Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc
From: Krzysztof Mazur <hidden>
Date: 2012-11-02 09:40:24
Also in:
lkml
Subsystem:
atm, networking [general], ppp over atm (rfc 2364), the rest · Maintainers:
Chas Williams, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Mitchell Blank Jr, Linus Torvalds
On Thu, Nov 01, 2012 at 10:26:28AM -0400, chas williams - CONTRACTOR wrote:
On Wed, 31 Oct 2012 23:04:35 +0100 Krzysztof Mazur [off-list ref] wrote:quoted
There are also some minor potential issues in pppoatm driver: - locking issues, but now only between pppoatm_send() and vcc_sendmsg() and maybe some other functions,these have been around for a while. i agree that something should be done about it. just not sure what should be synchronizing this mess.
I think the ATM socket lock should be used. I'm sending the latest patch that adds this locking after David Woodhouse's comments. The vcc->flags check is now probably unnecessary.
quoted
- missing check for SS_CONNECTED in pppoatm_ioctl,in practice you will never run into this because a pvc is immediately put into SS_CONNECTED mode (right before the userspace open() returns). however, should it check? yes. i dont see anything preventing you from running ppp on svc's.
I can confirm that the problem really exists, without connect() in pppoatm plugin in pppd, I have seen an Oops and panic. I will send appropriate patch. Thanks. Krzysiek -- >8 --
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index f27a07a..ef19436 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c@@ -269,10 +269,23 @@ static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size) static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) { struct pppoatm_vcc *pvcc = chan_to_pvcc(chan); + struct atm_vcc *vcc; + int ret; + ATM_SKB(skb)->vcc = pvcc->atmvcc; pr_debug("(skb=0x%p, vcc=0x%p)\n", skb, pvcc->atmvcc); if (skb->data[0] == '\0' && (pvcc->flags & SC_COMP_PROT)) (void) skb_pull(skb, 1); + + vcc = ATM_SKB(skb)->vcc; + bh_lock_sock(sk_atm(vcc)); + if (sock_owned_by_user(sk_atm(vcc))) + goto nospace; + if (test_bit(ATM_VF_RELEASED, &vcc->flags) + || test_bit(ATM_VF_CLOSE, &vcc->flags) + || !test_bit(ATM_VF_READY, &vcc->flags)) + goto nospace; + switch (pvcc->encaps) { /* LLC encapsulation needed */ case e_llc: if (skb_headroom(skb) < LLC_LEN) {
@@ -285,8 +298,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) } consume_skb(skb); skb = n; - if (skb == NULL) + if (skb == NULL) { + bh_unlock_sock(sk_atm(vcc)); return DROP_PACKET; + } } else if (!pppoatm_may_send(pvcc, skb->truesize)) goto nospace; memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
@@ -296,6 +311,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) goto nospace; break; case e_autodetect: + bh_unlock_sock(sk_atm(vcc)); pr_debug("Trying to send without setting encaps!\n"); kfree_skb(skb); return 1;
@@ -305,9 +321,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options; pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev); - return ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb) + ret = ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb) ? DROP_PACKET : 1; + bh_unlock_sock(sk_atm(vcc)); + return ret; nospace: + bh_unlock_sock(sk_atm(vcc)); /* * We don't have space to send this SKB now, but we might have * already applied SC_COMP_PROT compression, so may need to undo