[PATCH v3 0/7] pppoatm: fix multiple issues with pppoatm driver

[PATCH v3 0/7] pppoatm: fix multiple issues with pppoatm driver

[Krzysztof Mazur]

Hi,

This patch series fixes multiple issues in the pppoatm driver.
Two of them can be easily triggered and appropriate Oops messages
were added.

Changes in v3:

- the original crash after closing vcc is now fixed in ATM layer
  by "[PATCH v3 1/7] atm: detach protocol before closing vcc",
  this also fixes the same issue with other ATM protocols,
  suggested by Chas Williams

- add "[PATCH v3 2/7] atm: add owner of push() callback to atmvcc"
  and "[PATCH v3 4/7] pppoatm: fix module_put() race" to remove
  assumption of Big Kernel Lock, which is no longer true.
  The same assumption exists also in other ATM protocols, but it's
  not fixed by this version.

- patch "[PATCH v3 3/7] pppoatm: allow assign only on a connected socket"
  was added to fix crash, when pppoatm was used on non fully-connected
  socket.

- the order of patches 1 and 2 from v2, now
  "[PATCH v3 5/7] pppoatm: take ATM socket lock in pppoatm_send()"
  and "[PATCH v3 6/7] pppoatm: don't send frames on not-ready vcc"
  was reversed, in "[PATCH v3 5/7] pppoatm: take ATM socket lock
  in pppoatm_send()" lock operation was moved earlier to protect
  also pppoatm_may_send(), as suggested by David Woodhouse

  Both patches were previously Acked by David Woodhouse,
  but now they are slightly modified and commit message is
  rewritten, because orignal issue is already fixed in
  "PATCH v3 1/7".

- the "[PATCH v2 3/3] pppoatm: protect against freeing of vcc"
  was dropped because it's not necessary, this protection
  is already provided by ATM layer


[v2]

"[PATCH v2 1/3] pppoatm: don't send frames to destroyed vcc"
http://marc.info/?l=linux-kernel&m=135092672625585&w=2

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

Krzysztof Mazur

[PATCH v3 1/7] atm: detach protocol before closing vcc

[Krzysztof Mazur]

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: 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 the protocol is detached before vcc is closed.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Suggested-by: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
Cc: David Woodhouse <David.Woodhouse@intel.com>
Cc: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
---
 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.233.g54991f2

[PATCH v3 2/7] atm: add owner of push() callback to atmvcc

[Krzysztof Mazur]

The atm is using atmvcc->push(vcc, NULL) callback to notify protocol
that vcc will be closed and protocol must detach from it. This callback
is usually used by protocol to decrement module usage count by module_put(),
but it leaves small window then module is still used after module_put().

