[PATCH] pppoatm: don't send frames to destroyed vcc

[PATCH] pppoatm: don't send frames to destroyed vcc

[Krzysztof Mazur]

The pppoatm_send() uses vcc->send() directly and does not check if vcc
is ready for send(). This causes Oops when send() is used after
vcc_destroy_socket() at least with usbatm driver:

Oops: 0000 [#1] PREEMPT
Pid: 0, comm: swapper Not tainted 3.6.0-krzysiek-00001-gb7cd93b-dirty #60    /AK32
EIP: 0060:[<c01413c6>] EFLAGS: 00010082 CPU: 0
EIP is at __wake_up_common+0x16/0x70
EAX: 30707070 EBX: 00000292 ECX: 00000001 EDX: dca75fc0
ESI: 00000000 EDI: de7f500f EBP: df409f24 ESP: df409f08
 DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
CR0: 8005003b CR2: 30707070 CR3: 1c920000 CR4: 000007d0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
Process swapper (pid: 0, ti=df408000 task=c07bd4e0 task.ti=c07b0000)
Stack:
 00000000 00000001 00000001 dca75fc0 00000292 00000000 de7f500f df409f3c
 c0143299 00000000 00000000 dc84f000 dc84f000 df409f4c c0602bf0 00000000
 dc84f000 df409f58 c0604301 dc840cc0 df409fb4 c04672e5 c076a240 00000000
Call Trace:
 [<c0143299>] __wake_up+0x29/0x50
 [<c0602bf0>] vcc_write_space+0x40/0x80
 [<c0604301>] atm_pop_raw+0x21/0x30
 [<c04672e5>] usbatm_tx_process+0x2a5/0x380
 [<c0126cf9>] tasklet_action+0x39/0x70
 [<c0126f1f>] __do_softirq+0x7f/0x120
 [<c0126ea0>] ? local_bh_enable_ip+0xa0/0xa0
 <IRQ>

Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
This bug can be easily triggered with 9d02daf754238adac48fa075ee79e7edd3d79ed3
(pppoatm: Fix excessive queue bloat) present in Linux >= 3.5-rc1.
Steps to reproduce (tested with 9d02daf75 and v3.6):
$ ping -i 0.1 some.host.via.modem &
unplug ADSL line
sleep 3
plug ADSL line
The Oops occurs almost always.

On Linux v3.0.44, v3.4.12 the above steps did not trigger the bug, but
I saw similar Oops also on kernels < 3.5-rc1, but I was never able to
reproduce it or save kernel log.

This bug also exists in some stable kernels.

Maybe it's better to drop frame:
	if (..) {
		kfree_skb(skb);
		return 1;
	}
instead of
	goto nospace;

The full Oops + some logs from Linux 3.6.0 with some minor changes
(copying kernel log to VRAM after crash and enabled usbatm debugging with
some additional debug).

ATM dev 0: usbatm_atm_close entered
ATM dev 0: usbatm_atm_close: deallocating vcc 0xdca75e20 with vpi 0 vci 35
ATM dev 0: usbatm_cancel_send entered
ATM dev 0: usbatm_cancel_send: popping skb 0xdc840840
ATM dev 0: usbatm_cancel_send: popping current skb (0xdcb26740)
ATM dev 0: usbatm_cancel_send done
ATM dev 0: usbatm_atm_close successful
ATM dev 0: usbatm_atm_send entered (skb 0xdc840cc0, len 10) vcc=dc84f000
ATM dev 0: usbatm_atm_send queue skb: dc840cc0 vcc dc84f000
ATM dev 0: usbatm_atm_send entered (skb 0xdc84c920, len 10) vcc=dc84f000
ATM dev 0: usbatm_atm_send queue skb: dc84c920 vcc dc84f000
ATM dev 0: usbatm_tx_process skb dcb26740
  len=86
ATM dev 0: usbatm_tx_process skb dc840cc0
  len=10
usbatm_rx_process: 4991 callbacks suppressed
ATM dev 0: usbatm_rx_process: status 0 in frame 0!
ATM dev 0: usbatm_rx_process: status 0 in frame 1!
ATM dev 0: usbatm_rx_process: status 0 in frame 2!
ATM dev 0: usbatm_rx_process: status 0 in frame 0!
ATM dev 0: usbatm_rx_process: status 0 in frame 1!
ATM dev 0: usbatm_rx_process: status 0 in frame 2!
ATM dev 0: usbatm_rx_process: status 0 in frame 0!
ATM dev 0: usbatm_rx_process: status 0 in frame 1!
ATM dev 0: usbatm_rx_process: status 0 in frame 2!
ATM dev 0: usbatm_rx_process: status 0 in frame 0!
ATM dev 0: usbatm_atm_open: vpi 0, vci 35
ATM dev 0: usbatm_atm_open: allocated vcc data 0xdca75c20
ATM dev 0: usbatm_atm_send entered (skb 0xdcda8b40, len 12) vcc=d2761c00
ATM dev 0: usbatm_atm_send queue skb: dcda8b40 vcc d2761c00
ATM dev 0: usbatm_tx_process skb dc840cc0
  len=10
ATM dev 0: usbatm_atm_send entered (skb 0xdc8406c0, len 12) vcc=d2761c00
ATM dev 0: usbatm_atm_send queue skb: dc8406c0 vcc d2761c00
ATM dev 0: usbatm_tx_process skb dc840cc0
  len=10
ATM dev 0: usbatm_tx_process skb dc840cc0
  len=10
ATM dev 0: usbatm_tx_process pop skb dc840cc0, vcc dc84f000%
BUG: unable to handle kernel paging request at 30707070
IP: [<c01413c6>] __wake_up_common+0x16/0x70
*pde = 00000000 
Oops: 0000 [#1] PREEMPT 
Pid: 0, comm: swapper Not tainted 3.6.0-krzysiek-00001-gb7cd93b-dirty #60    /AK32 
EIP: 0060:[<c01413c6>] EFLAGS: 00010082 CPU: 0
EIP is at __wake_up_common+0x16/0x70
EAX: 30707070 EBX: 00000292 ECX: 00000001 EDX: dca75fc0
ESI: 00000000 EDI: de7f500f EBP: df409f24 ESP: df409f08
 DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
CR0: 8005003b CR2: 30707070 CR3: 1c920000 CR4: 000007d0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
Process swapper (pid: 0, ti=df408000 task=c07bd4e0 task.ti=c07b0000)
Stack:
 00000000 00000001 00000001 dca75fc0 00000292 00000000 de7f500f df409f3c
 c0143299 00000000 00000000 dc84f000 dc84f000 df409f4c c0602bf0 00000000
 dc84f000 df409f58 c0604301 dc840cc0 df409fb4 c04672e5 c076a240 00000000
Call Trace:
 [<c0143299>] __wake_up+0x29/0x50
 [<c0602bf0>] vcc_write_space+0x40/0x80
 [<c0604301>] atm_pop_raw+0x21/0x30
 [<c04672e5>] usbatm_tx_process+0x2a5/0x380
 [<c0126cf9>] tasklet_action+0x39/0x70
 [<c0126f1f>] __do_softirq+0x7f/0x120
 [<c0126ea0>] ? local_bh_enable_ip+0xa0/0xa0
 <IRQ> 

 [<c01271de>] ? irq_exit+0x6e/0x90
 [<c0103be3>] ? do_IRQ+0x43/0xb0
 [<c0144f9c>] ? sched_clock_local.constprop.1+0x4c/0x1a0
 [<c0626369>] ? common_interrupt+0x29/0x30
 [<c02f458b>] ? acpi_idle_enter_simple+0xf5/0x122
 [<c04bd81e>] ? cpuidle_enter+0x1e/0x30
 [<c04bdb08>] ? cpuidle_idle_call+0x68/0xd0
 [<c0109155>] ? cpu_idle+0x45/0x80
 [<c060b4df>] ? rest_init+0x63/0x74
 [<c07fd851>] ? start_kernel+0x25e/0x264
 [<c07fd42e>] ? repair_env_string+0x51/0x51
 [<c07fd26e>] ? i386_start_kernel+0x44/0x46
Code: c3 8d 74 26 00 31 f6 31 db eb cc 66 90 0f b6 4d e8 d3 eb eb bb 55 89 e5 57 56 53 83 ec 10 89 45 f0 89 55 ec 89 c2 8b 00 89 4d e8 <8b> 18 8d 70 f4 83 eb 0c 39 c2 75 0a eb 37 8d 74 26 00 89 de 89
EIP: [<c01413c6>] __wake_up_common+0x16/0x70 SS:ESP 0068:df409f08
CR2: 0000000030707070
---[ end trace bc86ff846f3d97ec ]---
Kernel panic - not syncing: Fatal exception in interrupt

 net/atm/pppoatm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 226dca9..0dcb5dc 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -269,6 +269,8 @@ 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;
+
 	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))
