Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-11-02 Thread Krzysztof Mazur 
On Fri, Nov 02, 2012 at 10:40:18AM +0100, Krzysztof Mazur wrote:
> 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  wrote:
> > >   - 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.

I'm sending the patch that fixes this issue. Works correctly with original
pppd, and does not crash with pppd without connect() - the pppd just
logs:

pppd[3460]: ioctl(ATM_SETBACKEND): Invalid argument

and exits.

Krzysiek

-- >8 --
Subject: [PATCH] pppoatm: allow assign only on a connected socket

The pppoatm does not check if the used vcc is in connected state,
causing an Oops in pppoatm_send() when vcc->send() is called
on not fully connected socket.

Now pppoatm can be assigned only on connected sockets; otherwise
-EINVAL error is returned.

Signed-off-by: Krzysztof Mazur 
---
BUG: unable to handle kernel NULL pointer dereference at   (null)
IP: [<  (null)>]   (null)
*pde =  
Oops:  [#1] PREEMPT 
Pid: 4154, comm: pppd Not tainted 3.6.0-krzysiek-2-g3ff1093 #95/AK32 
EIP: 0060:[<>] EFLAGS: 00010202 CPU: 0
EIP is at 0x0
EAX: d95f7800 EBX: d9d4ba80 ECX: d95f7800 EDX: d9d4ba80
ESI:  EDI: 0001 EBP: 01c0 ESP: d9823f34
 DS: 007b ES: 007b FS:  GS: 0033 SS: 0068
CR0: 8005003b CR2:  CR3: 1e7b6000 CR4: 07d0
DR0:  DR1:  DR2:  DR3: 
DR6: 0ff0 DR7: 0400
Process pppd (pid: 4154, ti=d9822000 task=d99918a0 task.ti=d9822000)
Stack:
 c060cd9b c043a2ca d99ed860 d99ed864 d9d4ba80 08094f22 c043a228 d9d4ba80
 d99ed860 000c c043a347  000c d94311a0 08094f22 c043a290
 c019f72e d9823f9c 0003 09df1090 d94311a0 08094f22 0008 d9822000
Call Trace:
 [] ? pppoatm_send+0x6b/0x300
 [] ? ppp_write+0x3a/0xe0
 [] ? ppp_channel_push+0x38/0xa0
 [] ? ppp_write+0xb7/0xe0
 [] ? ppp_channel_push+0xa0/0xa0
 [] ? vfs_write+0x8e/0x140
 [] ? sys_write+0x3c/0x70
 [] ? sysenter_do_call+0x12/0x26
Code:  Bad EIP value.
EIP: [<>] 0x0 SS:ESP 0068:d9823f34
CR2: 
---[ end trace e29cf1805f576278 ]---
Kernel panic - not syncing: Fatal exception in interrupt

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

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 226dca9..f27a07a 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -406,6 +406,8 @@ static int pppoatm_ioctl(struct socket *sock, unsigned int 
cmd,
return -ENOIOCTLCMD;
if (!capable(CAP_NET_ADMIN))
return -EPERM;
+   if (sock->state != SS_CONNECTED)
+   return -EINVAL;
return pppoatm_assign_vcc(atmvcc, argp);
}
case PPPIOCGCHAN:
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-11-02 Thread Krzysztof Mazur 
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  wrote:
> 
> > 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.

> 
> > - 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, >flags)
+   || test_bit(ATM_VF_CLOSE, >flags)
+   || !test_bit(ATM_VF_READY, >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-11-02 Thread Krzysztof Mazur 
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 krzys...@podlesie.net wrote:
 
  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.

 
  - 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-11-02 Thread Krzysztof Mazur 
On Fri, Nov 02, 2012 at 10:40:18AM +0100, Krzysztof Mazur wrote:
 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 krzys...@podlesie.net wrote:
 - 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.

I'm sending the patch that fixes this issue. Works correctly with original
pppd, and does not crash with pppd without connect() - the pppd just
logs:

pppd[3460]: ioctl(ATM_SETBACKEND): Invalid argument

and exits.

Krzysiek

-- 8 --
Subject: [PATCH] pppoatm: allow assign only on a connected socket

The pppoatm does not check if the used vcc is in connected state,
causing an Oops in pppoatm_send() when vcc-send() is called
on not fully connected socket.

Now pppoatm can be assigned only on connected sockets; otherwise
-EINVAL error is returned.

Signed-off-by: Krzysztof Mazur krzys...@podlesie.net
---
BUG: unable to handle kernel NULL pointer dereference at   (null)
IP: [  (null)]   (null)
*pde =  
Oops:  [#1] PREEMPT 
Pid: 4154, comm: pppd Not tainted 3.6.0-krzysiek-2-g3ff1093 #95/AK32 
EIP: 0060:[] EFLAGS: 00010202 CPU: 0
EIP is at 0x0
EAX: d95f7800 EBX: d9d4ba80 ECX: d95f7800 EDX: d9d4ba80
ESI:  EDI: 0001 EBP: 01c0 ESP: d9823f34
 DS: 007b ES: 007b FS:  GS: 0033 SS: 0068
CR0: 8005003b CR2:  CR3: 1e7b6000 CR4: 07d0
DR0:  DR1:  DR2:  DR3: 
DR6: 0ff0 DR7: 0400
Process pppd (pid: 4154, ti=d9822000 task=d99918a0 task.ti=d9822000)
Stack:
 c060cd9b c043a2ca d99ed860 d99ed864 d9d4ba80 08094f22 c043a228 d9d4ba80
 d99ed860 000c c043a347  000c d94311a0 08094f22 c043a290
 c019f72e d9823f9c 0003 09df1090 d94311a0 08094f22 0008 d9822000
Call Trace:
 [c060cd9b] ? pppoatm_send+0x6b/0x300
 [c043a2ca] ? ppp_write+0x3a/0xe0
 [c043a228] ? ppp_channel_push+0x38/0xa0
 [c043a347] ? ppp_write+0xb7/0xe0
 [c043a290] ? ppp_channel_push+0xa0/0xa0
 [c019f72e] ? vfs_write+0x8e/0x140
 [c019f88c] ? sys_write+0x3c/0x70
 [c062ab50] ? sysenter_do_call+0x12/0x26
Code:  Bad EIP value.
EIP: [] 0x0 SS:ESP 0068:d9823f34
CR2: 
---[ end trace e29cf1805f576278 ]---
Kernel panic - not syncing: Fatal exception in interrupt

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

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 226dca9..f27a07a 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -406,6 +406,8 @@ static int pppoatm_ioctl(struct socket *sock, unsigned int 
cmd,
return -ENOIOCTLCMD;
if (!capable(CAP_NET_ADMIN))
return -EPERM;
+   if (sock-state != SS_CONNECTED)
+   return -EINVAL;
return pppoatm_assign_vcc(atmvcc, argp);
}
case PPPIOCGCHAN:
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-11-01 Thread chas williams - CONTRACTOR 
On Wed, 31 Oct 2012 23:04:35 +0100
Krzysztof Mazur  wrote:

> 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.

>   - 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-11-01 Thread chas williams - CONTRACTOR 
On Wed, 31 Oct 2012 23:04:35 +0100
Krzysztof Mazur krzys...@podlesie.net wrote:

 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.

   - 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.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Wed, Oct 31, 2012 at 04:03:52PM -0400, chas williams - CONTRACTOR wrote:
> 
> reversing the order of the push and close certainly seems like the right
> thing to do. i would like to see if it would fix your problem.  making
> the minimal change to get something working would be preferred before
> adding additional complexity.  i am just surprised we havent seen this
> bug before.

Yes, it fixes the problem and it's probably the best fix for original
issue.

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,

- missing check for SS_CONNECTED in pppoatm_ioctl,

- problem described in comment in pppoatm_may_send() when
  sk->sk_sndbuf < MTU, sk_wmem_alloc_get() should be added
  there

but I think that for now the patch that changes the order of push
and close is sufficient.

I probably saw that bug a log time ago (around 2.6.30), but it was
too rare to see what caused panic, but after 
9d02daf754238adac48fa075ee79e7edd3d79ed3 (pppoatm: Fix excessive queue bloat)
this bug occurs much more frequently.

Thanks.

Krzysiek

-- >8 --
Subject: [PATCH] atm: detach protocol before closing vcc

The vcc_destroy_socket() closes vcc before the protocol is detached
from vcc by calling vcc->push() with NULL skb. This leaves some time
window, where the protocol may call vcc->send() on closed vcc.

It happens at least with pppoatm protocol and usbatm driver, and causes
an Oops:

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

Now the protocol is detached before vcc is closed.

Signed-off-by: Krzysztof Mazur 
---
 net/atm/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/atm/common.c b/net/atm/common.c
index 0c0ad93..a0e4411 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -171,10 +171,10 @@ static void vcc_destroy_socket(struct sock *sk)
set_bit(ATM_VF_CLOSE, >flags);
clear_bit(ATM_VF_READY, >flags);
if (vcc->dev) {
-   if (vcc->dev->ops->close)
-   vcc->dev->ops->close(vcc);
if (vcc->push)
vcc->push(vcc, NULL); /* atmarpd has no push */
+   if (vcc->dev->ops->close)
+   vcc->dev->ops->close(vcc);
 
while ((skb = skb_dequeue(>sk_receive_queue)) != NULL) {
atm_return(vcc, skb->truesize);
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread chas williams - CONTRACTOR 
On Wed, 31 Oct 2012 10:41:47 +0100
Krzysztof Mazur  wrote:

> On Tue, Oct 30, 2012 at 07:20:01PM +0100, Krzysztof Mazur wrote:
> > Yes, this problem can be probably fixed by reversing close and push
> > and adding some synchronization to pppoatm_unassign_vcc(), but I think
> > we need that locking anyway, for instance for synchronization for
> > checking and incrementing sk->sk_wmem_alloc, between pppoatm_send()
> > and vcc_sendmsg().
> > 
> 
> I think that the same problem exists in other drivers (net/atm/br2684.c,
> net/atm/clip.c, maybe other).
>
> Reversing order of close() and push(vcc, NULL) operations seems to
> be a good idea, but synchronization with push(vcc, NULL)
> and function that calls vcc->send() must be added to all drivers.

this was the scheme that was (and is) currently in place.  detaching a
protocol from the atm layer never had a separate function, so it was
decided at some point to just push a NULL skb as a signal to the next
layer that i needed to cleanly shutdown and detach.  the push(vcc,
NULL) always happens in a sleepable context, so waiting for whatever
attached protocol scheduler to finish up is not a problem.  after the
pushing of the skb NULL, the attached protocol should not send or recv
any data on that vcc.

reversing the order of the push and close certainly seems like the right
thing to do. i would like to see if it would fix your problem.  making
the minimal change to get something working would be preferred before
adding additional complexity.  i am just surprised we havent seen this
bug before.

> I think it's better to just use ATM socket lock - lock_sock(sk_atm(vcc)),
> it will fix also problems with synchronization with vcc_sendmsg()
> and possibly other functions (ioctl?).
> 
> I think that we should add a wrapper to vcc->send(), based on
> fixed pppoatm_send(), that performs required checks and takes the ATM socket
> lock.
> 
> But I think we should reverse those operations anyway, because some
> drivers may use other locks, not ATM socket lock, for proper
> synchronization.

i dont think this is a bad idea.  vcc_release_async() could happen
(this would be a bit unusual for a pvc but removing the usbatm device
would do this) and there is no point in sending on a vcc that is
closing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread David Woodhouse 
On Wed, 2012-10-31 at 12:30 +0100, Krzysztof Mazur wrote:
> Yes, original patch had also the same problem with sock_owned_by_user(),
> so I just incorrectly assumed that we can do "goto nospace" after
> pppoatm_may_send(), but ppooatm_may_send() must be the last test.
> 
> So I just moved all other tests earlier and and now pppoatm_may_send()
> is also protected by ATM socket lock as you suggested earlier.

I don't think that's sufficient. When we return zero from
pppoatm_send(), the generic PPP code considers the channel to be
blocked, and it won't send any more data to it, ever, until we call
ppp_output_wakeup(). Which we do from a tasklet, triggered in
pppoatm_pop() *iff* the BLOCKED flag is set.

So we play silly buggers in pppoatm_may_send() to ensure that *if* we're
going to return zero, we make damn sure the BLOCKED flag is set and that
pppoatm_pop() is going to see that it's set. There are extensive
comments in pppoatm_pop() and pppoatm_may_send() which try to explain
this. It works because there's *always* going to be packet in flight if
we say that the sk_wmem is full, so of course there's *always* going to
be a later call to pppoatm_pop() to wake things up.

However, if you're going to return zero from pppoatm_send() when
sock_owned_by_user() is true, what guarantees that ppp_output_wakeup()
will ever be called?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Wed, Oct 31, 2012 at 10:16:18AM +, David Woodhouse wrote:
> 
> Does this break the pvcc->blocked handling that coordinates with
> pppoatm_pop()?
> 
> If we have one packet in flight, so pppoatm_may_send() permits a new one
> to be queued... but they're *large* packets to sk_wmem_alloc doesn't
> permit it. Immediately after the check, pppoatm_pop() runs and leaves
> the queue empty. We return zero, blocking the queue??? which never gets
> woken because we didn't set the BLOCKED flag and thus the tasklet never
> runs.
> 
> In fact, I think we need the BLOCKED handling for the
> sock_owned_by_user() case too? When the VCC is actually closed, I
> suppose that's not recoverable and we don't care about waking the queue
> anyway? But any time we end up returning zero from pppoatm_send(), we
> *need* to ensure that a wakeup will happen in future unless the socket
> is actually dead.
> 

Yes, original patch had also the same problem with sock_owned_by_user(),
so I just incorrectly assumed that we can do "goto nospace" after
pppoatm_may_send(), but ppooatm_may_send() must be the last test.

So I just moved all other tests earlier and and now pppoatm_may_send()
is also protected by ATM socket lock as you suggested earlier.

Krzysiek

-- >8 --
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 
---
 net/atm/pppoatm.c | 27 +++
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 0dcb5dc..eb76bd3 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -270,11 +270,22 @@ 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, >flags)
+   || test_bit(ATM_VF_CLOSE, >flags)
+   || !test_bit(ATM_VF_READY, >flags))
+   goto nospace;
+
switch (pvcc->encaps) { /* LLC encapsulation needed */
case e_llc:
if (skb_headroom(skb) < LLC_LEN) {
@@ -287,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);
@@ -298,24 +311,22 @@ 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;
}
 
-   vcc = ATM_SKB(skb)->vcc;
-   if (test_bit(ATM_VF_RELEASED, >flags)
-   || test_bit(ATM_VF_CLOSE, >flags)
-   || !test_bit(ATM_VF_READY, >flags))
-   goto nospace;
-
atomic_add(skb->truesize, _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:
+   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
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Wed, Oct 31, 2012 at 10:41:47AM +0100, Krzysztof Mazur wrote:
> 
> I think that we should add a wrapper to vcc->send(), based on
> fixed pppoatm_send(), that performs required checks and takes the ATM socket
> lock.
> 

I'm sending initial version of such wrapper and update to pppoatm.
Untested but the code is just copied from pppoatm_send.

In final series I will fix some old _atm(ATM_SKB(skb)->vcc)-like
code from original version, before moving to vcc_send_bh(), but
it's just an initial idea for some comments.

Krzysiek

diff --git a/net/atm/common.c b/net/atm/common.c
index 0c0ad93..e0602d2 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -558,6 +558,32 @@ int vcc_recvmsg(struct kiocb *iocb, struct socket *sock, 
struct msghdr *msg,
return copied;
 }
 
+int vcc_send_bh(struct atm_vcc *vcc, struct sk_buff *skb)
+{
+   int ret;
+
+   bh_lock_sock(sk_atm(vcc));
+   ret = -EAGAIN;
+   if (sock_owned_by_user(sk_atm(vcc)))
+   goto out;
+   if (test_bit(ATM_VF_RELEASED, >flags)
+   || test_bit(ATM_VF_CLOSE, >flags)
+   || !test_bit(ATM_VF_READY, >flags))
+   goto out;
+
+   if (sk_wmem_alloc_get(sk_atm(vcc)) && !atm_may_send(vcc, skb->truesize))
+   goto out;
+
+   atomic_add(skb->truesize, _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);
+   ret = ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb);
+out:
+   bh_unlock_sock(sk_atm(vcc));
+   return ret;
+}
+
 int vcc_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *m,
size_t total_len)
 {
diff --git a/net/atm/common.h b/net/atm/common.h
index cc3c2da..3a1c340 100644
--- a/net/atm/common.h
+++ b/net/atm/common.h
@@ -15,6 +15,7 @@ int vcc_release(struct socket *sock);
 int vcc_connect(struct socket *sock, int itf, short vpi, int vci);
 int vcc_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg,
size_t size, int flags);
+int vcc_send_bh(struct atm_vcc *vcc, struct sk_buff *skb);
 int vcc_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *m,
size_t total_len);
 unsigned int vcc_poll(struct file *file, struct socket *sock, poll_table 
*wait);
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 5fc335a..7612f18 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -296,31 +296,10 @@ 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, >flags)
-   || test_bit(ATM_VF_CLOSE, >flags)
-   || !test_bit(ATM_VF_READY, >flags))
-   goto nospace_unlock_sock;
-
-   /*
-* It's not clear that we need to bother with using atm_may_send()
-* to check we don't exceed sk->sk_sndbuf.
-*/
-   if (sk_wmem_alloc_get(sk_atm(vcc)) && !atm_may_send(vcc, skb->truesize))
-   goto nospace_unlock_sock;
-
-   atomic_add(skb->truesize, _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);
-   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));
+   ret = vcc_send_bh(vcc, skb);
+   if (ret == -EAGAIN)
+   goto nospace;
+   return ret ? DROP_PACKET : 1;
 nospace:
/*
 * We don't have space to send this SKB now, but we might have
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread David Woodhouse 
On Tue, 2012-10-30 at 20:52 +0100, Krzysztof Mazur wrote:
> 
> --- a/net/atm/pppoatm.c
> +++ b/net/atm/pppoatm.c
> @@ -306,12 +306,9 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
> sk_buff *skb)
>  
> /*
>  * It's not clear that we need to bother with using atm_may_send()
> -* to check we don't exceed sk->sk_sndbuf. If userspace sets a
> -* value of sk_sndbuf which is lower than the MTU, we're going to
> -* block for ever. But the code always did that before we introduced
> -* the packet count limit, so...
> +* to check we don't exceed sk->sk_sndbuf.
>  */
> -   if (!atm_may_send(vcc, skb->truesize))
> +   if (sk_wmem_alloc_get(sk_atm(vcc)) && !atm_may_send(vcc, 
> skb->truesize))
> goto nospace_unlock_sock;

Does this break the pvcc->blocked handling that coordinates with
pppoatm_pop()?

If we have one packet in flight, so pppoatm_may_send() permits a new one
to be queued... but they're *large* packets to sk_wmem_alloc doesn't
permit it. Immediately after the check, pppoatm_pop() runs and leaves
the queue empty. We return zero, blocking the queue… which never gets
woken because we didn't set the BLOCKED flag and thus the tasklet never
runs.