Now the owner of push() callback is kept in atmvcc and
module_put(atmvcc->owner) is called after the protocol is detached from vcc.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 include/linux/atmdev.h | 1 +
 net/atm/common.c       | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 06fd4bb..31c16c6 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -315,6 +315,7 @@ struct atm_vcc {
 	void		*dev_data;	/* per-device data */
 	void		*proto_data;	/* per-protocol data */
 	struct k_atm_aal_stats *stats;	/* pointer to AAL stats group */
+	struct module *owner;		/* owner of ->push function */
 	/* SVC part --- may move later ------------------------------------- */
 	short		itf;		/* interface number */
 	struct sockaddr_atmsvc local;
diff --git a/net/atm/common.c b/net/atm/common.c
index a0e4411..ea952b2 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -156,6 +156,7 @@ int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
 	atomic_set(&sk->sk_rmem_alloc, 0);
 	vcc->push = NULL;
 	vcc->pop = NULL;
+	vcc->owner = NULL;
 	vcc->push_oam = NULL;
 	vcc->vpi = vcc->vci = 0; /* no VCI/VPI yet */
 	vcc->atm_options = vcc->aal_options = 0;
@@ -173,6 +174,7 @@ static void vcc_destroy_socket(struct sock *sk)
 	if (vcc->dev) {
 		if (vcc->push)
 			vcc->push(vcc, NULL); /* atmarpd has no push */
+		module_put(vcc->owner);
 		if (vcc->dev->ops->close)
 			vcc->dev->ops->close(vcc);
 
-- 
1.8.0.233.g54991f2

Re: [PATCH v3 2/7] atm: add owner of push() callback to atmvcc

[chas williams - CONTRACTOR]

On Tue,  6 Nov 2012 23:16:57 +0100
Krzysztof Mazur <krzysiek@podlesie.net> wrote:

> The atm is using atmvcc->push(vcc, NULL) callback to notify protocol
> that vcc will be closed and protocol must detach from it. This callback
> is usually used by protocol to decrement module usage count by module_put(),
> but it leaves small window then module is still used after module_put().
> 
> Now the owner of push() callback is kept in atmvcc and
> module_put(atmvcc->owner) is called after the protocol is detached from vcc.
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>

Acked-by: Chas Williams <chas@cmf.nrl.navy.mil>

[PATCH v3 3/7] pppoatm: allow assign only on a connected socket

[Krzysztof Mazur]

The pppoatm does not check if 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 <krzysiek@podlesie.net>
Cc: David Woodhouse <David.Woodhouse@intel.com>
Cc: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
---
Oops triggered by pppd with pppoatm plugin with removed connect():

BUG: unable to handle kernel NULL pointer dereference at   (null)
IP: [<  (null)>]   (null)
*pde = 00000000 
Oops: 0000 [#1] PREEMPT 
Pid: 4154, comm: pppd Not tainted 3.6.0-krzysiek-00002-g3ff1093 #95    /AK32 
EIP: 0060:[<00000000>] EFLAGS: 00010202 CPU: 0
EIP is at 0x0
EAX: d95f7800 EBX: d9d4ba80 ECX: d95f7800 EDX: d9d4ba80
ESI: ffffffff EDI: 00000001 EBP: 000001c0 ESP: d9823f34
 DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
CR0: 8005003b CR2: 00000000 CR3: 1e7b6000 CR4: 000007d0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
Process pppd (pid: 4154, ti=d9822000 task=d99918a0 task.ti=d9822000)
Stack:
 c060cd9b c043a2ca d99ed860 d99ed864 d9d4ba80 08094f22 c043a228 d9d4ba80
 d99ed860 0000000c c043a347 ffffffff 0000000c d94311a0 08094f22 c043a290
 c019f72e d9823f9c 00000003 09df1090 d94311a0 08094f22 00000008 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: [<00000000>] 0x0 SS:ESP 0068:d9823f34
CR2: 0000000000000000
---[ 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.233.g54991f2

[PATCH v3 4/7] pppoatm: fix module_put() race

[Krzysztof Mazur]

The pppoatm used module_put() during unassignment from vcc with
hope that we have BKL. This assumption is no longer true.

Now owner field in atmvcc is used to move this module_put()
to vcc_destroy_socket().

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

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index f27a07a..b23c672 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -60,6 +60,7 @@ struct pppoatm_vcc {
 	struct atm_vcc	*atmvcc;	/* VCC descriptor */
 	void (*old_push)(struct atm_vcc *, struct sk_buff *);
 	void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+	struct module *old_owner;
 					/* keep old push/pop for detaching */
 	enum pppoatm_encaps encaps;
 	atomic_t inflight;
@@ -155,8 +156,6 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)
 	ppp_unregister_channel(&pvcc->chan);
 	atmvcc->user_back = NULL;
 	kfree(pvcc);
-	/* Gee, I hope we have the big kernel lock here... */
-	module_put(THIS_MODULE);
 }
 
 /* Called when an AAL5 PDU comes in */
@@ -165,9 +164,13 @@ static void pppoatm_push(struct atm_vcc *atmvcc, struct sk_buff *skb)
 	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 	pr_debug("\n");
 	if (skb == NULL) {			/* VCC was closed */
+		struct module *module;
+
 		pr_debug("removing ATMPPP VCC %p\n", pvcc);
+		module = pvcc->old_owner;
 		pppoatm_unassign_vcc(atmvcc);
 		atmvcc->push(atmvcc, NULL);	/* Pass along bad news */
+		module_put(module);
 		return;
 	}
 	atm_return(atmvcc, skb->truesize);
@@ -362,6 +365,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	atomic_set(&pvcc->inflight, NONE_INFLIGHT);
 	pvcc->old_push = atmvcc->push;
 	pvcc->old_pop = atmvcc->pop;
+	pvcc->old_owner = atmvcc->owner;
 	pvcc->encaps = (enum pppoatm_encaps) be.encaps;
 	pvcc->chan.private = pvcc;
 	pvcc->chan.ops = &pppoatm_ops;
@@ -378,6 +382,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	atmvcc->push = pppoatm_push;
 	atmvcc->pop = pppoatm_pop;
 	__module_get(THIS_MODULE);
+	atmvcc->owner = THIS_MODULE;
 
 	/* re-process everything received between connection setup and
 	   backend setup */
-- 
1.8.0.233.g54991f2

[PATCH v3 5/7] pppoatm: take ATM socket lock in pppoatm_send()

[Krzysztof Mazur]

The pppoatm_send() does not take any lock that will prevent concurrent
vcc_sendmsg(). This causes two problems:

	- there is no locking between checking the send queue size
	  with atm_may_send() and incrementing sk_wmem_alloc,
	  and the real queue size can be a little higher than sk_sndbuf

	- the vcc->sendmsg() can be called concurrently. I'm not sure
	  if it's allowed. Some drivers (eni, nicstar, ...) seem
	  to assume it will never happen.

Now pppoatm_send() takes ATM socket lock, the same that is used
in vcc_sendmsg() and other ATM socket functions. The pppoatm_send()
is called with BH disabled, so bh_lock_sock() is used instead
of lock_sock().

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Cc: David Woodhouse <David.Woodhouse@intel.com>
Cc: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
---
 net/atm/pppoatm.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index b23c672..c4a57bc 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -272,10 +272,19 @@ 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;
+
 	switch (pvcc->encaps) {		/* LLC encapsulation needed */
 	case e_llc:
 		if (skb_headroom(skb) < LLC_LEN) {
@@ -288,8 +297,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);
@@ -299,6 +310,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;
@@ -308,9 +320,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
-- 
1.8.0.233.g54991f2

Re: [PATCH v3 5/7] pppoatm: take ATM socket lock in pppoatm_send()

[Woodhouse, David]

On Tue, 2012-11-06 at 23:17 +0100, Krzysztof Mazur wrote:
> +       if (sock_owned_by_user(sk_atm(vcc)))
> +               goto nospace;

I still think this one can lead to an infinite stall of the PPP channel,
because we return 0 from pppoatm_send() but never make a later call to
ppp_output_wakeup() to unblock it.

In the existing cases where we could return 0 (because
pppoatm_may_send() said no), we were careful to set the BLOCKED flag and
we *knew* that a packet was in flight so we'd get to wake it up again on
a subsequent pppoatm_pop().

None of that works for this new code path that returns zero.

-- 
                   Sent with MeeGo's ActiveSync support.

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

[PATCH v3 6/7] pppoatm: don't send frames on not-ready vcc

[Krzysztof Mazur]

Patches "atm: detach protocol before closing vcc"
and "pppoatm: allow assign only on a connected socket" fixed
common cases where the pppoatm_send() crashes while sending
frame to not-ready vcc. However there are still some other cases
where we can send frames to vcc, which is flagged as ATM_VF_CLOSE
(for instance after vcc_release_async()) or it's opened but not
ready yet.

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>
Cc: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/pppoatm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index c4a57bc..bf5d6c9 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -284,6 +284,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	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:
-- 
1.8.0.233.g54991f2

[PATCH v3 7/7] pppoatm: do not inline pppoatm_may_send()

[Krzysztof Mazur]

The pppoatm_may_send() is quite heavy and it's called three times
in pppoatm_send() and inlining costs more than 200 bytes of code
(more than 10% of total pppoatm driver code size).

add/remove: 1/0 grow/shrink: 0/1 up/down: 132/-367 (-235)
function                                     old     new   delta
pppoatm_may_send                               -     132    +132
pppoatm_send                                 900     533    -367

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

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index bf5d6c9..7507c20 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -214,7 +214,7 @@ error:
 	ppp_input_error(&pvcc->chan, 0);
 }
 
-static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
+static int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
 {
 	/*
 	 * It's not clear that we need to bother with using atm_may_send()
-- 
1.8.0.233.g54991f2

[PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

Now that we can return zero from pppoatm_send() for reasons *other* than
the queue being full, that means we can't depend on a subsequent call to
pppoatm_pop() waking the queue, and we might leave it stalled
indefinitely.

Fix this by immediately scheduling the wakeup tasklet. As documented
already elsewhere, the PPP channel's ->downl lock protects against the
wakeup happening too soon and effectively being missed.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
----
Untested.
With this sorted, Acked-By: David Woodhouse <David.Woodhouse@intel.com<
to the other seven. Thanks.

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 7507c20..56ad541 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -283,11 +283,11 @@ 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;
+		goto nospace_sched_wakeup;
 	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
-			|| test_bit(ATM_VF_CLOSE, &vcc->flags)
-			|| !test_bit(ATM_VF_READY, &vcc->flags))
-		goto nospace;
+	    || test_bit(ATM_VF_CLOSE, &vcc->flags)
+	    || !test_bit(ATM_VF_READY, &vcc->flags))
+		goto nospace_sched_wakeup;
 
 	switch (pvcc->encaps) {		/* LLC encapsulation needed */
 	case e_llc:
@@ -328,7 +328,17 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	    ? DROP_PACKET : 1;
 	bh_unlock_sock(sk_atm(vcc));
 	return ret;
-nospace:
+ nospace_sched_wakeup:
+	/* If we're returning zero for reasons *other* than the queue
+	 * being full, then we need to ensure that a wakeup will
+	 * happen and not just leave the channel stalled for ever.
+	 * Just schedule the wakeup tasklet directly. As observed in
+	 * pppoatm_pop(), it'll take the channel's ->downl lock which
+	 * is also held by our caller, so it can't happen "too soon"
+	 * and cause us to effectively miss a wakeup.
+	 */
+	tasklet_schedule(&pvcc->wakeup_tasklet);
+ nospace:
 	bh_unlock_sock(sk_atm(vcc));
 	/*
 	 * We don't have space to send this SKB now, but we might have

-- 
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 v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 07 Nov 2012 12:52:14 +0000

> Now that we can return zero from pppoatm_send() for reasons *other* than
> the queue being full, that means we can't depend on a subsequent call to
> pppoatm_pop() waking the queue, and we might leave it stalled
> indefinitely.
> 
> Fix this by immediately scheduling the wakeup tasklet. As documented
> already elsewhere, the PPP channel's ->downl lock protects against the
> wakeup happening too soon and effectively being missed.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> ----
> Untested.
> With this sorted, Acked-By: David Woodhouse <David.Woodhouse@intel.com<
> to the other seven. Thanks.

I don't know what to do with this patch because I don't have any
context whatsoever.

So I'm tossing it, please resubmit it when it's meant to be
applied, and with some context.

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Fri, 2012-11-09 at 16:30 -0500, David Miller wrote:
> I don't know what to do with this patch because I don't have any
> context whatsoever.

I sent two replies to Krzysztof's series starting with [PATCH v3 0/7]  
in Message-Id: <1352240222-363-1-git-send-email-krzysiek@podlesie.net>

The first was pointing out a problem; the second was a [PATCH v3 8/7]
which fixed the problem I'd pointed out. It wasn't clear to me that more
context would be needed. In particular, [PATCH v3 8/7] was a reply to
0/7, just as the other patches ##1-7 had been.

> So I'm tossing it, please resubmit it when it's meant to be
> applied, and with some context.

That's OK. I was hoping for an ack from Chas and/or Krzysztof,
especially as I hadn't tested my patch. So hopefully there'll be a v4
series of 8 patches, including this one... and all from the same person,
which makes it slightly easier to follow :)

-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Sat, 10 Nov 2012 07:36:13 +0000

> I was hoping for an ack from Chas and/or Krzysztof, especially as I
> hadn't tested my patch. So hopefully there'll be a v4 series of 8
> patches, including this one... and all from the same person, which
> makes it slightly easier to follow :)

Ok, great.

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Wed, Nov 07, 2012 at 12:52:14PM +0000, David Woodhouse wrote:
> 
> diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
> index 7507c20..56ad541 100644
> --- a/net/atm/pppoatm.c
> +++ b/net/atm/pppoatm.c
> @@ -283,11 +283,11 @@ 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;
> +		goto nospace_sched_wakeup;
>  	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
> -			|| test_bit(ATM_VF_CLOSE, &vcc->flags)
> -			|| !test_bit(ATM_VF_READY, &vcc->flags))
> -		goto nospace;
> +	    || test_bit(ATM_VF_CLOSE, &vcc->flags)
> +	    || !test_bit(ATM_VF_READY, &vcc->flags))
> +		goto nospace_sched_wakeup;
>  
>  	switch (pvcc->encaps) {		/* LLC encapsulation needed */
>  	case e_llc:
> @@ -328,7 +328,17 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
>  	    ? DROP_PACKET : 1;
>  	bh_unlock_sock(sk_atm(vcc));
>  	return ret;
> -nospace:
> + nospace_sched_wakeup:
> +	/* If we're returning zero for reasons *other* than the queue
> +	 * being full, then we need to ensure that a wakeup will
> +	 * happen and not just leave the channel stalled for ever.
> +	 * Just schedule the wakeup tasklet directly. As observed in
> +	 * pppoatm_pop(), it'll take the channel's ->downl lock which
> +	 * is also held by our caller, so it can't happen "too soon"
> +	 * and cause us to effectively miss a wakeup.
> +	 */
> +	tasklet_schedule(&pvcc->wakeup_tasklet);

With this tasklet_schedule() we implement a "spin_lock" here, but in
this case both conditions (vcc not ready and socket locked) can be true
for a long time and we can spin here for a long time. I confirmed it by
reverting patch 1 (atm: detach protocol before closing vcc) and now
I have 50% of CPU used by ksoftirqd and 50% by pppd (UP system).

Krzysiek

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Sat, 2012-11-10 at 21:23 +0100, Krzysztof Mazur wrote:
> With this tasklet_schedule() we implement a "spin_lock" here, but in
> this case both conditions (vcc not ready and socket locked) can be
> true for a long time and we can spin here for a long time. I confirmed
> it by reverting patch 1 (atm: detach protocol before closing vcc) and
> now I have 50% of CPU used by ksoftirqd and 50% by pppd (UP system).

Ah, thanks.

Can we take the lock in the tasklet, so we wait for it instead of
spinning?

-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Sat, Nov 10, 2012 at 09:02:02PM +0000, David Woodhouse wrote:
> On Sat, 2012-11-10 at 21:23 +0100, Krzysztof Mazur wrote:
> > With this tasklet_schedule() we implement a "spin_lock" here, but in
> > this case both conditions (vcc not ready and socket locked) can be
> > true for a long time and we can spin here for a long time. I confirmed
> > it by reverting patch 1 (atm: detach protocol before closing vcc) and
> > now I have 50% of CPU used by ksoftirqd and 50% by pppd (UP system).
> 
> Ah, thanks.
> 
> Can we take the lock in the tasklet, so we wait for it instead of
> spinning?
> 

I don't think so, we cannot sleep in tasklet.

I think we should use sk_add_backlog() or release_cb (introduced by:
46d3ceabd8d98ed0ad10f20c595ca784e34786c5 tcp: TCP Small Queues).
The release_cb callback is almost exactly what we need except that
it works on protocol level, not on socket. The same race with
vcc_sendmsg() exists also in other ATM protocols, so maybe we should
add wrapper in ATM layer that calls vcc->sock_release_cb().

But what about socket flags? Maybe we should just drop that frame?
When ppp is used on serial links "not-ready link" usually does that.
I'm sending an updated patch 6.

Krzysiek
-- >8 --
Subject: [PATCH] pppoatm: drop frames to not-ready vcc

Patches "atm: detach protocol before closing vcc"
and "pppoatm: allow assign only on a connected socket" fixed
common cases where the pppoatm_send() crashes while sending
frame to not-ready vcc. However there are still some other cases
where we can send frames to vcc, which is flagged as ATM_VF_CLOSE
(for instance after vcc_release_async()) or it's opened but not
ready yet.

Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready. If the vcc is not ready we just
drop frame. Queueing frames is much more complicated because we
don't have callbacks that inform us about vcc flags changes.

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

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index c4a57bc..63541a3 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -284,6 +284,13 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	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)) {
+		bh_unlock_sock(sk_atm(vcc));
+		kfree_skb(skb);
+		return DROP_PACKET;
+	}
 
 	switch (pvcc->encaps) {		/* LLC encapsulation needed */
 	case e_llc:
-- 
1.8.0.268.g9d5ca2e

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Sat, 2012-11-10 at 21:23 +0100, Krzysztof Mazur wrote:
> With this tasklet_schedule() we implement a "spin_lock" here, but in
> this case both conditions (vcc not ready and socket locked) can be
> true for a long time and we can spin here for a long time. 

Reading this more carefully this morning... I hadn't realised it was
these conditions, and not the sock_owned_by_user(), which had triggered.
Yes, perhaps we should just return zero in that case and find another
wakeup trigger... if indeed a wakeup is ever required in the VF_RELEASED
and VF_CLOSE case. And if we've fixed things so that !VF_READY can never
happen (have we?).... perhaps this one doesn't matter at all? It was the
sock_owned_by_user() case I was most interested in, and I was expecting
that lock would generally be held briefly enough that the tasklet would
be fine. Was that not so?

-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Sun, Nov 11, 2012 at 07:28:53AM +0000, David Woodhouse wrote:
> On Sat, 2012-11-10 at 21:23 +0100, Krzysztof Mazur wrote:
> > With this tasklet_schedule() we implement a "spin_lock" here, but in
> > this case both conditions (vcc not ready and socket locked) can be
> > true for a long time and we can spin here for a long time. 
> 
> Reading this more carefully this morning... I hadn't realised it was
> these conditions, and not the sock_owned_by_user(), which had triggered.

Yes, but socket can be also locked for a long time, vcc_sendmsg() sleeps
owning socket lock waiting for memory or atm_may_send().

pppd was spinning in tasklet_kill() (at least when I did sysrq+t).

> Yes, perhaps we should just return zero in that case and find another
> wakeup trigger... if indeed a wakeup is ever required in the VF_RELEASED
> and VF_CLOSE case. And if we've fixed things so that !VF_READY can never
> happen (have we?).... perhaps this one doesn't matter at all? It was the

I think we can just drop frame if vcc flags indicate not-ready link
and in this case we not need a wakeup event. I already sent you
an updated patch 3 that drops frame instead of returning zero.

> sock_owned_by_user() case I was most interested in, and I was expecting
> that lock would generally be held briefly enough that the tasklet would
> be fine. Was that not so?
> 

When vcc_sendmsg() waits for memory it can be a problem.

I think we can use release_cb callback that is called when socket lock
is released (we will need small wrapper in ATM layer that will call
new vcc->release_cb callback), but maybe we shouldn't use ATM socket
lock and use some new vcc->send_lock instead that will protect
vcc->send() and us against atm_may_send()/atomic_add(skb->truesize,
&sk->sk_wmem_alloc)/ race with vcc_sendmsg().


Any race with testing vcc flags is probably not really important
because:
	- vcc_release_async() does not take any lock so this check
	  will be always racy so we are probably allowed to send
	  new frames until vcc is really closed,
	- we are already protected against vcc_destroy_socket()
	- the only case that such test is really important
	  is openned, but not ready vcc, and we avoid any race
	  by not doing send.

Krzysiek

-- >8 --
release_cb wrapper for ATM - just to precisely show the first idea,
with this patch we can just implement vcc->release_cb() and call
tasklet_schedule() there (if needed).

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 31c16c6..a6f9d4b 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -310,6 +310,7 @@ struct atm_vcc {
 	struct atm_sap	sap;		/* SAP */
 	void (*push)(struct atm_vcc *vcc,struct sk_buff *skb);
 	void (*pop)(struct atm_vcc *vcc,struct sk_buff *skb); /* optional */
+	void (*release_cb)(struct atm_vcc *vcc);
 	int (*push_oam)(struct atm_vcc *vcc,void *cell);
 	int (*send)(struct atm_vcc *vcc,struct sk_buff *skb);
 	void		*dev_data;	/* per-device data */
diff --git a/net/atm/common.c b/net/atm/common.c
index ea952b2..4e7f305 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -126,10 +126,19 @@ static void vcc_write_space(struct sock *sk)
 	rcu_read_unlock();
 }
 
+void vcc_release_cb(struct sock *sk)
+{
+	struct atm_vcc *vcc = atm_sk(sk);
+
+	if (vcc->release_cb)
+		vcc->release_cb(vcc);
+}
+
 static struct proto vcc_proto = {
 	.name	  = "VCC",
 	.owner	  = THIS_MODULE,
 	.obj_size = sizeof(struct atm_vcc),
+	.release_cb = vcc_release_cb,
 };
 
 int vcc_create(struct net *net, struct socket *sock, int protocol, int family)

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Sun, 2012-11-11 at 12:04 +0100, Krzysztof Mazur wrote:
> Yes, but socket can be also locked for a long time, vcc_sendmsg() sleeps
> owning socket lock waiting for memory or atm_may_send().

Right. Something like this then, instead of my previous patch 8/7?

Only addresses the sock_owned_by_user() case and not ATM_VF_RELEASED,
ATM_VF_CLOSE or !ATM_VF_READY, but your amended patch 6 fixes that I
think.

diff --git a/include/linux/atmdev.h b/include/linux/atmdev.h
index 31c16c6..59532ed 100644
--- a/include/linux/atmdev.h
+++ b/include/linux/atmdev.h
@@ -308,6 +308,7 @@ struct atm_vcc {
 	struct atm_dev	*dev;		/* device back pointer */
 	struct atm_qos	qos;		/* QOS */
 	struct atm_sap	sap;		/* SAP */
+	void (*unlock_cb)(struct atm_vcc *vcc); /* release_sock callback */
 	void (*push)(struct atm_vcc *vcc,struct sk_buff *skb);
 	void (*pop)(struct atm_vcc *vcc,struct sk_buff *skb); /* optional */
 	int (*push_oam)(struct atm_vcc *vcc,void *cell);
diff --git a/net/atm/common.c b/net/atm/common.c
index ea952b2..c212016 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -126,10 +126,19 @@ static void vcc_write_space(struct sock *sk)
 	rcu_read_unlock();
 }
 
+static void vcc_unlock_cb(struct sock *sk)
+{
+	struct atm_vcc *vcc = atm_sk(sk);
+
+	if (vcc->unlock_cb)
+		vcc->unlock_cb(vcc);
+}
+
 static struct proto vcc_proto = {
 	.name	  = "VCC",
 	.owner	  = THIS_MODULE,
 	.obj_size = sizeof(struct atm_vcc),
+	.release_cb = vcc_unlock_cb,
 };
 
 int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
@@ -158,6 +167,7 @@ int vcc_create(struct net *net, struct socket *sock, int protocol, int family)
 	vcc->pop = NULL;
 	vcc->owner = NULL;
 	vcc->push_oam = NULL;
+	vcc->unlock_cb = NULL;
 	vcc->vpi = vcc->vci = 0; /* no VCI/VPI yet */
 	vcc->atm_options = vcc->aal_options = 0;
 	sk->sk_destruct = vcc_sock_destruct;
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 7507c20..751569a 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -60,6 +60,7 @@ struct pppoatm_vcc {
 	struct atm_vcc	*atmvcc;	/* VCC descriptor */
 	void (*old_push)(struct atm_vcc *, struct sk_buff *);
 	void (*old_pop)(struct atm_vcc *, struct sk_buff *);
+	void (*old_unlock_cb)(struct atm_vcc *);
 	struct module *old_owner;
 					/* keep old push/pop for detaching */
 	enum pppoatm_encaps encaps;
@@ -108,6 +109,14 @@ static void pppoatm_wakeup_sender(unsigned long arg)
 	ppp_output_wakeup((struct ppp_channel *) arg);
 }
 
+static void pppoatm_unlock_cb(struct atm_vcc *atmvcc)
+{
+	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
+
+	tasklet_schedule(&pvcc->wakeup_tasklet);
+	if (pvcc->old_unlock_cb)
+		pvcc->old_unlock_cb(atmvcc);
+}
 /*
  * This gets called every time the ATM card has finished sending our
  * skb.  The ->old_pop will take care up normal atm flow control,
@@ -152,6 +161,7 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)
 	pvcc = atmvcc_to_pvcc(atmvcc);
 	atmvcc->push = pvcc->old_push;
 	atmvcc->pop = pvcc->old_pop;
+	atmvcc->unlock_cb = pvcc->old_unlock_cb;
 	tasklet_kill(&pvcc->wakeup_tasklet);
 	ppp_unregister_channel(&pvcc->chan);
 	atmvcc->user_back = NULL;
@@ -385,6 +395,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	pvcc->old_push = atmvcc->push;
 	pvcc->old_pop = atmvcc->pop;
 	pvcc->old_owner = atmvcc->owner;
+	pvcc->old_unlock_cb = atmvcc->unlock_cb;
 	pvcc->encaps = (enum pppoatm_encaps) be.encaps;
 	pvcc->chan.private = pvcc;
 	pvcc->chan.ops = &pppoatm_ops;


-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Sun, Nov 11, 2012 at 11:39:53AM +0000, David Woodhouse wrote:
> 
> Right. Something like this then, instead of my previous patch 8/7?
> 
> Only addresses the sock_owned_by_user() case and not ATM_VF_RELEASED,
> ATM_VF_CLOSE or !ATM_VF_READY, but your amended patch 6 fixes that I
> think.
> 

Looks and works ok after:

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 7b8dafe..87e792c 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -414,6 +414,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
 	atmvcc->user_back = pvcc;
 	atmvcc->push = pppoatm_push;
 	atmvcc->pop = pppoatm_pop;
+	atmvcc->unlock_cb = pppoatm_unlock_cb;
 	__module_get(THIS_MODULE);
 	atmvcc->owner = THIS_MODULE;

With this change:
Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>

This patch should be also acked by Chas, at least changes in generic ATM
code (maybe as separate patch).

I need also an ack for new version of patch 6 (pppoatm: drop frames to
not-ready vcc).

Maybe we should schedule tasklet in pppoatm_unlock_cb() only when it's needed.


Thanks,

Krzysiek

-- >8 --
Subject: [PATCH] pppoatm: wakeup after ATM unlock only when it's needed

We need to schedule wakeup tasklet only when ATM socket locked
was previously locked. The locking is provided by the sk->sk_lock.slock
spinlock.

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

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 87e792c..841b9f8 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -66,6 +66,7 @@ struct pppoatm_vcc {
 	enum pppoatm_encaps encaps;
 	atomic_t inflight;
 	unsigned long blocked;
+	int need_wakeup;
 	int flags;			/* SC_COMP_PROT - compress protocol */
 	struct ppp_channel chan;	/* interface to generic ppp layer */
 	struct tasklet_struct wakeup_tasklet;
@@ -113,7 +114,10 @@ static void pppoatm_unlock_cb(struct atm_vcc *atmvcc)
 {
 	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-	tasklet_schedule(&pvcc->wakeup_tasklet);
+	if (pvcc->need_wakeup) {
+		pvcc->need_wakeup = 0;
+		tasklet_schedule(&pvcc->wakeup_tasklet);
+	}
 	if (pvcc->old_unlock_cb)
 		pvcc->old_unlock_cb(atmvcc);
 }
@@ -292,8 +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)))
+	if (sock_owned_by_user(sk_atm(vcc))) {
+		pvcc->need_wakeup = 1;
 		goto nospace;
+	}
 	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
 			|| test_bit(ATM_VF_CLOSE, &vcc->flags)
 			|| !test_bit(ATM_VF_READY, &vcc->flags)) {
-- 
1.8.0.268.g9d5ca2e

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Sun, 2012-11-11 at 14:50 +0100, Krzysztof Mazur wrote:
> Looks and works ok after:
> +       atmvcc->unlock_cb = pppoatm_unlock_cb;

Heh, yeah. That would probably help :)