@@ -301,6 +303,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 		return 1;
 	}
 
+	vcc = ATM_SKB(skb)->vcc;
+	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
+			|| test_bit(ATM_VF_CLOSE, &vcc->flags)
+			|| !test_bit(ATM_VF_READY, &vcc->flags))
+		goto nospace;
+
 	atomic_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
 	ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
 	pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
-- 
1.7.12.2.2.g1c3c581

Re: [PATCH] pppoatm: don't send frames to destroyed vcc

[David Woodhouse]

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

On Sat, 2012-10-06 at 14:19 +0200, Krzysztof Mazur wrote:
> Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
> indicate that vcc is not ready.

And what locking prevents the flag from being set immediately after we
check it?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation



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

Re: [PATCH] pppoatm: don't send frames to destroyed vcc

[Krzysztof Mazur]

On Sat, Oct 06, 2012 at 02:32:50PM +0100, David Woodhouse wrote:
> On Sat, 2012-10-06 at 14:19 +0200, Krzysztof Mazur wrote:
> > Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
> > indicate that vcc is not ready.
> 
> And what locking prevents the flag from being set immediately after we
> check it?
> 

nothing, this patch should fix this. 

The vcc_sendmsg() uses lock_sock(sk). This lock is used by
vcc_release(), so vcc_destroy_socket() will not be called between
check and during ->send(). The vcc_release_async() sets ATM_VF_CLOSE,
but it should be safe to call ->send() after it, because
vcc->dev->ops->close() is not called.