In fact, I think we need the BLOCKED handling for the
sock_owned_by_user() case too? When the VCC is actually closed, I
suppose that's not recoverable and we don't care about waking the queue
anyway? But any time we end up returning zero from pppoatm_send(), we
*need* to ensure that a wakeup will happen in future unless the socket
is actually dead.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 07:20:01PM +0100, Krzysztof Mazur wrote:
> On Tue, Oct 30, 2012 at 10:26:46AM -0400, Chas Williams (CONTRACTOR) wrote:
> > In message 
> > <1350926091-12642-2-git-send-email-krzys...@podlesie.net>,Krzysztof Mazur 
> > writes:
> > 
> > as i recall from way back, this shouldnt be necessary.  closing a vcc
> > for an attached protocol isnt supposed to require addtional locking
> > or synchronization.
> 
> Such locking is already used by vcc_sendmsg() and I think we should do here
> exacly what vcc_sendmsg() does.
> 
> > 
> > vcc_release() locks the socket and vcc_destroy_socket() calls the device's
> > vcc close routine and pushes a NULL skb to the attached protocol.
> > this NULL push is supposed to let the attached protocol that no more
> > sends and recvs can be handled. 
> > 
> > that said, the order for the device vcc close and push does seem
> > reversed.  since i imagine there could be a pending pppoatm_send()
> > during this interval.  the push of the NULL skb is allowed to wait for
> > the subprotocol to finish its cleanup/shutdown.
> 
> Yes, this problem can be probably fixed by reversing close and push
> and adding some synchronization to pppoatm_unassign_vcc(), but I think
> we need that locking anyway, for instance for synchronization for
> checking and incrementing sk->sk_wmem_alloc, between pppoatm_send()
> and vcc_sendmsg().
> 