Not sure if it's really necessary to optimise out the unneeded wakeups —
I don't think that code path gets exercised very hard for normal passing
of packets. Maybe only LCP echo and responses, on a live connection?

But yeah, the locking *is* that simple, isn't it — and not the painful
stuff I had to do for the BLOCKED flag, which is why I deferred that
question to concentrate on the basic concept of using ->release_cb().

So it's silly *not* to do the 'need_wakeup'. But could it also live in
the 'blocked' word rather than expanding the structure further? Or just
*use* the BLOCKED bit, for that matter?

-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Sun, Nov 11, 2012 at 03:26:41PM +0000, David Woodhouse wrote:
> On Sun, 2012-11-11 at 14:50 +0100, Krzysztof Mazur wrote:
> > Looks and works ok after:
> > +       atmvcc->unlock_cb = pppoatm_unlock_cb;
> 
> Heh, yeah. That would probably help :)
> 
> Not sure if it's really necessary to optimise out the unneeded wakeups ???
> I don't think that code path gets exercised very hard for normal passing
> of packets. Maybe only LCP echo and responses, on a live connection?
> 
> But yeah, the locking *is* that simple, isn't it ??? and not the painful
> stuff I had to do for the BLOCKED flag, which is why I deferred that
> question to concentrate on the basic concept of using ->release_cb().
> 
> So it's silly *not* to do the 'need_wakeup'. But could it also live in
> the 'blocked' word rather than expanding the structure further? Or just
> *use* the BLOCKED bit, for that matter?
> 