The pppoatm_send() is called with bottom halfs disabled, so
bh_lock_sock() should be used instead of lock_sock().

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 0dcb5dc..29afc68 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -270,6 +270,7 @@ 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);
@@ -304,17 +305,22 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	}
 
 	vcc = ATM_SKB(skb)->vcc;
+	bh_lock_sock(sk_atm(vcc));
 	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
 			|| test_bit(ATM_VF_CLOSE, &vcc->flags)
-			|| !test_bit(ATM_VF_READY, &vcc->flags))
+			|| !test_bit(ATM_VF_READY, &vcc->flags)) {
+		bh_unlock_sock(sk_atm(vcc));
 		goto nospace;
+	}
 
 	atomic_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
 	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:
 	/*
 	 * We don't have space to send this SKB now, but we might have

-- 
Krzysiek

Re: [PATCH] pppoatm: don't send frames to destroyed vcc

[Krzysztof Mazur]

On Sat, Oct 06, 2012 at 05:38:04PM +0200, Krzysztof Mazur wrote:
> On Sat, Oct 06, 2012 at 02:32:50PM +0100, David Woodhouse wrote:
> > On Sat, 2012-10-06 at 14:19 +0200, Krzysztof Mazur wrote:
> > > Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
> > > indicate that vcc is not ready.
> > 
> > And what locking prevents the flag from being set immediately after we
> > check it?
> > 
> 
> nothing, this patch should fix this. 
> 

I think there is another problem here. The pppoatm gets a reference
to atmvcc, but I don't see anything that protects against removal
of that vcc.

The vcc uses vcc->sk socket for reference counting, so sock_hold()
and sock_put() should be used by pppoatm.

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 29afc68..126f57f 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -154,6 +154,7 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)
 	tasklet_kill(&pvcc->wakeup_tasklet);
 	ppp_unregister_channel(&pvcc->chan);
 	atmvcc->user_back = NULL;
+	sock_put(sk_atm(pvcc->atmvcc));
 	kfree(pvcc);
 	/* Gee, I hope we have the big kernel lock here... */
 	module_put(THIS_MODULE);