I think that the same problem exists in other drivers (net/atm/br2684.c,
net/atm/clip.c, maybe other).

Reversing order of close() and push(vcc, NULL) operations seems to
be a good idea, but synchronization with push(vcc, NULL)
and function that calls vcc->send() must be added to all drivers.
I think it's better to just use ATM socket lock - lock_sock(sk_atm(vcc)),
it will fix also problems with synchronization with vcc_sendmsg()
and possibly other functions (ioctl?).

I think that we should add a wrapper to vcc->send(), based on
fixed pppoatm_send(), that performs required checks and takes the ATM socket
lock.

But I think we should reverse those operations anyway, because some
drivers may use other locks, not ATM socket lock, for proper
synchronization.

Krzysiek

-- >8 --
diff --git a/net/atm/common.c b/net/atm/common.c
index 0c0ad93..a0e4411 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -171,10 +171,10 @@ static void vcc_destroy_socket(struct sock *sk)
set_bit(ATM_VF_CLOSE, >flags);
clear_bit(ATM_VF_READY, >flags);
if (vcc->dev) {
-   if (vcc->dev->ops->close)
-   vcc->dev->ops->close(vcc);
if (vcc->push)
vcc->push(vcc, NULL); /* atmarpd has no push */
+   if (vcc->dev->ops->close)
+   vcc->dev->ops->close(vcc);
 
while ((skb = skb_dequeue(>sk_receive_queue)) != NULL) {
atm_return(vcc, skb->truesize);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 07:20:01PM +0100, Krzysztof Mazur wrote:
 On Tue, Oct 30, 2012 at 10:26:46AM -0400, Chas Williams (CONTRACTOR) wrote:
  In message 
  1350926091-12642-2-git-send-email-krzys...@podlesie.net,Krzysztof Mazur 
  writes:
  
  as i recall from way back, this shouldnt be necessary.  closing a vcc
  for an attached protocol isnt supposed to require addtional locking
  or synchronization.
 
 Such locking is already used by vcc_sendmsg() and I think we should do here
 exacly what vcc_sendmsg() does.
 
  
  vcc_release() locks the socket and vcc_destroy_socket() calls the device's
  vcc close routine and pushes a NULL skb to the attached protocol.
  this NULL push is supposed to let the attached protocol that no more
  sends and recvs can be handled. 
  
  that said, the order for the device vcc close and push does seem
  reversed.  since i imagine there could be a pending pppoatm_send()
  during this interval.  the push of the NULL skb is allowed to wait for
  the subprotocol to finish its cleanup/shutdown.
 
 Yes, this problem can be probably fixed by reversing close and push
 and adding some synchronization to pppoatm_unassign_vcc(), but I think
 we need that locking anyway, for instance for synchronization for
 checking and incrementing sk-sk_wmem_alloc, between pppoatm_send()
 and vcc_sendmsg().
 

I think that the same problem exists in other drivers (net/atm/br2684.c,
net/atm/clip.c, maybe other).

Reversing order of close() and push(vcc, NULL) operations seems to
be a good idea, but synchronization with push(vcc, NULL)
and function that calls vcc-send() must be added to all drivers.
I think it's better to just use ATM socket lock - lock_sock(sk_atm(vcc)),
it will fix also problems with synchronization with vcc_sendmsg()
and possibly other functions (ioctl?).

I think that we should add a wrapper to vcc-send(), based on
fixed pppoatm_send(), that performs required checks and takes the ATM socket
lock.

But I think we should reverse those operations anyway, because some
drivers may use other locks, not ATM socket lock, for proper
synchronization.

Krzysiek

-- 8 --
diff --git a/net/atm/common.c b/net/atm/common.c
index 0c0ad93..a0e4411 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -171,10 +171,10 @@ static void vcc_destroy_socket(struct sock *sk)
set_bit(ATM_VF_CLOSE, vcc-flags);
clear_bit(ATM_VF_READY, vcc-flags);
if (vcc-dev) {
-   if (vcc-dev-ops-close)
-   vcc-dev-ops-close(vcc);
if (vcc-push)
vcc-push(vcc, NULL); /* atmarpd has no push */
+   if (vcc-dev-ops-close)
+   vcc-dev-ops-close(vcc);
 
while ((skb = skb_dequeue(sk-sk_receive_queue)) != NULL) {
atm_return(vcc, skb-truesize);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread David Woodhouse 
On Tue, 2012-10-30 at 20:52 +0100, Krzysztof Mazur wrote:
 
 --- a/net/atm/pppoatm.c
 +++ b/net/atm/pppoatm.c
 @@ -306,12 +306,9 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
 sk_buff *skb)
  
 /*
  * It's not clear that we need to bother with using atm_may_send()
 -* to check we don't exceed sk-sk_sndbuf. If userspace sets a
 -* value of sk_sndbuf which is lower than the MTU, we're going to
 -* block for ever. But the code always did that before we introduced
 -* the packet count limit, so...
 +* to check we don't exceed sk-sk_sndbuf.
  */
 -   if (!atm_may_send(vcc, skb-truesize))
 +   if (sk_wmem_alloc_get(sk_atm(vcc))  !atm_may_send(vcc, 
 skb-truesize))
 goto nospace_unlock_sock;

Does this break the pvcc-blocked handling that coordinates with
pppoatm_pop()?

If we have one packet in flight, so pppoatm_may_send() permits a new one
to be queued... but they're *large* packets to sk_wmem_alloc doesn't
permit it. Immediately after the check, pppoatm_pop() runs and leaves
the queue empty. We return zero, blocking the queue… which never gets
woken because we didn't set the BLOCKED flag and thus the tasklet never
runs.

In fact, I think we need the BLOCKED handling for the
sock_owned_by_user() case too? When the VCC is actually closed, I
suppose that's not recoverable and we don't care about waking the queue
anyway? But any time we end up returning zero from pppoatm_send(), we
*need* to ensure that a wakeup will happen in future unless the socket
is actually dead.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Wed, Oct 31, 2012 at 10:41:47AM +0100, Krzysztof Mazur wrote:
 
 I think that we should add a wrapper to vcc-send(), based on
 fixed pppoatm_send(), that performs required checks and takes the ATM socket
 lock.
 

I'm sending initial version of such wrapper and update to pppoatm.
Untested but the code is just copied from pppoatm_send.

In final series I will fix some old sk_atm(ATM_SKB(skb)-vcc)-like
code from original version, before moving to vcc_send_bh(), but
it's just an initial idea for some comments.

Krzysiek

diff --git a/net/atm/common.c b/net/atm/common.c
index 0c0ad93..e0602d2 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -558,6 +558,32 @@ int vcc_recvmsg(struct kiocb *iocb, struct socket *sock, 
struct msghdr *msg,
return copied;
 }
 
+int vcc_send_bh(struct atm_vcc *vcc, struct sk_buff *skb)
+{
+   int ret;
+
+   bh_lock_sock(sk_atm(vcc));
+   ret = -EAGAIN;
+   if (sock_owned_by_user(sk_atm(vcc)))
+   goto out;
+   if (test_bit(ATM_VF_RELEASED, vcc-flags)
+   || test_bit(ATM_VF_CLOSE, vcc-flags)
+   || !test_bit(ATM_VF_READY, vcc-flags))
+   goto out;
+
+   if (sk_wmem_alloc_get(sk_atm(vcc))  !atm_may_send(vcc, skb-truesize))
+   goto out;
+
+   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);
+   ret = ATM_SKB(skb)-vcc-send(ATM_SKB(skb)-vcc, skb);
+out:
+   bh_unlock_sock(sk_atm(vcc));
+   return ret;
+}
+
 int vcc_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *m,
size_t total_len)
 {
diff --git a/net/atm/common.h b/net/atm/common.h
index cc3c2da..3a1c340 100644
--- a/net/atm/common.h
+++ b/net/atm/common.h
@@ -15,6 +15,7 @@ int vcc_release(struct socket *sock);
 int vcc_connect(struct socket *sock, int itf, short vpi, int vci);
 int vcc_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg,
size_t size, int flags);
+int vcc_send_bh(struct atm_vcc *vcc, struct sk_buff *skb);
 int vcc_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *m,
size_t total_len);
 unsigned int vcc_poll(struct file *file, struct socket *sock, poll_table 
*wait);
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 5fc335a..7612f18 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -296,31 +296,10 @@ 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_unlock_sock;
-
-   /*
-* It's not clear that we need to bother with using atm_may_send()
-* to check we don't exceed sk-sk_sndbuf.
-*/
-   if (sk_wmem_alloc_get(sk_atm(vcc))  !atm_may_send(vcc, skb-truesize))
-   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);
-   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));
+   ret = vcc_send_bh(vcc, skb);
+   if (ret == -EAGAIN)
+   goto nospace;
+   return ret ? DROP_PACKET : 1;
 nospace:
/*
 * We don't have space to send this SKB now, but we might have
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Wed, Oct 31, 2012 at 10:16:18AM +, David Woodhouse wrote:
 
 Does this break the pvcc-blocked handling that coordinates with
 pppoatm_pop()?
 
 If we have one packet in flight, so pppoatm_may_send() permits a new one
 to be queued... but they're *large* packets to sk_wmem_alloc doesn't
 permit it. Immediately after the check, pppoatm_pop() runs and leaves
 the queue empty. We return zero, blocking the queue??? which never gets
 woken because we didn't set the BLOCKED flag and thus the tasklet never
 runs.
 
 In fact, I think we need the BLOCKED handling for the
 sock_owned_by_user() case too? When the VCC is actually closed, I
 suppose that's not recoverable and we don't care about waking the queue
 anyway? But any time we end up returning zero from pppoatm_send(), we
 *need* to ensure that a wakeup will happen in future unless the socket
 is actually dead.
 

Yes, original patch had also the same problem with sock_owned_by_user(),
so I just incorrectly assumed that we can do goto nospace after
pppoatm_may_send(), but ppooatm_may_send() must be the last test.

So I just moved all other tests earlier and and now pppoatm_may_send()
is also protected by ATM socket lock as you suggested earlier.

Krzysiek

-- 8 --
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 krzys...@podlesie.net
---
 net/atm/pppoatm.c | 27 +++
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 0dcb5dc..eb76bd3 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -270,11 +270,22 @@ 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) {
@@ -287,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);
@@ -298,24 +311,22 @@ 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;
}
 
-   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,
 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
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread David Woodhouse 
On Wed, 2012-10-31 at 12:30 +0100, Krzysztof Mazur wrote:
 Yes, original patch had also the same problem with sock_owned_by_user(),
 so I just incorrectly assumed that we can do goto nospace after
 pppoatm_may_send(), but ppooatm_may_send() must be the last test.
 
 So I just moved all other tests earlier and and now pppoatm_may_send()
 is also protected by ATM socket lock as you suggested earlier.

I don't think that's sufficient. When we return zero from
pppoatm_send(), the generic PPP code considers the channel to be
blocked, and it won't send any more data to it, ever, until we call
ppp_output_wakeup(). Which we do from a tasklet, triggered in
pppoatm_pop() *iff* the BLOCKED flag is set.

So we play silly buggers in pppoatm_may_send() to ensure that *if* we're
going to return zero, we make damn sure the BLOCKED flag is set and that
pppoatm_pop() is going to see that it's set. There are extensive
comments in pppoatm_pop() and pppoatm_may_send() which try to explain
this. It works because there's *always* going to be packet in flight if
we say that the sk_wmem is full, so of course there's *always* going to
be a later call to pppoatm_pop() to wake things up.

However, if you're going to return zero from pppoatm_send() when
sock_owned_by_user() is true, what guarantees that ppp_output_wakeup()
will ever be called?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread chas williams - CONTRACTOR 
On Wed, 31 Oct 2012 10:41:47 +0100
Krzysztof Mazur krzys...@podlesie.net wrote:

 On Tue, Oct 30, 2012 at 07:20:01PM +0100, Krzysztof Mazur wrote:
  Yes, this problem can be probably fixed by reversing close and push
  and adding some synchronization to pppoatm_unassign_vcc(), but I think
  we need that locking anyway, for instance for synchronization for
  checking and incrementing sk-sk_wmem_alloc, between pppoatm_send()
  and vcc_sendmsg().
  
 
 I think that the same problem exists in other drivers (net/atm/br2684.c,
 net/atm/clip.c, maybe other).

 Reversing order of close() and push(vcc, NULL) operations seems to
 be a good idea, but synchronization with push(vcc, NULL)
 and function that calls vcc-send() must be added to all drivers.

this was the scheme that was (and is) currently in place.  detaching a
protocol from the atm layer never had a separate function, so it was
decided at some point to just push a NULL skb as a signal to the next
layer that i needed to cleanly shutdown and detach.  the push(vcc,
NULL) always happens in a sleepable context, so waiting for whatever
attached protocol scheduler to finish up is not a problem.  after the
pushing of the skb NULL, the attached protocol should not send or recv
any data on that vcc.

reversing the order of the push and close certainly seems like the right
thing to do. i would like to see if it would fix your problem.  making
the minimal change to get something working would be preferred before
adding additional complexity.  i am just surprised we havent seen this
bug before.

 I think it's better to just use ATM socket lock - lock_sock(sk_atm(vcc)),
 it will fix also problems with synchronization with vcc_sendmsg()
 and possibly other functions (ioctl?).
 
 I think that we should add a wrapper to vcc-send(), based on
 fixed pppoatm_send(), that performs required checks and takes the ATM socket
 lock.
 
 But I think we should reverse those operations anyway, because some
 drivers may use other locks, not ATM socket lock, for proper
 synchronization.

i dont think this is a bad idea.  vcc_release_async() could happen
(this would be a bit unusual for a pvc but removing the usbatm device
would do this) and there is no point in sending on a vcc that is
closing.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-31 Thread Krzysztof Mazur 
On Wed, Oct 31, 2012 at 04:03:52PM -0400, chas williams - CONTRACTOR wrote:
 
 reversing the order of the push and close certainly seems like the right
 thing to do. i would like to see if it would fix your problem.  making
 the minimal change to get something working would be preferred before
 adding additional complexity.  i am just surprised we havent seen this
 bug before.

Yes, it fixes the problem and it's probably the best fix for original
issue.

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,

- missing check for SS_CONNECTED in pppoatm_ioctl,

- problem described in comment in pppoatm_may_send() when
  sk-sk_sndbuf  MTU, sk_wmem_alloc_get() should be added
  there

but I think that for now the patch that changes the order of push
and close is sufficient.

I probably saw that bug a log time ago (around 2.6.30), but it was
too rare to see what caused panic, but after 
9d02daf754238adac48fa075ee79e7edd3d79ed3 (pppoatm: Fix excessive queue bloat)
this bug occurs much more frequently.

Thanks.

Krzysiek

-- 8 --
Subject: [PATCH] atm: detach protocol before closing vcc

The vcc_destroy_socket() closes vcc before the protocol is detached
from vcc by calling vcc-push() with NULL skb. This leaves some time
window, where the protocol may call vcc-send() on closed vcc.

It happens at least with pppoatm protocol and usbatm driver, and causes
an Oops:

Oops:  [#1] PREEMPT
Pid: 0, comm: swapper Not tainted 3.6.0-krzysiek-1-gb7cd93b-dirty #60
/AK32
EIP: 0060:[c01413c6] EFLAGS: 00010082 CPU: 0
EIP is at __wake_up_common+0x16/0x70
EAX: 30707070 EBX: 0292 ECX: 0001 EDX: dca75fc0
ESI:  EDI: de7f500f EBP: df409f24 ESP: df409f08
 DS: 007b ES: 007b FS:  GS:  SS: 0068
CR0: 8005003b CR2: 30707070 CR3: 1c92 CR4: 07d0
DR0:  DR1:  DR2:  DR3: 
DR6: 0ff0 DR7: 0400
Process swapper (pid: 0, ti=df408000 task=c07bd4e0 task.ti=c07b)
Stack:
  0001 0001 dca75fc0 0292  de7f500f df409f3c
 c0143299   dc84f000 dc84f000 df409f4c c0602bf0 
 dc84f000 df409f58 c0604301 dc840cc0 df409fb4 c04672e5 c076a240 
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 the protocol is detached before vcc is closed.

Signed-off-by: Krzysztof Mazur krzys...@podlesie.net
---
 net/atm/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/atm/common.c b/net/atm/common.c
index 0c0ad93..a0e4411 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -171,10 +171,10 @@ static void vcc_destroy_socket(struct sock *sk)
set_bit(ATM_VF_CLOSE, vcc-flags);
clear_bit(ATM_VF_READY, vcc-flags);
if (vcc-dev) {
-   if (vcc-dev-ops-close)
-   vcc-dev-ops-close(vcc);
if (vcc-push)
vcc-push(vcc, NULL); /* atmarpd has no push */
+   if (vcc-dev-ops-close)
+   vcc-dev-ops-close(vcc);
 