It would require using atomic ops because also pppoatm_pop() can modify this
word. I think it's better to add additional word instead of using
atomic ops.

Krzysiek

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Sun, 2012-11-11 at 17:12 +0100, Krzysztof Mazur wrote:
> It would require using atomic ops because also pppoatm_pop() can
> modify this word. I think it's better to add additional word instead
> of using atomic ops.

Or use the existing flags word, perhaps. Only one bit of which is
actually used already. We'd have to filter the new bit out in
pppoatm_devppp_ioctl().

-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Sun, Nov 11, 2012 at 05:03:21PM +0000, David Woodhouse wrote:
> On Sun, 2012-11-11 at 17:12 +0100, Krzysztof Mazur wrote:
> > It would require using atomic ops because also pppoatm_pop() can
> > modify this word. I think it's better to add additional word instead
> > of using atomic ops.
> 
> Or use the existing flags word, perhaps. Only one bit of which is
> actually used already. We'd have to filter the new bit out in
> pppoatm_devppp_ioctl().
> 

In pppoatm_devppp_ioctl() we also don't have sk->sk_lock.slock lock.
In original patch synchronization was trivial because callback
from socket lock is used.

I also though about sharing word with encaps enum - encaps needs only 2 bits,
but it's ugly.

Krzysiek

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Sun, 2012-11-11 at 19:49 +0100, Krzysztof Mazur wrote:
> 
> In pppoatm_devppp_ioctl() we also don't have sk->sk_lock.slock lock.
> In original patch synchronization was trivial because callback
> from socket lock is used.
> 
> I also though about sharing word with encaps enum - encaps needs only 2 bits,
> but it's ugly.