@@ -371,6 +372,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	if (pvcc == NULL)
 		return -ENOMEM;
 	pvcc->atmvcc = atmvcc;
+	sock_hold(sk_atm(atmvcc));
 
 	/* Maximum is zero, so that we can use atomic_inc_not_zero() */
 	atomic_set(&pvcc->inflight, NONE_INFLIGHT);
@@ -385,6 +387,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	pvcc->wakeup_tasklet.data = (unsigned long) &pvcc->chan;
 	err = ppp_register_channel(&pvcc->chan);
 	if (err != 0) {
+		sock_put(sk_atm(atmvcc));
 		kfree(pvcc);
 		return err;
 	}

-- 
Krzysiek

Re: [PATCH] pppoatm: don't send frames to destroyed vcc

[Krzysztof Mazur]

On Sat, Oct 06, 2012 at 05:38:04PM +0200, Krzysztof Mazur wrote:
> On Sat, Oct 06, 2012 at 02:32:50PM +0100, David Woodhouse wrote:
> > On Sat, 2012-10-06 at 14:19 +0200, Krzysztof Mazur wrote:
> > > Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
> > > indicate that vcc is not ready.
> > 
> > And what locking prevents the flag from being set immediately after we
> > check it?
> > 
> 
> nothing, this patch should fix this. 
> 
>  
>  	vcc = ATM_SKB(skb)->vcc;
> +	bh_lock_sock(sk_atm(vcc));

After bh_lock_sock() sock_owned_by_user(sk_atm(vcc)) should be checked
here. I'm sending fixed patch.

>  	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
>  			|| test_bit(ATM_VF_CLOSE, &vcc->flags)

Krzysiek
-- 
>From 3ae93335423ed0ba526dc80163ff6dfeba9bbea1 Mon Sep 17 00:00:00 2001
From: Krzysztof Mazur <krzysiek@podlesie.net>
Date: Mon, 8 Oct 2012 08:10:22 +0200
Subject: [PATCH] pppoatm: fix race condition with destroying of vcc

The pppoatm_send() calls vcc->send() and now also checks for
some vcc flags that indicate destroyed vcc without proper locking.

The vcc_sendmsg() uses lock_sock(sk). This lock is used by
vcc_release(), so vcc_destroy_socket() will not be called between
check and during ->send(). The vcc_release_async() sets ATM_VF_CLOSE,
but it should be safe to call ->send() after it, because
vcc->dev->ops->close() is not called.

The pppoatm_send() is called with BH disabled, so bh_lock_sock()
should be used instead of lock_sock().

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 net/atm/pppoatm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 0dcb5dc..e3b2d69 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -270,6 +270,7 @@ 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);
@@ -304,17 +305,24 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	}
 
 	vcc = ATM_SKB(skb)->vcc;
+	bh_lock_sock(sk_atm(vcc));
+	if (sock_owned_by_user(sk_atm(vcc)))
+		goto nospace_unlock_sock;
 	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
 			|| test_bit(ATM_VF_CLOSE, &vcc->flags)
 			|| !test_bit(ATM_VF_READY, &vcc->flags))
-		goto nospace;
+		goto nospace_unlock_sock;
 
 	atomic_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
 	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_unlock_sock:
+	bh_unlock_sock(sk_atm(vcc));
 nospace:
 	/*
 	 * We don't have space to send this SKB now, but we might have
-- 
1.7.12.2.2.g1c3c581