while ((skb = skb_dequeue(sk-sk_receive_queue)) != NULL) {
atm_return(vcc, skb-truesize);
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 08:07:25PM +0100, Krzysztof Mazur wrote:
> On Tue, Oct 30, 2012 at 09:37:48AM +, David Woodhouse wrote:
> > 
> > Should we be locking it earlier, so that the atm_may_send() call is also
> > covered by the lock?
> 
> Yes, but only to protect against concurent vcc_sendmsg().
> 
> > 
> > Either way, it's an obvious improvement on what we had before ??? and even
> > if the answer to my question above is 'yes', exceeding the configured
> > size by one packet is both harmless and almost never going to happen
> > since we now limit ourselves to two packets anyway. So:
> > 
> > Acked-By: David Woodhouse 
> > 
> 

David, I think we should also fix the issue with sk_sndbuf < MTU,
which is described in comment in pppoatm_may_send() added by
your "pppoatm: Fix excessive queue bloat" patch.

The vcc_sendmsg() already does that.

Krzysiek

-- >8 --
Subject: [PATCH] pppoatm: fix sending packets when sk_sndbuf < MTU

Now pppoatm_send() works, when sk_sndbuf is smaller than MTU. This
issue was already pointed in comment:

/*
 * It's not clear that we need to bother with using atm_may_send()
 * to check we don't exceed sk->sk_sndbuf. If userspace sets a
 * value of sk_sndbuf which is lower than the MTU, we're going to
 * block for ever. But the code always did that before we introduced
 * the packet count limit, so...
 */

The test is copied from alloc_tx() which is used by vcc_sendmsg().

Signed-off-by: Krzysztof Mazur 
---
 net/atm/pppoatm.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 4cc81b5..f25536b 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -306,12 +306,9 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
sk_buff *skb)
 