Yeah, fair enough. It's not the end of the world having it in a separate
word. I was just trying to avoid bloating the structure more than we
needed to.

Acked-by: David Woodhouse <David.Woodhouse@intel.com> for your new
version of patch #6 (returning DROP_PACKET for !VF_READY), and your
followup to my patch #8, adding the 'need_wakeup' flag. Which we might
as well merge into (the pppoatm part of) my patch.

Chas, are you happy with the generic ATM part of that? And the
nomenclature? I didn't want to call it 'release_cb' like the core socket
code does, because we use 'release' to mean something different in ATM.
So I called it 'unlock_cb' instead...

-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Chas Williams (CONTRACTOR)]

In message <1352667081.9449.135.camel@shinybook.infradead.org>,David Woodhouse writes:
>Acked-by: David Woodhouse <David.Woodhouse@intel.com> for your new
>version of patch #6 (returning DROP_PACKET for !VF_READY), and your
>followup to my patch #8, adding the 'need_wakeup' flag. Which we might
>as well merge into (the pppoatm part of) my patch.
>
>Chas, are you happy with the generic ATM part of that? And the
>nomenclature? I didn't want to call it 'release_cb' like the core socket
>code does, because we use 'release' to mean something different in ATM.
>So I called it 'unlock_cb' instead...