/*
 * It's not clear that we need to bother with using atm_may_send()
-* to check we don't exceed sk->sk_sndbuf. If userspace sets a
-* value of sk_sndbuf which is lower than the MTU, we're going to
-* block for ever. But the code always did that before we introduced
-* the packet count limit, so...
+* to check we don't exceed sk->sk_sndbuf.
 */
-   if (!atm_may_send(vcc, skb->truesize))
+   if (sk_wmem_alloc_get(sk_atm(vcc)) && !atm_may_send(vcc, skb->truesize))
goto nospace_unlock_sock;
 
atomic_add(skb->truesize, _atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 09:37:48AM +, David Woodhouse wrote:
> 
> Should we be locking it earlier, so that the atm_may_send() call is also
> covered by the lock?

Yes, but only to protect against concurent vcc_sendmsg().

> 
> Either way, it's an obvious improvement on what we had before ??? and even
> if the answer to my question above is 'yes', exceeding the configured
> size by one packet is both harmless and almost never going to happen
> since we now limit ourselves to two packets anyway. So:
> 
> Acked-By: David Woodhouse 
> 

I'm sending proposed patch (not tested).

Should I squash it into original patch or send it later because it's
not really important?

Thanks.

Krzysiek

-- >8 --
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index a766d96..3081834 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -214,15 +214,7 @@ error:
 
 static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
 {
-   /*
-* It's not clear that we need to bother with using atm_may_send()
-* to check we don't exceed sk->sk_sndbuf. If userspace sets a
-* value of sk_sndbuf which is lower than the MTU, we're going to
-* block for ever. But the code always did that before we introduced
-* the packet count limit, so...
-*/
-   if (atm_may_send(pvcc->atmvcc, size) &&
-   atomic_inc_not_zero_hint(>inflight, NONE_INFLIGHT))
+   if (atomic_inc_not_zero_hint(>inflight, NONE_INFLIGHT))
return 1;
 
/*
@@ -251,8 +243,7 @@ static inline int pppoatm_may_send(struct pppoatm_vcc 
*pvcc, int size)
 * code path that calls pppoatm_send(), and is thus going to
 * wait for us to finish.
 */
-   if (atm_may_send(pvcc->atmvcc, size) &&
-   atomic_inc_not_zero(>inflight))
+   if (atomic_inc_not_zero(>inflight))
return 1;
 
return 0;
@@ -314,6 +305,16 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
sk_buff *skb)
|| !test_bit(ATM_VF_READY, >flags))
goto nospace_unlock_sock;
 
+   /*
+* It's not clear that we need to bother with using atm_may_send()
+* to check we don't exceed sk->sk_sndbuf. If userspace sets a
+* value of sk_sndbuf which is lower than the MTU, we're going to
+* block for ever. But the code always did that before we introduced
+* the packet count limit, so...
+*/
+   if (!atm_may_send(vcc, skb->truesize))
+   goto nospace_unlock_sock;
+
atomic_add(skb->truesize, _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",
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 10:26:46AM -0400, Chas Williams (CONTRACTOR) wrote:
> In message 
> <1350926091-12642-2-git-send-email-krzys...@podlesie.net>,Krzysztof Mazur 
> writes:
> 
> as i recall from way back, this shouldnt be necessary.  closing a vcc
> for an attached protocol isnt supposed to require addtional locking
> or synchronization.

Such locking is already used by vcc_sendmsg() and I think we should do here
exacly what vcc_sendmsg() does.

> 
> vcc_release() locks the socket and vcc_destroy_socket() calls the device's
> vcc close routine and pushes a NULL skb to the attached protocol.
> this NULL push is supposed to let the attached protocol that no more
> sends and recvs can be handled. 
> 
> that said, the order for the device vcc close and push does seem
> reversed.  since i imagine there could be a pending pppoatm_send()
> during this interval.  the push of the NULL skb is allowed to wait for
> the subprotocol to finish its cleanup/shutdown.

Yes, this problem can be probably fixed by reversing close and push
and adding some synchronization to pppoatm_unassign_vcc(), but I think
we need that locking anyway, for instance for synchronization for
checking and incrementing sk->sk_wmem_alloc, between pppoatm_send()
and vcc_sendmsg().

Thanks.

Krzysiek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Chas Williams (CONTRACTOR) 
In message <1350926091-12642-2-git-send-email-krzys...@podlesie.net>,Krzysztof 
Mazur writes:
>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.

as i recall from way back, this shouldnt be necessary.  closing a vcc
for an attached protocol isnt supposed to require addtional locking
or synchronization.

vcc_release() locks the socket and vcc_destroy_socket() calls the device's
vcc close routine and pushes a NULL skb to the attached protocol.
this NULL push is supposed to let the attached protocol that no more
sends and recvs can be handled. 

that said, the order for the device vcc close and push does seem
reversed.  since i imagine there could be a pending pppoatm_send()
during this interval.  the push of the NULL skb is allowed to wait for
the subprotocol to finish its cleanup/shutdown.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread David Woodhouse 
On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
> 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().

Should we be locking it earlier, so that the atm_may_send() call is also
covered by the lock?

Either way, it's an obvious improvement on what we had before — and even
if the answer to my question above is 'yes', exceeding the configured
size by one packet is both harmless and almost never going to happen
since we now limit ourselves to two packets anyway. So:

Acked-By: David Woodhouse 

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread David Woodhouse 
On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
 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().

Should we be locking it earlier, so that the atm_may_send() call is also
covered by the lock?

Either way, it's an obvious improvement on what we had before — and even
if the answer to my question above is 'yes', exceeding the configured
size by one packet is both harmless and almost never going to happen
since we now limit ourselves to two packets anyway. So:

Acked-By: David Woodhouse david.woodho...@intel.com

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Chas Williams (CONTRACTOR) 
In message 1350926091-12642-2-git-send-email-krzys...@podlesie.net,Krzysztof 
Mazur writes:
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.

as i recall from way back, this shouldnt be necessary.  closing a vcc
for an attached protocol isnt supposed to require addtional locking
or synchronization.

vcc_release() locks the socket and vcc_destroy_socket() calls the device's
vcc close routine and pushes a NULL skb to the attached protocol.
this NULL push is supposed to let the attached protocol that no more
sends and recvs can be handled. 

that said, the order for the device vcc close and push does seem
reversed.  since i imagine there could be a pending pppoatm_send()
during this interval.  the push of the NULL skb is allowed to wait for
the subprotocol to finish its cleanup/shutdown.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 10:26:46AM -0400, Chas Williams (CONTRACTOR) wrote:
 In message 
 1350926091-12642-2-git-send-email-krzys...@podlesie.net,Krzysztof Mazur 
 writes:
 
 as i recall from way back, this shouldnt be necessary.  closing a vcc
 for an attached protocol isnt supposed to require addtional locking
 or synchronization.

Such locking is already used by vcc_sendmsg() and I think we should do here
exacly what vcc_sendmsg() does.

 
 vcc_release() locks the socket and vcc_destroy_socket() calls the device's
 vcc close routine and pushes a NULL skb to the attached protocol.
 this NULL push is supposed to let the attached protocol that no more
 sends and recvs can be handled. 
 
 that said, the order for the device vcc close and push does seem
 reversed.  since i imagine there could be a pending pppoatm_send()
 during this interval.  the push of the NULL skb is allowed to wait for
 the subprotocol to finish its cleanup/shutdown.

Yes, this problem can be probably fixed by reversing close and push
and adding some synchronization to pppoatm_unassign_vcc(), but I think
we need that locking anyway, for instance for synchronization for
checking and incrementing sk-sk_wmem_alloc, between pppoatm_send()
and vcc_sendmsg().

Thanks.

Krzysiek
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 09:37:48AM +, David Woodhouse wrote:
 
 Should we be locking it earlier, so that the atm_may_send() call is also
 covered by the lock?

Yes, but only to protect against concurent vcc_sendmsg().

 
 Either way, it's an obvious improvement on what we had before ??? and even
 if the answer to my question above is 'yes', exceeding the configured
 size by one packet is both harmless and almost never going to happen
 since we now limit ourselves to two packets anyway. So:
 
 Acked-By: David Woodhouse david.woodho...@intel.com
 

I'm sending proposed patch (not tested).

Should I squash it into original patch or send it later because it's
not really important?

Thanks.

Krzysiek

-- 8 --
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index a766d96..3081834 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -214,15 +214,7 @@ error:
 
 static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
 {
-   /*
-* It's not clear that we need to bother with using atm_may_send()
-* to check we don't exceed sk-sk_sndbuf. If userspace sets a
-* value of sk_sndbuf which is lower than the MTU, we're going to
-* block for ever. But the code always did that before we introduced
-* the packet count limit, so...
-*/
-   if (atm_may_send(pvcc-atmvcc, size) 
-   atomic_inc_not_zero_hint(pvcc-inflight, NONE_INFLIGHT))
+   if (atomic_inc_not_zero_hint(pvcc-inflight, NONE_INFLIGHT))
return 1;
 
/*
@@ -251,8 +243,7 @@ static inline int pppoatm_may_send(struct pppoatm_vcc 
*pvcc, int size)
 * code path that calls pppoatm_send(), and is thus going to
 * wait for us to finish.
 */
-   if (atm_may_send(pvcc-atmvcc, size) 
-   atomic_inc_not_zero(pvcc-inflight))
+   if (atomic_inc_not_zero(pvcc-inflight))
return 1;
 
return 0;
@@ -314,6 +305,16 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
sk_buff *skb)
|| !test_bit(ATM_VF_READY, vcc-flags))
goto nospace_unlock_sock;
 
+   /*
+* It's not clear that we need to bother with using atm_may_send()
+* to check we don't exceed sk-sk_sndbuf. If userspace sets a
+* value of sk_sndbuf which is lower than the MTU, we're going to
+* block for ever. But the code always did that before we introduced
+* the packet count limit, so...
+*/
+   if (!atm_may_send(vcc, skb-truesize))
+   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,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

2012-10-30 Thread Krzysztof Mazur 
On Tue, Oct 30, 2012 at 08:07:25PM +0100, Krzysztof Mazur wrote:
 On Tue, Oct 30, 2012 at 09:37:48AM +, David Woodhouse wrote:
  
  Should we be locking it earlier, so that the atm_may_send() call is also
  covered by the lock?
 
 Yes, but only to protect against concurent vcc_sendmsg().
 
  
  Either way, it's an obvious improvement on what we had before ??? and even
  if the answer to my question above is 'yes', exceeding the configured
  size by one packet is both harmless and almost never going to happen
  since we now limit ourselves to two packets anyway. So:
  
  Acked-By: David Woodhouse david.woodho...@intel.com
  
 

David, I think we should also fix the issue with sk_sndbuf  MTU,
which is described in comment in pppoatm_may_send() added by
your pppoatm: Fix excessive queue bloat patch.

The vcc_sendmsg() already does that.

Krzysiek

-- 8 --
Subject: [PATCH] pppoatm: fix sending packets when sk_sndbuf  MTU

Now pppoatm_send() works, when sk_sndbuf is smaller than MTU. This
issue was already pointed in comment:

/*
 * It's not clear that we need to bother with using atm_may_send()
 * to check we don't exceed sk-sk_sndbuf. If userspace sets a
 * value of sk_sndbuf which is lower than the MTU, we're going to
 * block for ever. But the code always did that before we introduced
 * the packet count limit, so...
 */

The test is copied from alloc_tx() which is used by vcc_sendmsg().

Signed-off-by: Krzysztof Mazur krzys...@podlesie.net
---
 net/atm/pppoatm.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 4cc81b5..f25536b 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -306,12 +306,9 @@ static int pppoatm_send(struct ppp_channel *chan, struct 
sk_buff *skb)
 
/*
 * It's not clear that we need to bother with using atm_may_send()
-* to check we don't exceed sk-sk_sndbuf. If userspace sets a
-* value of sk_sndbuf which is lower than the MTU, we're going to
-* block for ever. But the code always did that before we introduced
-* the packet count limit, so...
+* to check we don't exceed sk-sk_sndbuf.
 */
-   if (!atm_may_send(vcc, skb-truesize))
+   if (sk_wmem_alloc_get(sk_atm(vcc))  !atm_may_send(vcc, skb-truesize))
goto nospace_unlock_sock;
 
atomic_add(skb-truesize, sk_atm(ATM_SKB(skb)-vcc)-sk_wmem_alloc);
-- 
1.8.0.172.g62af90c

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/