i really would prefer not to use a strange name since it might confuse
larger group of people who are more familiar with the traditional meaning
of this function.  vcc_release() isnt exported so we could rename it if
things get too confusing.

i have to look at this a bit more but we might be able to use release_cb
to get rid of the null push to detach the underlying protocol.  that would
be somewhat nice.

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Sun, 2012-11-11 at 17:57 -0500, Chas Williams (CONTRACTOR) wrote:
> In message <1352667081.9449.135.camel@shinybook.infradead.org>,David Woodhouse writes:
> >Acked-by: David Woodhouse <David.Woodhouse@intel.com> for your new
> >version of patch #6 (returning DROP_PACKET for !VF_READY), and your
> >followup to my patch #8, adding the 'need_wakeup' flag. Which we might
> >as well merge into (the pppoatm part of) my patch.
> >
> >Chas, are you happy with the generic ATM part of that? And the
> >nomenclature? I didn't want to call it 'release_cb' like the core socket
> >code does, because we use 'release' to mean something different in ATM.
> >So I called it 'unlock_cb' instead...
> 
> i really would prefer not to use a strange name since it might confuse
> larger group of people who are more familiar with the traditional meaning
> of this function.  vcc_release() isnt exported so we could rename it if
> things get too confusing.
> 
> i have to look at this a bit more but we might be able to use release_cb
> to get rid of the null push to detach the underlying protocol.  that would
> be somewhat nice.

In the meantime, should I resend this patch with the name 'release_cb'
instead of 'unlock_cb'? I'll just put a comment in to make sure it isn't
confused with vcc_release(), and if we need to change vcc_release()
later we can.

-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[chas williams - CONTRACTOR]

On Tue, 27 Nov 2012 13:27:47 +0000
David Woodhouse <dwmw2@infradead.org> wrote:

> > i really would prefer not to use a strange name since it might confuse
> > larger group of people who are more familiar with the traditional meaning
> > of this function.  vcc_release() isnt exported so we could rename it if
> > things get too confusing.
> > 
> > i have to look at this a bit more but we might be able to use release_cb
> > to get rid of the null push to detach the underlying protocol.  that would
> > be somewhat nice.
> 
> In the meantime, should I resend this patch with the name 'release_cb'
> instead of 'unlock_cb'? I'll just put a comment in to make sure it isn't
> confused with vcc_release(), and if we need to change vcc_release()
> later we can.
> 

yes, but dont call it 8/7 since that doesnt make sense.

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Tue, 2012-11-27 at 10:23 -0500, chas williams - CONTRACTOR wrote:
> yes, but dont call it 8/7 since that doesnt make sense.

It made enough sense when it was a single patch appended to a thread of
7 other patches from Krzysztof. But now it's all got a little more
complex, so I've tried to collect together the latest version of
everything we've discussed:

 http://git.infradead.org/users/dwmw2/atm.git
  git://git.infradead.org/users/dwmw2/atm.git

David Woodhouse (5):
      solos-pci: Wait for pending TX to complete when releasing vcc
      br2684: don't send frames on not-ready vcc
      atm: Add release_cb() callback to vcc
      pppoatm: fix missing wakeup in pppoatm_send()
      br2684: fix module_put() race

Krzysztof Mazur (6):
      atm: add owner of push() callback to atmvcc
      pppoatm: allow assign only on a connected socket
      pppoatm: fix module_put() race
      pppoatm: take ATM socket lock in pppoatm_send()
      pppoatm: drop frames to not-ready vcc
      pppoatm: do not inline pppoatm_may_send()


-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Wed, Nov 28, 2012 at 12:48:17AM +0000, David Woodhouse wrote:
> On Tue, 2012-11-27 at 10:23 -0500, chas williams - CONTRACTOR wrote:
> > yes, but dont call it 8/7 since that doesnt make sense.
> 
> It made enough sense when it was a single patch appended to a thread of
> 7 other patches from Krzysztof. But now it's all got a little more
> complex, so I've tried to collect together the latest version of
> everything we've discussed:

There was also discussion about patch 9/7 "pppoatm: wakeup after ATM
unlock only when it's needed".

> 
>  http://git.infradead.org/users/dwmw2/atm.git
>   git://git.infradead.org/users/dwmw2/atm.git
> 
> David Woodhouse (5):
>       atm: Add release_cb() callback to vcc
>       pppoatm: fix missing wakeup in pppoatm_send()
>       br2684: fix module_put() race

for the three patches above:

Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>

Krzysiek

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Wed, 2012-11-28 at 09:12 +0100, Krzysztof Mazur wrote:
> On Wed, Nov 28, 2012 at 12:48:17AM +0000, David Woodhouse wrote:
> > On Tue, 2012-11-27 at 10:23 -0500, chas williams - CONTRACTOR wrote:
> > > yes, but dont call it 8/7 since that doesnt make sense.
> > 
> > It made enough sense when it was a single patch appended to a thread of
> > 7 other patches from Krzysztof. But now it's all got a little more
> > complex, so I've tried to collect together the latest version of
> > everything we've discussed:
> 
> There was also discussion about patch 9/7 "pppoatm: wakeup after ATM
> unlock only when it's needed".

True. Is that really necessary? How often is the lock actually taken? Is
it once per packet that PPP sends (which is mostly just LCP
echo/response during an active connection)? And does that really warrant
the optimisation?

This is a tasklet that we used to run after absolutely *every* packet,
remember. Optimising *that* made sense, but I'm less sure it's worth the
added complexity for this case. As I have a vague recollection that we
decided we couldn't use the existing BLOCKED bit for it... or can we? 

Can this work? Feel free to replace that test_bit() and the
corresponding comment, with a test_and_clear_bit() and a new comment
explaining *why* it's safe... while I go make another cup of tea.

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 446a7f0..da58863 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -113,7 +113,13 @@ static void pppoatm_release_cb(struct atm_vcc *atmvcc)
 {
 	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-	tasklet_schedule(&pvcc->wakeup_tasklet);
+	/*
+	 * We can't clear it here because I haven't had enough caffeine
+	 * this morning to deal with the concurrency issues. Just leave
+	 * it set, and let pppoatm_pop() clear it later.
+	 */
+	if (test_bit(BLOCKED, &pvcc->blocked))
+		tasklet_schedule(&pvcc->wakeup_tasklet);
 	if (pvcc->old_release_cb)
 		pvcc->old_release_cb(atmvcc);
 }
@@ -342,6 +348,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 	bh_unlock_sock(sk_atm(vcc));
 	return ret;
 nospace:
+	/*
+	 * Needs to happen (and be flushed, hence test_and_) before we unlock
+	 * the socket. It needs to be seen by the time our ->release_cb gets
+	 * called.
+	 */
+	test_and_set_bit(BLOCKED, &pvcc->blocked);
 	bh_unlock_sock(sk_atm(vcc));
 	/*
 	 * We don't have space to send this SKB now, but we might have


> > David Woodhouse (5):
> >       atm: Add release_cb() callback to vcc
> >       pppoatm: fix missing wakeup in pppoatm_send()
> >       br2684: fix module_put() race
> 
> for the three patches above:
> 
> Acked-by: Krzysztof Mazur <krzysiek@podlesie.net>

Ta.
-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Krzysztof Mazur]

On Wed, Nov 28, 2012 at 09:24:09AM +0000, David Woodhouse wrote:
> On Wed, 2012-11-28 at 09:12 +0100, Krzysztof Mazur wrote:
> > On Wed, Nov 28, 2012 at 12:48:17AM +0000, David Woodhouse wrote:
> > > On Tue, 2012-11-27 at 10:23 -0500, chas williams - CONTRACTOR wrote:
> > > > yes, but dont call it 8/7 since that doesnt make sense.
> > > 
> > > It made enough sense when it was a single patch appended to a thread of
> > > 7 other patches from Krzysztof. But now it's all got a little more
> > > complex, so I've tried to collect together the latest version of
> > > everything we've discussed:
> > 
> > There was also discussion about patch 9/7 "pppoatm: wakeup after ATM
> > unlock only when it's needed".
> 
> True. Is that really necessary? How often is the lock actually taken? Is
> it once per packet that PPP sends (which is mostly just LCP
> echo/response during an active connection)? And does that really warrant
> the optimisation?
> 
> This is a tasklet that we used to run after absolutely *every* packet,
> remember. Optimising *that* made sense, but I'm less sure it's worth the
> added complexity for this case. As I have a vague recollection that we
> decided we couldn't use the existing BLOCKED bit for it... or can we? 
> 
> Can this work? Feel free to replace that test_bit() and the
> corresponding comment, with a test_and_clear_bit() and a new comment
> explaining *why* it's safe... while I go make another cup of tea.
> 

ok, I think that we should just drop that patch, with test_bit()
I think it's no longer an optimization.

Krzysiek

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[David Woodhouse]

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

On Wed, 2012-11-28 at 10:58 +0100, Krzysztof Mazur wrote:
> ok, I think that we should just drop that patch, with test_bit()
> I think it's no longer an optimization.

After another cup of tea, it now uses test_and_clear_bit()... and
doesn't break the carefully crafted handling of the BLOCKED bit in the
normal flow control case, by setting it at the nospace: label!

From b2cf6a466697ecf19061cb11b8f4ec5bb381550a Mon Sep 17 00:00:00 2001
From: David Woodhouse <David.Woodhouse@intel.com>
Date: Wed, 28 Nov 2012 10:15:05 +0000
Subject: [PATCH] pppoatm: optimise PPP channel wakeups after sock_owned_by_user()

We don't need to schedule the wakeup tasklet on *every* unlock; only if we
actually blocked the channel in the first place.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 net/atm/pppoatm.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 446a7f0..172e44e 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -113,7 +113,17 @@ static void pppoatm_release_cb(struct atm_vcc *atmvcc)
 {
 	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
 
-	tasklet_schedule(&pvcc->wakeup_tasklet);
+	/*
+	 * As in pppoatm_pop(), it's safe to clear the BLOCKED bit here because
+	 * the wakeup *can't* race with pppoatm_send(). They both hold the PPP
+	 * channel's ->downl lock. And the potential race with *setting* it,
+	 * which leads to the double-check dance in pppoatm_may_send(), doesn't
+	 * exist here. In the sock_owned_by_user() case in pppoatm_send(), we
+	 * set the BLOCKED bit while the socket is still locked. We know that
+	 * ->release_cb() can't be called until that's done.
+	 */
+	if (test_and_clear_bit(BLOCKED, &pvcc->blocked))
+		tasklet_schedule(&pvcc->wakeup_tasklet);
 	if (pvcc->old_release_cb)
 		pvcc->old_release_cb(atmvcc);
 }
@@ -292,8 +302,15 @@ 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)))
+	if (sock_owned_by_user(sk_atm(vcc))) {
+		/*
+		 * Needs to happen (and be flushed, hence test_and_) before we unlock
+		 * the socket. It needs to be seen by the time our ->release_cb gets
+		 * called.
+		 */
+		test_and_set_bit(BLOCKED, &pvcc->blocked);
 		goto nospace;
+	}
 	if (test_bit(ATM_VF_RELEASED, &vcc->flags)
 			|| test_bit(ATM_VF_CLOSE, &vcc->flags)
 			|| !test_bit(ATM_VF_READY, &vcc->flags)) {
-- 
1.8.0



-- 
dwmw2


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

Re: [PATCH v3 8/7] pppoatm: fix missing wakeup in pppoatm_send()

[Chas Williams (CONTRACTOR)]

In message <20121111110437.GA25894@shrek.podlesie.net>,Krzysztof Mazur writes:
>Any race with testing vcc flags is probably not really important
>because:
>	- vcc_release_async() does not take any lock so this check
>	  will be always racy so we are probably allowed to send
>	  new frames until vcc is really closed,

this locking/synchronization is handled in the atm device drivers.
the send and close routines are responsbile for synchronization (yes, i
believe that leads to duplicated code but that is the way it currently is)