From: Krzysztof Mazur <hidden> Date: 2012-10-22 17:23:20
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 <redacted>
Cc: David Woodhouse <dwmw2@infradead.org>
---
net/atm/pppoatm.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
From: Krzysztof Mazur <hidden> Date: 2012-10-22 17:24:20
The pppoatm gets a reference to atmvcc, but does not increment vcc
usage count. The vcc uses vcc->sk socket for reference counting,
so sock_hold() and sock_put() should be used by pppoatm.
Signed-off-by: Krzysztof Mazur <redacted>
Cc: David Woodhouse <dwmw2@infradead.org>
---
net/atm/pppoatm.c | 3 +++
1 file changed, 3 insertions(+)
@@ -154,6 +154,7 @@ static void pppoatm_unassign_vcc(struct atm_vcc *atmvcc)tasklet_kill(&pvcc->wakeup_tasklet);ppp_unregister_channel(&pvcc->chan);atmvcc->user_back=NULL;+sock_put(sk_atm(pvcc->atmvcc));kfree(pvcc);/* Gee, I hope we have the big kernel lock here... */module_put(THIS_MODULE);
@@ -373,6 +374,7 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)if(pvcc==NULL)return-ENOMEM;pvcc->atmvcc=atmvcc;+sock_hold(sk_atm(atmvcc));/* Maximum is zero, so that we can use atomic_inc_not_zero() */atomic_set(&pvcc->inflight,NONE_INFLIGHT);
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-10-23 08:13:07
On Tue, 2012-10-23 at 02:52 -0400, David Miller wrote:
David, if you could review this series I'd really appreciate it.
Will do. I glanced at it last night but need to be in the right frame of
mind for thinking about ATM locking.
I know I have a bottle of vodka *somewhere* around here... I saw it
after we moved...
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-10-30 09:35:19
On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready.
I note that vcc_sendmsg() also checks for sock->state == SS_CONNECTED.
Is that check not needed here? Otherwise, looks sane enough.
Acked-By: David Woodhouse <redacted>
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-10-30 09:37:57
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 <redacted>
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-10-30 09:39:28
On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
The pppoatm gets a reference to atmvcc, but does not increment vcc
usage count. The vcc uses vcc->sk socket for reference counting,
so sock_hold() and sock_put() should be used by pppoatm.
Signed-off-by: Krzysztof Mazur <redacted>
Cc: David Woodhouse <dwmw2@infradead.org>
Acked-By: David Woodhouse <redacted>
But did you spot what's in the end of the context of the first hunk...?
*atmvcc)
tasklet_kill(&pvcc->wakeup_tasklet);
ppp_unregister_channel(&pvcc->chan);
atmvcc->user_back = NULL;
+ sock_put(sk_atm(pvcc->atmvcc));
kfree(pvcc);
/* Gee, I hope we have the big kernel lock here... */
module_put(THIS_MODULE);
Fairly sure that hope is unfounded these days... :)
--
dwmw2
From: Chas Williams (CONTRACTOR) <hidden> Date: 2012-10-30 14:41:58
In message [off-list ref],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.
From: Krzysztof Mazur <hidden> Date: 2012-10-30 18:27:46
On Tue, Oct 30, 2012 at 10:26:46AM -0400, Chas Williams (CONTRACTOR) wrote:
In message [off-list ref],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
From: Krzysztof Mazur <hidden> Date: 2012-10-30 19:07:31
On Tue, Oct 30, 2012 at 09:37:48AM +0000, 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 <redacted>
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 --
From: Krzysztof Mazur <hidden> Date: 2012-10-30 19:26:45
On Tue, Oct 30, 2012 at 09:39:22AM +0000, David Woodhouse wrote:
On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
quoted
The pppoatm gets a reference to atmvcc, but does not increment vcc
usage count. The vcc uses vcc->sk socket for reference counting,
so sock_hold() and sock_put() should be used by pppoatm.
Signed-off-by: Krzysztof Mazur <redacted>
Cc: David Woodhouse <dwmw2@infradead.org>
Acked-By: David Woodhouse <redacted>
This patch is not needed, because vcc_destroy_socket()
calls pppoatm_push(vcc, NULL) to indicate that vcc is now closed,
before vcc_release() calls sock_put() and it's properly handled
by pppoatm.
I will drop this patch.
But did you spot what's in the end of the context of the first hunk...?
*atmvcc)
tasklet_kill(&pvcc->wakeup_tasklet);
ppp_unregister_channel(&pvcc->chan);
atmvcc->user_back = NULL;
+ sock_put(sk_atm(pvcc->atmvcc));
kfree(pvcc);
/* Gee, I hope we have the big kernel lock here... */
module_put(THIS_MODULE);
Fairly sure that hope is unfounded these days... :)
From: Krzysztof Mazur <hidden> Date: 2012-10-30 19:52:28
On Tue, Oct 30, 2012 at 08:07:25PM +0100, Krzysztof Mazur wrote:
On Tue, Oct 30, 2012 at 09:37:48AM +0000, David Woodhouse wrote:
quoted
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().
quoted
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 <redacted>
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 <redacted>
---
net/atm/pppoatm.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
From: Krzysztof Mazur <hidden> Date: 2012-10-30 20:19:42
On Tue, Oct 30, 2012 at 09:35:00AM +0000, David Woodhouse wrote:
On Mon, 2012-10-22 at 19:14 +0200, Krzysztof Mazur wrote:
quoted
Now pppoatm_send(), like vcc_sendmsg(), checks for vcc flags that
indicate that vcc is not ready.
I note that vcc_sendmsg() also checks for sock->state == SS_CONNECTED.
Is that check not needed here? Otherwise, looks sane enough.
Acked-By: David Woodhouse <redacted>
I don't think so. We never leave SS_CONNECTED state. This check is
done in vcc_sendmsg() because it's called from userspace.
However maybe we should check socket state before assigning vcc to
pppoatm (untested):
From: Krzysztof Mazur <hidden> Date: 2012-10-31 09:41:57
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:
quoted
In message [off-list ref],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.
quoted
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 --
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
From: Krzysztof Mazur <hidden> Date: 2012-10-31 10:22:16
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
From: Krzysztof Mazur <hidden> Date: 2012-10-31 11:30:29
On Wed, Oct 31, 2012 at 10:16:18AM +0000, 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 <redacted>
---
net/atm/pppoatm.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-10-31 11:52:37
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
From: chas williams - CONTRACTOR <hidden> Date: 2012-10-31 20:05:19
On Wed, 31 Oct 2012 10:41:47 +0100
Krzysztof Mazur [off-list ref] wrote:
On Tue, Oct 30, 2012 at 07:20:01PM +0100, Krzysztof Mazur wrote:
quoted
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.
From: Krzysztof Mazur <hidden> Date: 2012-10-31 22:04:40
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: 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 <redacted>
---
net/atm/common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-01 14:26:40
On Wed, 31 Oct 2012 23:04:35 +0100
Krzysztof Mazur [off-list ref] 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.
From: Krzysztof Mazur <hidden> Date: 2012-11-02 09:40:24
On Thu, Nov 01, 2012 at 10:26:28AM -0400, chas williams - CONTRACTOR wrote:
On Wed, 31 Oct 2012 23:04:35 +0100
Krzysztof Mazur [off-list ref] wrote:
quoted
There are also some minor potential issues in pppoatm driver:
- locking issues, but now only between pppoatm_send() and
vcc_sendmsg() and maybe some other functions,
these have been around for a while. i agree that something should be
done about it. just not sure what should be synchronizing this mess.
I think the ATM socket lock should be used. I'm sending the latest
patch that adds this locking after David Woodhouse's comments. The vcc->flags
check is now probably unnecessary.
quoted
- missing check for SS_CONNECTED in pppoatm_ioctl,
in practice you will never run into this because a pvc is immediately
put into SS_CONNECTED mode (right before the userspace open()
returns). however, should it check? yes. i dont see anything
preventing you from running ppp on svc's.
I can confirm that the problem really exists, without connect() in pppoatm
plugin in pppd, I have seen an Oops and panic. I will send appropriate
patch.
Thanks.
Krzysiek
-- >8 --
From: Krzysztof Mazur <hidden> Date: 2012-11-02 10:54:30
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:
quoted
On Wed, 31 Oct 2012 23:04:35 +0100
Krzysztof Mazur [off-list ref] wrote:
quoted
- missing check for SS_CONNECTED in pppoatm_ioctl,
in practice you will never run into this because a pvc is immediately
put into SS_CONNECTED mode (right before the userspace open()
returns). however, should it check? yes. i dont see anything
preventing you from running ppp on svc's.
I can confirm that the problem really exists, without connect() in pppoatm
plugin in pppd, I have seen an Oops and panic. I will send appropriate
patch.
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 <redacted>
---
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(+)
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-27 17:16:40
Krzysztof, you've fixed a bunch of races... but I think there's one
still left.
An ATM driver will often have code like this, which gets called from
arbitrary contexts:
if (vcc->pop)
vcc->pop(vcc, skb);
Now, what happens if pppoatm_send(vcc, NULL) happens after the address
of vcc->pop (currently pppoatm_pop) has been loaded, but before the
function is actually called?
You tear down all the setup and set vcc->user_back to NULL. And then
pppoatm_pop() gets called. And promptly crashes because pvcc is NULL.
A lot of these problems exist for br2684 too, and in prodding at it a
little I can consistently crash the system by sending a flood of
outbound packets while I kill the br2684ctl program. I end up in
br2684_pop() with vcc->user_back == NULL. In looking to see how you'd
fixed that in pppoatm, I realised that you haven't... :)
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-27 17:39:11
On Tue, Nov 27, 2012 at 05:16:32PM +0000, David Woodhouse wrote:
Krzysztof, you've fixed a bunch of races... but I think there's one
still left.
An ATM driver will often have code like this, which gets called from
arbitrary contexts:
if (vcc->pop)
vcc->pop(vcc, skb);
Now, what happens if pppoatm_send(vcc, NULL) happens after the address
of vcc->pop (currently pppoatm_pop) has been loaded, but before the
function is actually called?
You tear down all the setup and set vcc->user_back to NULL. And then
pppoatm_pop() gets called. And promptly crashes because pvcc is NULL.
A lot of these problems exist for br2684 too, and in prodding at it a
little I can consistently crash the system by sending a flood of
outbound packets while I kill the br2684ctl program. I end up in
br2684_pop() with vcc->user_back == NULL. In looking to see how you'd
fixed that in pppoatm, I realised that you haven't... :)
Yes, I missed that one - it's even worse, I introduced that bug
in "[PATCH 1/7] atm: detach protocol before closing vcc". Before that
patch that scenario shouldn't happen because vcc was closed before
calling pppoatm_send(vcc, NULL) - the driver should provide appropriate
synchronization.
I think that we should just drop that patch. With later changes it's not
necessary - the pppoatm_send() can be safely called while closing vcc.
Krzysiek
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-27 18:02:34
On Tue, 2012-11-27 at 18:39 +0100, Krzysztof Mazur wrote:
Yes, I missed that one - it's even worse, I introduced that bug
in "[PATCH 1/7] atm: detach protocol before closing vcc". Before that
patch that scenario shouldn't happen because vcc was closed before
calling pppoatm_send(vcc, NULL) - the driver should provide appropriate
synchronization.
I think that we should just drop that patch. With later changes it's not
necessary - the pppoatm_send() can be safely called while closing vcc.
I'm not running with that patch. This bug exists for br2684 even before
it, and I think also for pppoatm.
In solos-pci at least, the ops->close() function doesn't flush all
pending skbs for this vcc before returning. So can be a tasklet
somewhere which has loaded the address of the vcc->pop function from one
of them, and is going to call it in some unspecified amount of time.
Should we make the device's ->close function wait for all TX and RX skbs
for this vcc to complete?
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-27 18:28:48
On Tue, Nov 27, 2012 at 06:02:29PM +0000, David Woodhouse wrote:
On Tue, 2012-11-27 at 18:39 +0100, Krzysztof Mazur wrote:
quoted
Yes, I missed that one - it's even worse, I introduced that bug
in "[PATCH 1/7] atm: detach protocol before closing vcc". Before that
patch that scenario shouldn't happen because vcc was closed before
calling pppoatm_send(vcc, NULL) - the driver should provide appropriate
synchronization.
I think that we should just drop that patch. With later changes it's not
necessary - the pppoatm_send() can be safely called while closing vcc.
I'm not running with that patch. This bug exists for br2684 even before
it, and I think also for pppoatm.
In solos-pci at least, the ops->close() function doesn't flush all
pending skbs for this vcc before returning. So can be a tasklet
somewhere which has loaded the address of the vcc->pop function from one
of them, and is going to call it in some unspecified amount of time.
Should we make the device's ->close function wait for all TX and RX skbs
for this vcc to complete?
Yes, the ->close() can sleep and after vcc is closed the ->pop() shouldn't be
called.
While reviewing your br2684 patch I also found that some ATM drivers does
not call ->pop() when ->send() fails, they should do:
if (vcc->pop)
vcc->pop(vcc, skb);
else
dev_kfree_skb(skb);
but some drivers just call dev_kfree_skb(skb).
I think that we should add atm_pop() function that does that and fix all
drivers.
Krzysiek
From: Krzysztof Mazur <hidden> Date: 2012-11-27 18:39:41
On Tue, Nov 27, 2012 at 06:02:29PM +0000, David Woodhouse wrote:
I'm not running with that patch. This bug exists for br2684 even before
it, and I think also for pppoatm.
Did you use your "atm: br2684: Fix excessive queue bloat" patch?
With that patch for pppoatm the dev->close()/pppoatm_send() race
was much easier to trigger. Maybe with an equivalent patch for br2684
the races are also easier triggerable.
Krzysiek
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-27 18:55:00
On Tue, 27 Nov 2012 18:02:29 +0000
David Woodhouse [off-list ref] wrote:
In solos-pci at least, the ops->close() function doesn't flush all
pending skbs for this vcc before returning. So can be a tasklet
somewhere which has loaded the address of the vcc->pop function from one
of them, and is going to call it in some unspecified amount of time.
Should we make the device's ->close function wait for all TX and RX skbs
for this vcc to complete?
the driver's close routine should wait for any of the pending tx and rx
to complete. take a look at the he.c in driver/atm
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-27 22:36:35
We should no longer be calling the old pop routine for the vcc, after
vcc_release() has completed. Make sure we wait for any pending TX skbs
to complete, by waiting for our own PKT_PCLOSE control skb to be sent.
Signed-off-by: David Woodhouse <redacted>
---
On Tue, 2012-11-27 at 13:54 -0500, chas williams - CONTRACTOR wrote:
the driver's close routine should wait for any of the pending tx and
rx to complete.
Nathan, does this help? I can test here to a certain extent, but when I
use it in PPPoE mode and then crash the router, the DSLAM tends to
refuse to talk to me for an arbitrary period of time after that. Which
is something of a PITA.
@@ -881,11 +883,15 @@ static void pclose(struct atm_vcc *vcc)header->vci=cpu_to_le16(vcc->vci);header->type=cpu_to_le16(PKT_PCLOSE);+SKB_CB(skb)->c=&c;+fpga_queue(card,SOLOS_CHAN(vcc->dev),skb,NULL);clear_bit(ATM_VF_ADDR,&vcc->flags);clear_bit(ATM_VF_READY,&vcc->flags);+wait_for_completion(&c);+/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in thetasklethasfinishedprocessinganyincomingpackets(and,moretothepoint,usingthevccpointer).*/
@@ -1011,9 +1017,12 @@ static uint32_t fpga_tx(struct solos_card *card)if(vcc){atomic_inc(&vcc->stats->tx);solos_pop(vcc,oldskb);-}else+}else{+structpkt_hdr*header=(void*)oldskb->data;+if(le16_to_cpu(header->type)==PKT_PCLOSE)+complete(SKB_CB(oldskb)->c);dev_kfree_skb_irq(oldskb);-+}}}/* For non-DMA TX, write the 'TX start' bit for all four ports simultaneously */
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-27 23:28:44
Avoid submitting patches to a vcc which is being closed. Things go badly
wrong when the ->pop method gets later called after everything's been
torn down.
Signed-off-by: David Woodhouse <redacted>
---
On Tue, 2012-11-27 at 22:36 +0000, David Woodhouse wrote:
Nathan, does this help?
I think that's necessary, but not sufficient. You'll want something like
this too... I can now kill br2684ctl while there's a flood of outgoing
packets, and get a handful of the printks that I had in here until a few
seconds ago when I edited it out of the patch in my mail client... and
no more panic.
I do also now have Krzysztof's patch 1/7 (detach protocol before closing
vcc) but I don't think it actually matters any more.
From: Krzysztof Mazur <hidden> Date: 2012-11-27 23:51:37
On Tue, Nov 27, 2012 at 11:28:36PM +0000, David Woodhouse wrote:
Avoid submitting patches to a vcc which is being closed. Things go badly
wrong when the ->pop method gets later called after everything's been
torn down.
Signed-off-by: David Woodhouse <redacted>
---
On Tue, 2012-11-27 at 22:36 +0000, David Woodhouse wrote:
quoted
Nathan, does this help?
I think that's necessary, but not sufficient. You'll want something like
this too... I can now kill br2684ctl while there's a flood of outgoing
packets, and get a handful of the printks that I had in here until a few
seconds ago when I edited it out of the patch in my mail client... and
no more panic.
I do also now have Krzysztof's patch 1/7 (detach protocol before closing
vcc) but I don't think it actually matters any more.
If you do this actually it's better to don't use patch 1/7 because
it introduces race condition that you found earlier.
@@ -249,6 +249,12 @@ static int br2684_xmit_vcc(struct sk_bufskb_debug(skb);ATM_SKB(skb)->vcc=atmvcc=brvcc->atmvcc;+if(test_bit(ATM_VF_RELEASED,&atmvcc->flags)+||test_bit(ATM_VF_CLOSE,&atmvcc->flags)+||!test_bit(ATM_VF_READY,&atmvcc->flags)){+dev_kfree_skb(skb);+return0;+}pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",skb,atmvcc,atmvcc->dev);atomic_add(skb->truesize,&sk_atm(atmvcc)->sk_wmem_alloc);ATM_SKB(skb)->atm_options=atmvcc->atm_options;
With this patch you have still theoretical race that was fixed in patches
5 and 8 in pppoatm series, but I never seen that in practice.
Acked-by: Krzysztof Mazur <redacted>
Krzysiek
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-28 00:54:53
On Wed, 2012-11-28 at 00:51 +0100, Krzysztof Mazur wrote:
If you do this actually it's better to don't use patch 1/7 because
it introduces race condition that you found earlier.
Right. I've omitted that from the git tree I just pushed out.
With this patch you have still theoretical race that was fixed in patches
5 and 8 in pppoatm series, but I never seen that in practice.
And I think it's even less likely for br2684. At least with pppoatm you
might have had pppd sending frames. But for br2684 they *only* come from
its start_xmit function... which is serialised anyway.
I do get strange oopses when I try to add BQL to br2684, but that's not
something to be looking at at 1am...
I *do* need the equivalent of your patch 4, which is the module_put
race.
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-28 08:08:08
On Wed, Nov 28, 2012 at 12:54:46AM +0000, David Woodhouse wrote:
On Wed, 2012-11-28 at 00:51 +0100, Krzysztof Mazur wrote:
quoted
If you do this actually it's better to don't use patch 1/7 because
it introduces race condition that you found earlier.
Right. I've omitted that from the git tree I just pushed out.
quoted
With this patch you have still theoretical race that was fixed in patches
5 and 8 in pppoatm series, but I never seen that in practice.
And I think it's even less likely for br2684. At least with pppoatm you
might have had pppd sending frames. But for br2684 they *only* come from
its start_xmit function... which is serialised anyway.
I do get strange oopses when I try to add BQL to br2684, but that's not
something to be looking at at 1am...
I *do* need the equivalent of your patch 4, which is the module_put
race.
I think you might need also an equivalent of
"[PATCH v3 3/7] pppoatm: allow assign only on a connected socket".
I'm not sure yet. In will test if I can trigger that Oops on pppoatm
without that patch. Testing vcc flags might be sufficient - that's
what I did in the first patch, but you asked what about SOCK_CONNECTED,
and I think it was really needed.
Krzysiek
-- >8 --
Subject: [PATCH] br2684: allow assign only on a connected socket
The br2684 does not check if used vcc is in connected state,
causing potential Oops in pppoatm_send() when vcc->send() is called
on not fully connected socket.
Now br2684 can be assigned only on connected sockets; otherwise
-EINVAL error is returned.
Signed-off-by: Krzysztof Mazur <redacted>
---
net/atm/br2684.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
From: David Laight <hidden> Date: 2012-11-28 09:22:57
On Tue, 27 Nov 2012 18:02:29 +0000
David Woodhouse [off-list ref] wrote:
quoted
In solos-pci at least, the ops->close() function doesn't flush all
pending skbs for this vcc before returning. So can be a tasklet
somewhere which has loaded the address of the vcc->pop function from one
of them, and is going to call it in some unspecified amount of time.
Should we make the device's ->close function wait for all TX and RX skbs
for this vcc to complete?
the driver's close routine should wait for any of the pending tx and rx
to complete. take a look at the he.c in driver/atm
I'm not sure that sleeping for long periods in close() is always a
good idea. If the process is event driven it will be unable to
handle events on other fd until the close completes.
This may be known not to be true in this case, but is more generally
a problem.
In this case the close should probably (IMHO at least) only sleep
while pending tx and rx are aborted/discarded.
Even when it might make sense to sleep in close until tx drains
there needs to be a finite timeout before it become abortive.
David
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-28 09:59:06
On Wed, 2012-11-28 at 09:08 +0100, Krzysztof Mazur wrote:
I think you might need also an equivalent of
"[PATCH v3 3/7] pppoatm: allow assign only on a connected socket".
I'm not sure yet. In will test if I can trigger that Oops on pppoatm
without that patch. Testing vcc flags might be sufficient - that's
what I did in the first patch, but you asked what about SOCK_CONNECTED,
and I think it was really needed.
Even if the READY check avoids the oops, I think the patch makes sense.
Especially as it makes things consistent with pppoatm. I've applied it
to the tree. Thanks.
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-28 10:04:15
On Wed, Nov 28, 2012 at 09:21:37AM -0000, David Laight wrote:
quoted
On Tue, 27 Nov 2012 18:02:29 +0000
David Woodhouse [off-list ref] wrote:
quoted
In solos-pci at least, the ops->close() function doesn't flush all
pending skbs for this vcc before returning. So can be a tasklet
somewhere which has loaded the address of the vcc->pop function from one
of them, and is going to call it in some unspecified amount of time.
Should we make the device's ->close function wait for all TX and RX skbs
for this vcc to complete?
the driver's close routine should wait for any of the pending tx and rx
to complete. take a look at the he.c in driver/atm
I'm not sure that sleeping for long periods in close() is always a
good idea. If the process is event driven it will be unable to
handle events on other fd until the close completes.
This may be known not to be true in this case, but is more generally
a problem.
In this case the close should probably (IMHO at least) only sleep
while pending tx and rx are aborted/discarded.
Even when it might make sense to sleep in close until tx drains
there needs to be a finite timeout before it become abortive.
The ->close() routine can just abort any pending rx/tx and just wait
for completion of currently running rx/tx code. That shouldn't take
long.
Krzysiek
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-28 10:24:36
On Wed, 2012-11-28 at 11:04 +0100, Krzysztof Mazur wrote:
The ->close() routine can just abort any pending rx/tx and just wait
for completion of currently running rx/tx code. That shouldn't take
long.
If it's been submitted to the hardware for DMA, it can't do that very
easily.
And if I can't be bothered to write code to go through the entire damn
queue and inspect every packet to see if it's a data packet and check
the VCI/VPI and try to steal it, it can't be done for the software queue
either :)
The queue ought to be short; if it isn't, then we already screwed up.
The close therefore should be quick, and it *doesn't* have to be
instant.
If someone wants to return immediately, there's always
vcc_release_async()...
--
dwmw2
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-28 15:19:08
On Wed, 28 Nov 2012 10:24:28 +0000
David Woodhouse [off-list ref] wrote:
On Wed, 2012-11-28 at 11:04 +0100, Krzysztof Mazur wrote:
quoted
The ->close() routine can just abort any pending rx/tx and just wait
for completion of currently running rx/tx code. That shouldn't take
long.
If it's been submitted to the hardware for DMA, it can't do that very
easily.
And if I can't be bothered to write code to go through the entire damn
queue and inspect every packet to see if it's a data packet and check
the VCI/VPI and try to steal it, it can't be done for the software queue
either :)
The queue ought to be short; if it isn't, then we already screwed up.
The close therefore should be quick, and it *doesn't* have to be
instant.
If someone wants to return immediately, there's always
vcc_release_async()...
i dont think that would be quite the right way to do it.
vcc_release_async() just mark's the vcc for deletion--you still need to
go through and close it eventually. however, nothing would prevent you
from writing a close routine that could just reschedule something
periodically to check to see if the hardware finally finished closing
the vcc and can be reused. the part that needs fixed for this would be
marking the vcc for reuse. you would need to keep the vpi.vci marked
as busy so that someone else doesnt try to reuse it while it is
closing. right now, vcc_destroy_socket() always removes the vcc from
the vcc list -- regardless of whether or not close fully succeeded.
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-28 17:01:23
On Wed, 2012-11-28 at 11:41 -0500, David Miller wrote:
Please:
if (X ||
Y ||
Z)
not:
if (X
|| Y
|| Z)
Thanks. Fixed in both Krzysztof's original pppoatm version, and my
br2684 patch, in the git tree at git://git.infradead.org/~dwmw2/atm.git
I knew there was something else that offended me about the original,
other than just the whitespace (which is also fixed).
--
dwmw2
From: David Miller <davem@davemloft.net> Date: 2012-11-28 17:04:18
From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 28 Nov 2012 17:01:10 +0000
On Wed, 2012-11-28 at 11:41 -0500, David Miller wrote:
quoted
Please:
if (X ||
Y ||
Z)
not:
if (X
|| Y
|| Z)
Thanks. Fixed in both Krzysztof's original pppoatm version, and my
br2684 patch, in the git tree at git://git.infradead.org/~dwmw2/atm.git
I knew there was something else that offended me about the original,
other than just the whitespace (which is also fixed).
Do you want me to pull that tree into net-next or is there a plan to
repost the entire series of work for a final submission?
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-28 17:09:24
On Wed, 2012-11-28 at 12:04 -0500, David Miller wrote:
Do you want me to pull that tree into net-next or is there a plan to
repost the entire series of work for a final submission?
I think it needs a little more testing/consensus first. I'd like an ack
from Chas on the atm ->release_cb() thing, at least. And I wouldn't mind
confirmation from Nathan's customer that they're no longer seeing the
panics.
And then I'll either send an explicit pull request, or submit it as
patches — whichever you prefer.
Hopefully in the next day or so; the merge window is approaching...
--
dwmw2
From: David Miller <davem@davemloft.net> Date: 2012-11-28 17:11:06
From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 28 Nov 2012 17:09:15 +0000
And then I'll either send an explicit pull request, or submit it as
patches ― whichever you prefer.
The canonical thing is to do both, send the pull request in the
"[PATCH 0/N]" email, and then the patches so everyone can see the
final form of the changes.
Hopefully in the next day or so; the merge window is approaching...
From: Krzysztof Mazur <hidden> Date: 2012-11-28 20:18:44
On Tue, Nov 27, 2012 at 07:28:43PM +0100, Krzysztof Mazur wrote:
While reviewing your br2684 patch I also found that some ATM drivers does
not call ->pop() when ->send() fails, they should do:
if (vcc->pop)
vcc->pop(vcc, skb);
else
dev_kfree_skb(skb);
but some drivers just call dev_kfree_skb(skb).
I think that we should add atm_pop() function that does that and fix all
drivers.
I'm sending a patch that implements that idea.
Currently we need two arguments vcc and skb. However, we have reserved
ATM_SKB(skb)->vcc in skb control block for keeping vcc
and we can create single argument version vcc_pop(skb). In that case
we need to move:
ATM_SKB(skb)->vcc = vcc;
from ATM drivers to functions that call atmdev_ops->send().
Krzysiek
-- >8 --
Subject: [PATCH] atm: introduce vcc_pop_*()
The atm drivers to free skb, that they got from ->send(), cannot just use
dev_kfree_skb*(), but they must use something like:
if (vcc->pop)
vcc->pop(vcc, skb);
else
dev_kfree_skb_any(skb);
When vcc->pop is non-NULL, but they must in such case call vcc->pop().
This causes duplicated code in many drivers, and some drivers even forgot
to call vcc->pop() in some error handling code.
The new vcc_pop_*() functions are equivalent to dev_kfree_skb*().
Currently we always use dev_kfree_skb_any() to free, because using
other versions it's probably worthless optimization - in ->pop() we
already use only dev_kfree_skb_any(). The other functions we added
only to not loose information from converting existing code that
uses some non-any dev_kfree_skb*() variants.
Signed-off-by: Krzysztof Mazur <redacted>
---
include/linux/atmdev.h | 11 +++++++++++
net/atm/common.c | 9 +++++++++
2 files changed, 20 insertions(+)
@@ -283,6 +283,17 @@ int atm_pcr_goal(const struct atm_trafprm *tp);voidvcc_release_async(structatm_vcc*vcc,intreply);+/*+*vcc_pop_*()functionsshouldbeusedbyATMdrivertofreetransmitted+*skbs-skbsthatweresenttodriverbyatmdev_opt->send()function.+*+*Weprovidethreefunctionsthatcanbeusedindifferentcontexts.+*Seedev_kfree_skb*()documentationfordetails.+*/+voidvcc_pop_any(structatm_vcc*vcc,structsk_buff*skb);+#define vcc_pop(vcc, skb) vcc_pop_any(vcc, skb)+#define vcc_pop_irq(vcc, skb) vcc_pop_any(vcc, skb)+structatm_ioctl{structmodule*owner;/* A module reference is kept if appropriate over this call.
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-28 21:21:00
On Wed, 28 Nov 2012 21:18:37 +0100
Krzysztof Mazur [off-list ref] wrote:
On Tue, Nov 27, 2012 at 07:28:43PM +0100, Krzysztof Mazur wrote:
quoted
I think that we should add atm_pop() function that does that and fix all
drivers.
I'm sending a patch that implements that idea.
Currently we need two arguments vcc and skb. However, we have reserved
ATM_SKB(skb)->vcc in skb control block for keeping vcc
and we can create single argument version vcc_pop(skb). In that case
we need to move:
ATM_SKB(skb)->vcc = vcc;
from ATM drivers to functions that call atmdev_ops->send().
i dont like the vcc->pop() implementation and at one point i had the
crazy idea of using skb->destructors to handle it. however, i think it
would be necessary to clone the skb's so any existing destructor is
preserved.
From: Krzysztof Mazur <hidden> Date: 2012-11-28 21:45:38
On Wed, Nov 28, 2012 at 04:20:01PM -0500, chas williams - CONTRACTOR wrote:
On Wed, 28 Nov 2012 21:18:37 +0100
Krzysztof Mazur [off-list ref] wrote:
quoted
On Tue, Nov 27, 2012 at 07:28:43PM +0100, Krzysztof Mazur wrote:
quoted
I think that we should add atm_pop() function that does that and fix all
drivers.
I'm sending a patch that implements that idea.
Currently we need two arguments vcc and skb. However, we have reserved
ATM_SKB(skb)->vcc in skb control block for keeping vcc
and we can create single argument version vcc_pop(skb). In that case
we need to move:
ATM_SKB(skb)->vcc = vcc;
from ATM drivers to functions that call atmdev_ops->send().
i dont like the vcc->pop() implementation and at one point i had the
crazy idea of using skb->destructors to handle it. however, i think it
would be necessary to clone the skb's so any existing destructor is
preserved.
With this patch we will kill vcc->pop() in drivers and in future
we can do that without changes in drivers.
don't define these if you dont plan on using them anway.
I removed them. I also added check if vcc is NULL, as David Woodhouse
suggested, some drivers use that.
Krzysiek
-- >8 --
Subject: [PATCH v2] atm: introduce vcc_pop()
The atm drivers to free skb, that they got from ->send(), cannot just use
dev_kfree_skb*(), but they must use something like:
if (vcc->pop)
vcc->pop(vcc, skb);
else
dev_kfree_skb_any(skb);
When vcc->pop() is non-NULL, but they must in such case call vcc->pop().
This causes duplicated code in many drivers, and some drivers even forgot
to call vcc->pop() in some error handling code.
Signed-off-by: Krzysztof Mazur <redacted>
---
include/linux/atmdev.h | 8 ++++++++
net/atm/common.c | 9 +++++++++
2 files changed, 17 insertions(+)
@@ -283,6 +283,14 @@ int atm_pcr_goal(const struct atm_trafprm *tp);voidvcc_release_async(structatm_vcc*vcc,intreply);+/**+*vcc_pop-freetransmittedATMskb+*+*vcc_pop()shouldbeusedbyATMdrivertofreeskbs,thatweresent+*todriverbyatmdev_opt->send()function.+*/+voidvcc_pop(structatm_vcc*vcc,structsk_buff*skb);+structatm_ioctl{structmodule*owner;/* A module reference is kept if appropriate over this call.
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-28 21:59:26
On Wed, 28 Nov 2012 22:45:34 +0100
Krzysztof Mazur [off-list ref] wrote:
On Wed, Nov 28, 2012 at 04:20:01PM -0500, chas williams - CONTRACTOR wrote:
quoted
i dont like the vcc->pop() implementation and at one point i had the
crazy idea of using skb->destructors to handle it. however, i think it
would be necessary to clone the skb's so any existing destructor is
preserved.
With this patch we will kill vcc->pop() in drivers and in future
we can do that without changes in drivers.
From: Krzysztof Mazur <hidden> Date: 2012-11-28 22:10:45
On Wed, Nov 28, 2012 at 04:59:06PM -0500, chas williams - CONTRACTOR wrote:
On Wed, 28 Nov 2012 22:45:34 +0100
Krzysztof Mazur [off-list ref] wrote:
quoted
On Wed, Nov 28, 2012 at 04:20:01PM -0500, chas williams - CONTRACTOR wrote:
quoted
i dont like the vcc->pop() implementation and at one point i had the
crazy idea of using skb->destructors to handle it. however, i think it
would be necessary to clone the skb's so any existing destructor is
preserved.
With this patch we will kill vcc->pop() in drivers and in future
we can do that without changes in drivers.
don't define these if you dont plan on using them anway.
I removed them. I also added check if vcc is NULL, as David Woodhouse
suggested, some drivers use that.
it should probably be if (likely(vcc) && likely(vcc->pop)) since it
will almost always be the case.
Thanks,
Krzysiek
-- >8 --
Subject: [PATCH v3] atm: introduce vcc_pop()
The atm drivers to free skb, that they got from ->send(), cannot just use
dev_kfree_skb*(), but they must use something like:
if (vcc->pop)
vcc->pop(vcc, skb);
else
dev_kfree_skb_any(skb);
When vcc->pop() is non-NULL, but they must in such case call vcc->pop().
This causes duplicated code in many drivers, and some drivers even forgot
to call vcc->pop() in some error handling code.
Signed-off-by: Krzysztof Mazur <redacted>
---
include/linux/atmdev.h | 8 ++++++++
net/atm/common.c | 9 +++++++++
2 files changed, 17 insertions(+)
@@ -283,6 +283,14 @@ int atm_pcr_goal(const struct atm_trafprm *tp);voidvcc_release_async(structatm_vcc*vcc,intreply);+/**+*vcc_pop-freetransmittedATMskb+*+*vcc_pop()shouldbeusedbyATMdrivertofreeskbs,thatweresent+*todriverbyatmdev_opt->send()function.+*/+voidvcc_pop(structatm_vcc*vcc,structsk_buff*skb);+structatm_ioctl{structmodule*owner;/* A module reference is kept if appropriate over this call.
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-28 22:18:54
On Wed, 2012-11-28 at 09:21 +0000, David Laight wrote:
Even when it might make sense to sleep in close until tx drains
there needs to be a finite timeout before it become abortive.
You are, of course, right. We should never wait for hardware for ever.
And just to serve me right, I seem to have hit a bug in the latest Solos
firmware (1.11) which makes it sometimes lock up when I reboot. So it
never responds to the PKT_PCLOSE packet... and thus it deadlocks when I
try to kill pppd and unload the module to reset it :)
New version...
From 53dd01c08fec5b26006a009b25e4210127fdb27a Mon Sep 17 00:00:00 2001
From: David Woodhouse <redacted>
Date: Tue, 27 Nov 2012 23:49:24 +0000
Subject: [PATCH] solos-pci: Wait for pending TX to complete when releasing
vcc
We should no longer be calling the old pop routine for the vcc, after
vcc_release() has completed. Make sure we wait for any pending TX skbs
to complete, by waiting for our own PKT_PCLOSE control skb to be sent.
Signed-off-by: David Woodhouse <redacted>
---
drivers/atm/solos-pci.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
@@ -881,11 +882,18 @@ static void pclose(struct atm_vcc *vcc)header->vci=cpu_to_le16(vcc->vci);header->type=cpu_to_le16(PKT_PCLOSE);+init_completion(&SKB_CB(skb)->c);+fpga_queue(card,SOLOS_CHAN(vcc->dev),skb,NULL);clear_bit(ATM_VF_ADDR,&vcc->flags);clear_bit(ATM_VF_READY,&vcc->flags);+if(!wait_for_completion_timeout(&SKB_CB(skb)->c,+jiffies+msecs_to_jiffies(5000)))+dev_warn(&card->dev->dev,"Timeout waiting for VCC close on port %d\n",+SOLOS_CHAN(vcc->dev));+/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in thetasklethasfinishedprocessinganyincomingpackets(and,moretothepoint,usingthevccpointer).*/
@@ -1011,9 +1019,12 @@ static uint32_t fpga_tx(struct solos_card *card)if(vcc){atomic_inc(&vcc->stats->tx);solos_pop(vcc,oldskb);-}else+}else{+structpkt_hdr*header=(void*)oldskb->data;+if(le16_to_cpu(header->type)==PKT_PCLOSE)+complete(&SKB_CB(oldskb)->c);dev_kfree_skb_irq(oldskb);-+}}}/* For non-DMA TX, write the 'TX start' bit for all four ports simultaneously */
From: Krzysztof Mazur <hidden> Date: 2012-11-28 22:33:05
On Wed, Nov 28, 2012 at 11:10:40PM +0100, Krzysztof Mazur wrote:
On Wed, Nov 28, 2012 at 04:59:06PM -0500, chas williams - CONTRACTOR wrote:
quoted
On Wed, 28 Nov 2012 22:45:34 +0100
Krzysztof Mazur [off-list ref] wrote:
quoted
On Wed, Nov 28, 2012 at 04:20:01PM -0500, chas williams - CONTRACTOR wrote:
quoted
i dont like the vcc->pop() implementation and at one point i had the
crazy idea of using skb->destructors to handle it. however, i think it
would be necessary to clone the skb's so any existing destructor is
preserved.
With this patch we will kill vcc->pop() in drivers and in future
we can do that without changes in drivers.
don't define these if you dont plan on using them anway.
I removed them. I also added check if vcc is NULL, as David Woodhouse
suggested, some drivers use that.
it should probably be if (likely(vcc) && likely(vcc->pop)) since it
will almost always be the case.
I think that we should also add that single-argument skb-only version.
Currently it can be used only after the driver does ATM_SKB(skb)->vcc = vcc.
Most drivers do that.
Thanks,
Krzysiek
-- >8 --
Subject: [PATCH] atm: introduce atm_pop_skb()
Many ATM drivers store vcc in ATM_SKB(skb)->vcc and use it for
freeing skbs. Now they can just use atm_pop_skb() to free such
buffers.
Signed-off-by: Krzysztof Mazur <redacted>
---
include/linux/atmdev.h | 8 ++++++++
net/atm/common.c | 6 ++++++
2 files changed, 14 insertions(+)
@@ -291,6 +291,14 @@ void vcc_release_async(struct atm_vcc *vcc, int reply);*/voidvcc_pop(structatm_vcc*vcc,structsk_buff*skb);+/**+*vcc_pop_skb-freetransmittedATMskb+*+*Thisvariantofvcc_pop()assumesthatATM_SKB(skb)->vccisset+*bydriver.+*/+voidvcc_pop_skb(structsk_buff*skb);+structatm_ioctl{structmodule*owner;/* A module reference is kept if appropriate over this call.
From: Krzysztof Mazur <hidden> Date: 2012-11-29 10:57:20
On Wed, Nov 28, 2012 at 10:18:35PM +0000, David Woodhouse wrote:
quoted hunk
On Wed, 2012-11-28 at 09:21 +0000, David Laight wrote:
quoted
Even when it might make sense to sleep in close until tx drains
there needs to be a finite timeout before it become abortive.
You are, of course, right. We should never wait for hardware for ever.
And just to serve me right, I seem to have hit a bug in the latest Solos
firmware (1.11) which makes it sometimes lock up when I reboot. So it
never responds to the PKT_PCLOSE packet... and thus it deadlocks when I
try to kill pppd and unload the module to reset it :)
New version...
From 53dd01c08fec5b26006a009b25e4210127fdb27a Mon Sep 17 00:00:00 2001
From: David Woodhouse <redacted>
Date: Tue, 27 Nov 2012 23:49:24 +0000
Subject: [PATCH] solos-pci: Wait for pending TX to complete when releasing
vcc
We should no longer be calling the old pop routine for the vcc, after
vcc_release() has completed. Make sure we wait for any pending TX skbs
to complete, by waiting for our own PKT_PCLOSE control skb to be sent.
Signed-off-by: David Woodhouse <redacted>
---
drivers/atm/solos-pci.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
@@ -881,11 +882,18 @@ static void pclose(struct atm_vcc *vcc)header->vci=cpu_to_le16(vcc->vci);header->type=cpu_to_le16(PKT_PCLOSE);+init_completion(&SKB_CB(skb)->c);+fpga_queue(card,SOLOS_CHAN(vcc->dev),skb,NULL);clear_bit(ATM_VF_ADDR,&vcc->flags);clear_bit(ATM_VF_READY,&vcc->flags);+if(!wait_for_completion_timeout(&SKB_CB(skb)->c,+jiffies+msecs_to_jiffies(5000)))+dev_warn(&card->dev->dev,"Timeout waiting for VCC close on port %d\n",+SOLOS_CHAN(vcc->dev));+
do we really need to wait here?
Why don't just do something like that:
tasklet_disable(&card->tlet);
spin_lock(&card->tx_queue_lock);
for each skb in queue
SKB_CB(skb)->vcc = NULL;
spin_unlock(&card->tx_queue_lock);
tasklet_enable(&card->tlet);
or if we really want to call vcc->pop() for such skbs:
tasklet_disable(&card->tlet);
spin_lock(&card->tx_queue_lock);
for each skb in queue {
skb_get(skb);
solos_pop(SKB_CB(skb)->vcc, skb);
SKB_CB(skb)->vcc = NULL;
}
spin_unlock(&card->tx_queue_lock);
tasklet_enable(&card->tlet);
Krzysiek
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-29 11:55:58
On Thu, 2012-11-29 at 11:57 +0100, Krzysztof Mazur wrote:
do we really need to wait here?
Why don't just do something like that:
tasklet_disable(&card->tlet);
spin_lock(&card->tx_queue_lock);
for each skb in queue
SKB_CB(skb)->vcc = NULL;
spin_unlock(&card->tx_queue_lock);
tasklet_enable(&card->tlet);
or if we really want to call vcc->pop() for such skbs:
tasklet_disable(&card->tlet);
spin_lock(&card->tx_queue_lock);
for each skb in queue {
skb_get(skb);
solos_pop(SKB_CB(skb)->vcc, skb);
SKB_CB(skb)->vcc = NULL;
}
spin_unlock(&card->tx_queue_lock);
tasklet_enable(&card->tlet);
Yes, we could certainly remove the packets from the tx_queue first.
However, in the card->using_dma case there might be a skb for this vcc
*currently* being DMA'd, and we'd still need to wait for that one.
I suppose we could just have a waitqueue in *every* TX skb, and under
card->tx_lock we could add ourselves to *that* waitqueue. Or just a
global waitqueue for DMA tx_done, perhaps. But waiting for our own
PKT_PCLOSE skb is just 'cleaner' in my view. It's simpler, and it's much
easier to test. Even if I had DMA-capable hardware, I'd have to get the
right timing to properly test that TX-pending-DMA case.
So dequeuing the packets would only serve to make pclose() slightly
faster, rather than simplifying it. It's hardly a fast path that we care
about, and I've also already ensured that there should only be one or
two packets queued per vcc *anyway*. So I'm mostly inclined not to
bother.
(I did fix the timeout argument to wait_for_completion_timeout())
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-29 12:43:48
On Thu, Nov 29, 2012 at 11:55:43AM +0000, David Woodhouse wrote:
On Thu, 2012-11-29 at 11:57 +0100, Krzysztof Mazur wrote:
quoted
do we really need to wait here?
Why don't just do something like that:
tasklet_disable(&card->tlet);
spin_lock(&card->tx_queue_lock);
for each skb in queue
SKB_CB(skb)->vcc = NULL;
spin_unlock(&card->tx_queue_lock);
tasklet_enable(&card->tlet);
or if we really want to call vcc->pop() for such skbs:
tasklet_disable(&card->tlet);
spin_lock(&card->tx_queue_lock);
for each skb in queue {
skb_get(skb);
solos_pop(SKB_CB(skb)->vcc, skb);
SKB_CB(skb)->vcc = NULL;
}
spin_unlock(&card->tx_queue_lock);
tasklet_enable(&card->tlet);
Yes, we could certainly remove the packets from the tx_queue first.
However, in the card->using_dma case there might be a skb for this vcc
*currently* being DMA'd, and we'd still need to wait for that one.
Removing packets from tx_queue is not needed. We can transmit packets
also after close. We just can't call vcc->pop() after close,
so we can just set SKB_CB(skb)->vcc of such packets to NULL so fpga_tx()
won't call vcc->pop().
Maybe I was not precise enough, I'm think that all we need is
something like that:
-- >8 --
Subject: [PATCH] solos-pci: don't call vcc->pop() after pclose()
After atmdev_ops->close() we cannot use vcc->pop() because the vcc may,
and probably will be destroyed.
We can just set vcc for such frames to NULL because fpga_tx() after
completion will call dev_kfree_skb() in that case.
Signed-off-by: Krzysztof Mazur <redacted>
---
drivers/atm/solos-pci.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-29 12:57:26
On Thu, 2012-11-29 at 13:43 +0100, Krzysztof Mazur wrote:
Removing packets from tx_queue is not needed. We can transmit packets
also after close. We just can't call vcc->pop() after close,
so we can just set SKB_CB(skb)->vcc of such packets to NULL so
fpga_tx() won't call vcc->pop().
Your patch doesn't do that, does it? You'd want something like
if (card->tx_skb[port] && SKB_CB(card->tx_skb[port]->vcc) == vcc)
SKB_CB(card->tx_skb[port]->vcc) = NULL;
Under card->tx_lock should suffice.
And do we just *not* call the ->pop() on that skb ever? And hope that it
doesn't screw up some other state somewhere? Like if we're doing MLPPP
and I've implemented BQL for PPP... we might never call
ppp_completed_queue() for that skb, so even though this *channel* is
going away, it might still contribute towards the perceived queue on the
overall PPP netdev?
Failing to call ->pop() could cause memory leaks and other issues; I
don't think it's reasonable. I think we *have* to wait for
card->tx_skb[port] if it's for the VCC we're closing.
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-29 13:20:26
On Thu, Nov 29, 2012 at 12:57:17PM +0000, David Woodhouse wrote:
On Thu, 2012-11-29 at 13:43 +0100, Krzysztof Mazur wrote:
quoted
Removing packets from tx_queue is not needed. We can transmit packets
also after close. We just can't call vcc->pop() after close,
so we can just set SKB_CB(skb)->vcc of such packets to NULL so
fpga_tx() won't call vcc->pop().
Your patch doesn't do that, does it? You'd want something like
if (card->tx_skb[port] && SKB_CB(card->tx_skb[port]->vcc) == vcc)
SKB_CB(card->tx_skb[port]->vcc) = NULL;
No, I want to process all queued packets, not just only those that
were already sent do card.
In that case we will need to remove other packets with that vcc
from queue, of couse we can still do that in the same loop, something
like:
if (SKB_CB(skb)->vcc == vcc) {
if (card->tx_skb[port] == skb) {
skb_get(skb);
solos_pop(SKB_CB(skb)->vcc, skb);
SKB_CB(skb)->vcc = NULL;
} else {
skb_unlink(skb, &card->tx_queue[port]);
solos_pop(SKB_CB(skb)->vcc, skb);
}
}
But I don't think that this optization is needed.
Under card->tx_lock should suffice.
And do we just *not* call the ->pop() on that skb ever? And hope that it
doesn't screw up some other state somewhere? Like if we're doing MLPPP
and I've implemented BQL for PPP... we might never call
ppp_completed_queue() for that skb, so even though this *channel* is
going away, it might still contribute towards the perceived queue on the
overall PPP netdev?
Failing to call ->pop() could cause memory leaks and other issues; I
don't think it's reasonable. I think we *have* to wait for
card->tx_skb[port] if it's for the VCC we're closing.
We are calling ->pop() in solos_pop() just before SKB_CB(skb)->vcc = NULL,
but we are doing that before we really finish processing that packet,
that's why we do skb_get(skb).
Krzysiek
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-29 14:29:56
On Thu, 29 Nov 2012 11:57:15 +0100
Krzysztof Mazur [off-list ref] wrote:
or if we really want to call vcc->pop() for such skbs:
you need to call ->pop() to cleaning up the wmem_alloc accounting.
otherwise you will get complaints from the atm stack later about
freeing a vcc that had outstanding data.
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-29 14:41:18
On Thu, 29 Nov 2012 13:43:44 +0100
Krzysztof Mazur [off-list ref] wrote:
Removing packets from tx_queue is not needed. We can transmit packets
also after close. We just can't call vcc->pop() after close,
so we can just set SKB_CB(skb)->vcc of such packets to NULL so fpga_tx()
won't call vcc->pop().
i dont think you can transmit packets after close(). you can transmit
packets during close() though. if you transmit after close that means
that you are using the vpi/vci pair that the atm stack thinks is no
longer in use. additionally after close(), the hardware should be in a
state such that you cannot transmit or receive on the vpi/vci that has
been closed.
close() needs to make sure that any pending tx packets are sent or
otherwise disposed of (like turning off the transmit segmentation
engine, clearing the packets, or whatever). any partially reassembled
pdu's also need to be cleared as well.
Um... yes, that would probably work. But it's subtle enough that it
bothers me. And if it *did* cause any strange issues, it's a rare case
and would be hard to reproduce/debug. Is it *really* necessary, just to
speed up the vcc close? I'm inclined to stick with the 'KISS' approach.
(Note that the card->tx_skb[port] skb isn't on the queue any more; you'd
do that bit separately and not in the skb_queue_walk() loop.)
--
dwmw2
Um... yes, that would probably work. But it's subtle enough that it
bothers me. And if it *did* cause any strange issues, it's a rare case
and would be hard to reproduce/debug. Is it *really* necessary, just to
speed up the vcc close? I'm inclined to stick with the 'KISS' approach.
That's why I proposed that simple loop that just calls ->pop() and
sets vcc to NULL.
Forget about that, Chas said that we cannot leave close() until we
close that vcc, so we really need to wait.
Krzysiek
From: Krzysztof Mazur <hidden> Date: 2012-11-29 15:09:49
On Wed, Nov 28, 2012 at 10:18:35PM +0000, David Woodhouse wrote:
quoted hunk
On Wed, 2012-11-28 at 09:21 +0000, David Laight wrote:
quoted
Even when it might make sense to sleep in close until tx drains
there needs to be a finite timeout before it become abortive.
You are, of course, right. We should never wait for hardware for ever.
And just to serve me right, I seem to have hit a bug in the latest Solos
firmware (1.11) which makes it sometimes lock up when I reboot. So it
never responds to the PKT_PCLOSE packet... and thus it deadlocks when I
try to kill pppd and unload the module to reset it :)
New version...
From 53dd01c08fec5b26006a009b25e4210127fdb27a Mon Sep 17 00:00:00 2001
From: David Woodhouse <redacted>
Date: Tue, 27 Nov 2012 23:49:24 +0000
Subject: [PATCH] solos-pci: Wait for pending TX to complete when releasing
vcc
We should no longer be calling the old pop routine for the vcc, after
vcc_release() has completed. Make sure we wait for any pending TX skbs
to complete, by waiting for our own PKT_PCLOSE control skb to be sent.
Signed-off-by: David Woodhouse <redacted>
---
drivers/atm/solos-pci.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
@@ -881,11 +882,18 @@ static void pclose(struct atm_vcc *vcc)header->vci=cpu_to_le16(vcc->vci);header->type=cpu_to_le16(PKT_PCLOSE);+init_completion(&SKB_CB(skb)->c);+fpga_queue(card,SOLOS_CHAN(vcc->dev),skb,NULL);clear_bit(ATM_VF_ADDR,&vcc->flags);clear_bit(ATM_VF_READY,&vcc->flags);+if(!wait_for_completion_timeout(&SKB_CB(skb)->c,+jiffies+msecs_to_jiffies(5000)))+dev_warn(&card->dev->dev,"Timeout waiting for VCC close on port %d\n",+SOLOS_CHAN(vcc->dev));+
I don't like two thinks about this patch:
- if allos_skb(sizeof(*header), GFP_ATOMIC) at beginning of
pclose() fails we will crash
- if card wakes up after this timeout we will probably crash too
That's why proposed different approach, but it has other problems.
Krzysiek
you shouldnt clear ATM_VF_ADDR until the vpi/vci is actually closed and
ready for reuse. at this point, it isnt. ATM_VF_READY should already
be clear at this point but you should set it before you queue your
PKT_CLOSE. these flags probably should be handled outside the drivers
since the context for them is pretty clear. just another patch i never
got around to writing...
checking for ATM_VF_READY in find_vcc() is probably going to give you
grief as well since ATM_VF_READY isnt entirely under your control. you
need to be able to find the vcc until after pclose() is finished since
your tasklet might have a few packets it is still processing?
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-29 15:48:11
On Thu, 2012-11-29 at 16:09 +0100, Krzysztof Mazur wrote:
I don't like two thinks about this patch:
- if allos_skb(sizeof(*header), GFP_ATOMIC) at beginning of
pclose() fails we will crash
- if card wakes up after this timeout we will probably crash too
That's why proposed different approach, but it has other problems.
How about this variant on what you suggested. Yes, we can definitely
remove everything that's in the queue... as long as we use
skb_queue_walk_safe() instead of skb_queue_walk().
We can use GFP_KERNEL instead of GFP_ATOMIC, which at least reduces the
likelihood of failing to close the vcc.
We end up waiting *only* if there is a packet which is *currently* being
DMA'd to the card. And if the card doesn't take that within 5 seconds,
it almost certainly never will. So I can live with that.
I'd definitely want someone with a DMA-capable FPGA to test this
properly, adding printks to it to make sure the interesting path is
being exercised. Nathan, you should be able to trigger it with the same
test that used to just crash the system entirely — send a flood of
packets while you kill br2684ctl.
@@ -868,10 +867,11 @@ static int popen(struct atm_vcc *vcc)staticvoidpclose(structatm_vcc*vcc){structsolos_card*card=vcc->dev->dev_data;-structsk_buff*skb;+structsk_buff*skb,*tmpskb;structpkt_hdr*header;+unsignedcharport=SOLOS_CHAN(vcc->dev);-skb=alloc_skb(sizeof(*header),GFP_ATOMIC);+skb=alloc_skb(sizeof(*header),GFP_KERNEL);if(!skb){dev_warn(&card->dev->dev,"Failed to allocate sk_buff in pclose()\n");return;
@@ -883,22 +883,50 @@ static void pclose(struct atm_vcc *vcc)header->vci=cpu_to_le16(vcc->vci);header->type=cpu_to_le16(PKT_PCLOSE);-init_completion(&SKB_CB(skb)->c);-fpga_queue(card,SOLOS_CHAN(vcc->dev),skb,NULL);clear_bit(ATM_VF_ADDR,&vcc->flags);clear_bit(ATM_VF_READY,&vcc->flags);-if(!wait_for_completion_timeout(&SKB_CB(skb)->c,-msecs_to_jiffies(5000)))-dev_warn(&card->dev->dev,"Timeout waiting for VCC close on port %d\n",-SOLOS_CHAN(vcc->dev));+/* Remove any yet-to-be-transmitted packets from the pending queue */+spin_lock(&card->tx_queue_lock);+skb_queue_walk_safe(&card->tx_queue[port],skb,tmpskb){+if(SKB_CB(skb)->vcc==vcc){+skb_unlink(skb,&card->tx_queue[port]);+solos_pop(vcc,skb);+}+}+spin_unlock(&card->tx_queue_lock);/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in thetasklethasfinishedprocessinganyincomingpackets(and,moretothepoint,usingthevccpointer).*/tasklet_unlock_wait(&card->tlet);++/*+*Ifwe'reinDMAmodeandaskbonthisVCCis*currently*being+*submitted,waitforittofinish(usingparam_wq)+*/+if(card->using_dma){+DEFINE_WAIT(wait);++spin_lock(&card->tx_lock);+while(card->tx_skb[port]&&SKB_CB(card->tx_skb[port])->vcc==vcc){+prepare_to_wait(&card->param_wq,&wait,TASK_UNINTERRUPTIBLE);+spin_unlock(&card->tx_lock);++if(schedule_timeout(5*HZ)){+dev_warn(&card->dev->dev,+"Timeout waiting for VCC close on port %d\n",+port);+gotodone_waiting;+}+spin_lock(&card->tx_lock);+}+spin_unlock(&card->tx_lock);+done_waiting:+finish_wait(&card->param_wq,&wait);+}return;}
@@ -1020,12 +1048,15 @@ static uint32_t fpga_tx(struct solos_card *card)if(vcc){atomic_inc(&vcc->stats->tx);solos_pop(vcc,oldskb);-}else{-structpkt_hdr*header=(void*)oldskb->data;-if(le16_to_cpu(header->type)==PKT_PCLOSE)-complete(&SKB_CB(oldskb)->c);++/*+*Ifit'saTXskbonaclosedVCC,pclose()+*maybewaitingforit...+*/+if(!test_bit(ATM_VF_READY,&vcc->flags))+wake_up(&card->param_wq);+}elsedev_kfree_skb_irq(oldskb);-}}}/* For non-DMA TX, write the 'TX start' bit for all four ports simultaneously */
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-29 15:59:16
On Thu, 2012-11-29 at 10:37 -0500, chas williams - CONTRACTOR wrote:
you shouldnt clear ATM_VF_ADDR until the vpi/vci is actually closed and
ready for reuse. at this point, it isnt.
So I should always wait for the completion of my PKT_CLOSE and only
clear ATM_VF_ADDR when it's actually done?
But can you define 'ready for reuse'? From the moment I clear
ATM_VF_ADDR, another CPU may enter my popen() function to set up another
VCC with the same parameters, and everything should work fine. The
PKT_POPEN will end up on the queue *after* my PKT_PCLOSE for the old
VCC. Any received packets will be dropped until the new VCC gets
ATM_VF_READY set (by the popen function).
What's the actual failure mode, caused by me clearing ATM_VF_ADDR "too
early"?
ATM_VF_READY should already be clear at this point but you should set
it before you queue your PKT_CLOSE.
I should *set* it? Do you mean clear it? Yes, I see it's cleared by
vcc_destroy_socket()... but all the other ATM drivers also seem to clear
it for themselves, and that would appear to be harmless.
checking for ATM_VF_READY in find_vcc() is probably going to give you
grief as well since ATM_VF_READY isnt entirely under your control.
That's fine. If *anyone* has cleared ATM_VF_READY, I stop sending
packets up it. Or, more to the point, I stop using the damn thing at
all. See commit 1f6ea6e511e5ec730d8e88651da1b7b6e8fd1333.
you need to be able to find the vcc until after pclose() is finished since
your tasklet might have a few packets it is still processing?
The whole point of that check is that the tasklet *won't* be able to
find it any more, and it'll just discard incoming packets for the
obsolescent VCC.
--
dwmw2
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-29 16:00:51
On Thu, 29 Nov 2012 15:47:57 +0000
David Woodhouse [off-list ref] wrote:
quoted hunk
@@ -1020,12 +1048,15 @@ static uint32_t fpga_tx(struct solos_card *card) if (vcc) { atomic_inc(&vcc->stats->tx); solos_pop(vcc, oldskb);- } else {- struct pkt_hdr *header = (void *)oldskb->data;- if (le16_to_cpu(header->type) == PKT_PCLOSE)- complete(&SKB_CB(oldskb)->c);++ /*+ * If it's a TX skb on a closed VCC, pclose()+ * may be waiting for it...+ */+ if (!test_bit(ATM_VF_READY, &vcc->flags))+ wake_up(&card->param_wq);+ } else dev_kfree_skb_irq(oldskb);- }
the part that bothers me (and i dont have the programmer's guide for
the solos hardware) is that you are watching for the PKT_PCLOSE to be
sent to the card. shouldnt you be watching for the PKT_PCLOSE to be
returned from the card (assuming it does such a thing) so that you can
be assured that the tx/rx for this vpi/vci pair has been "stopped"?
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-29 16:11:49
On Thu, 29 Nov 2012 15:59:08 +0000
David Woodhouse [off-list ref] wrote:
On Thu, 2012-11-29 at 10:37 -0500, chas williams - CONTRACTOR wrote:
quoted
you shouldnt clear ATM_VF_ADDR until the vpi/vci is actually closed and
ready for reuse. at this point, it isnt.
So I should always wait for the completion of my PKT_CLOSE and only
clear ATM_VF_ADDR when it's actually done?
But can you define 'ready for reuse'? From the moment I clear
ATM_VF_ADDR, another CPU may enter my popen() function to set up another
VCC with the same parameters, and everything should work fine. The
PKT_POPEN will end up on the queue *after* my PKT_PCLOSE for the old
VCC. Any received packets will be dropped until the new VCC gets
ATM_VF_READY set (by the popen function).
What's the actual failure mode, caused by me clearing ATM_VF_ADDR "too
early"?
there may not be one (due to serialization from other parts of the
atm stack) but you "shouldn't" clear ATM_VF_ADDR until the vpi/vci pair
is ready for reuse. by reuse, i mean that any previous rx/tx data in
the vpi/vci segmentation hardware has been removed/cleared.
quoted
ATM_VF_READY should already be clear at this point but you should set
it before you queue your PKT_CLOSE.
I should *set* it? Do you mean clear it? Yes, I see it's cleared by
sorry, i did mean clear it.
vcc_destroy_socket()... but all the other ATM drivers also seem to clear
it for themselves, and that would appear to be harmless.
yeah, like i said, it is spuriously cleared in the drivers and should
probably just be moved to under the control of the next layer up
completely. drivers/atm should just handle the hardware side, not the
software side.
quoted
checking for ATM_VF_READY in find_vcc() is probably going to give you
grief as well since ATM_VF_READY isnt entirely under your control.
That's fine. If *anyone* has cleared ATM_VF_READY, I stop sending
packets up it. Or, more to the point, I stop using the damn thing at
all. See commit 1f6ea6e511e5ec730d8e88651da1b7b6e8fd1333.
quoted
you need to be able to find the vcc until after pclose() is finished since
your tasklet might have a few packets it is still processing?
The whole point of that check is that the tasklet *won't* be able to
find it any more, and it'll just discard incoming packets for the
obsolescent VCC.
that's fine as long as you understand this. in the case of the he, i
needed to be able to find the vcc until close() is finished so that i
can wakeup the sleeper in the close() routine that is waiting for the
reassembly queue to be cleared/reset. also, i still needed to find the
vcc for the tx side during close() since i still might need to pop()
skb's that are being sent during the close() while i am still trying to
get the hardware to shutdown the transmit dma engine.
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-29 16:24:37
On Thu, 2012-11-29 at 10:59 -0500, chas williams - CONTRACTOR wrote:
the part that bothers me (and i dont have the programmer's guide for
the solos hardware) is that you are watching for the PKT_PCLOSE to be
sent to the card. shouldnt you be watching for the PKT_PCLOSE to be
returned from the card (assuming it does such a thing) so that you can
be assured that the tx/rx for this vpi/vci pair has been "stopped"?
Define "stopped".
For the RX case... the other end may *always* take it upon itself to
send us a packet marked with arbitrary VCI/VPI, right? There's no
connection setup for it "on the wire", in the case of PVC?
So bearing that in mind: from the moment ATM_VF_READY gets cleared, as
far as the ATM core is concerned, we will no longer receive packets on
the given VCC. If we receive any, we'll just complain about receiving
packets for an unknown VCI/VPI.
For the TX case ... yes, we need to be sure we aren't continuing to send
packets after our close() routine completes. We *used* to, but the
resulting ->pop() calls were causing problems, and that's why we're
looking at this code path closer. The currently proposed patches (except
one suggestion from Krzyztof that we both shouted down) would fix that.
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-29 16:28:23
On Thu, Nov 29, 2012 at 03:47:57PM +0000, David Woodhouse wrote:
On Thu, 2012-11-29 at 16:09 +0100, Krzysztof Mazur wrote:
quoted
I don't like two thinks about this patch:
- if allos_skb(sizeof(*header), GFP_ATOMIC) at beginning of
pclose() fails we will crash
- if card wakes up after this timeout we will probably crash too
That's why proposed different approach, but it has other problems.
How about this variant on what you suggested. Yes, we can definitely
remove everything that's in the queue... as long as we use
skb_queue_walk_safe() instead of skb_queue_walk().
We can use GFP_KERNEL instead of GFP_ATOMIC, which at least reduces the
likelihood of failing to close the vcc.
We end up waiting *only* if there is a packet which is *currently* being
DMA'd to the card. And if the card doesn't take that within 5 seconds,
it almost certainly never will. So I can live with that.
Yeah, that shouldn't happen.
+ if (!test_bit(ATM_VF_READY, &vcc->flags))
+ wake_up(&card->param_wq);
+ } else
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-29 17:17:18
On Thu, 29 Nov 2012 16:24:29 +0000
David Woodhouse [off-list ref] wrote:
On Thu, 2012-11-29 at 10:59 -0500, chas williams - CONTRACTOR wrote:
quoted
the part that bothers me (and i dont have the programmer's guide for
the solos hardware) is that you are watching for the PKT_PCLOSE to be
sent to the card. shouldnt you be watching for the PKT_PCLOSE to be
returned from the card (assuming it does such a thing) so that you can
be assured that the tx/rx for this vpi/vci pair has been "stopped"?
Define "stopped".
For the RX case... the other end may *always* take it upon itself to
send us a packet marked with arbitrary VCI/VPI, right? There's no
connection setup for it "on the wire", in the case of PVC?
most atm hardware that i am familiar with, wont deliver vpi/vci data
unless you are actually trying to receive it. however, this hardware
is generally doing its reassembly in hardware and delivering aal5
pdu's and needs to manage its memory resources carefully. you might be
trying to reassemble 1000 pdu's from different vpi/vci's.
So bearing that in mind: from the moment ATM_VF_READY gets cleared, as
far as the ATM core is concerned, we will no longer receive packets on
the given VCC. If we receive any, we'll just complain about receiving
packets for an unknown VCI/VPI.
For the TX case ... yes, we need to be sure we aren't continuing to send
packets after our close() routine completes. We *used* to, but the
resulting ->pop() calls were causing problems, and that's why we're
looking at this code path closer. The currently proposed patches (except
one suggestion from Krzyztof that we both shouted down) would fix that.
again part of this is poor synchronization. the detach protocol (i.e.
push of a NULL skb) should be flushing any pending transmits and
shutting down whatever in the protocol is doing any sending and
receiving. however, i release this might be difficult to do since the
detach protocol is invoked in such a strange way.
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-29 18:12:45
On Thu, 2012-11-29 at 12:17 -0500, chas williams - CONTRACTOR wrote:
most atm hardware that i am familiar with, wont deliver vpi/vci data
unless you are actually trying to receive it. however, this hardware
is generally doing its reassembly in hardware and delivering aal5
pdu's and needs to manage its memory resources carefully. you might be
trying to reassemble 1000 pdu's from different vpi/vci's.
In almost *all* cases, there is only one VCC. So much so, in fact, that
I only just realised its firmware seems to crash if you send it a
PKT_POPEN for a new VPI/VCI pair while it's already got one open. But
yes, I think it does actually work in the same way.
We do see the 'packet received for unknown VCC' complaint, after we
reboot the host without resetting the card. And as shown in the commit I
just referenced, we rely on the !ATM_VF_READY check in order to prevent
a use-after-free when the tasklet is sending packets up a VCC that's
just been closed.
quoted
So bearing that in mind: from the moment ATM_VF_READY gets cleared, as
far as the ATM core is concerned, we will no longer receive packets on
the given VCC. If we receive any, we'll just complain about receiving
packets for an unknown VCI/VPI.
For the TX case ... yes, we need to be sure we aren't continuing to send
packets after our close() routine completes. We *used* to, but the
resulting ->pop() calls were causing problems, and that's why we're
looking at this code path closer. The currently proposed patches (except
one suggestion from Krzyztof that we both shouted down) would fix that.
again part of this is poor synchronization. the detach protocol (i.e.
push of a NULL skb) should be flushing any pending transmits and
shutting down whatever in the protocol is doing any sending and
receiving. however, i release this might be difficult to do since the
detach protocol is invoked in such a strange way.
You mean that (e.g.) pppoatm_push(vcc, NULL) should be waiting for any
pending TX skb (that has already been passed off to the driver) to
*complete*? How would it even do that?
--
dwmw2
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-29 18:29:26
On Thu, 29 Nov 2012 18:11:48 +0000
David Woodhouse [off-list ref] wrote:
We do see the 'packet received for unknown VCC' complaint, after we
reboot the host without resetting the card. And as shown in the commit I
just referenced, we rely on the !ATM_VF_READY check in order to prevent
a use-after-free when the tasklet is sending packets up a VCC that's
just been closed.
well that behavior is just crap. why is it delivering cells for a
vpi/vci pair that is not open? regardless, i pointed out the behavior
of find_vcc() just in case you need to do some clean up on data that is
coming back while you are attempting to finish operations during your
driver's close() but this cleanup might not be happening since you
arent able to get a reference to the vcc so you can pop() the data or
whatever you might need to do.
quoted
again part of this is poor synchronization. the detach protocol (i.e.
push of a NULL skb) should be flushing any pending transmits and
shutting down whatever in the protocol is doing any sending and
receiving. however, i release this might be difficult to do since the
detach protocol is invoked in such a strange way.
You mean that (e.g.) pppoatm_push(vcc, NULL) should be waiting for any
pending TX skb (that has already been passed off to the driver) to
*complete*? How would it even do that?
i think the order of the vcc_destroy_socket() operations is a bit
wrong. it should call detach protocol (i.e. push a NULL). this should
cause the attached protocol to stop any future sends and receives, and
it CAN sleep in this context (and only this context -- generally
sending cannot sleep which is why this might seem confusing) to do
whatever it needs to do to wait for the attached protocol to clean up
queues, flush data etc.
then the driver close() should be called. this takes care of cleaning
up any pending tx or rx that is in the hardware. and of course,
close() can sleep since it will be in a interrutible context.
the pop() might be screwed up here though. you might have skb's in the
driver that should be pop()'ed with the formerly attached protocol.
you could wait for pending tx's sent to the driver. you know that your
attached protocol's pop() will be called. keep count of the
outstanding transmit skb's. you cant do this though if you close() the
vcc before you detach the protocol.
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-29 22:17:23
On Thu, 2012-11-29 at 13:29 -0500, chas williams - CONTRACTOR wrote:
On Thu, 29 Nov 2012 18:11:48 +0000
David Woodhouse [off-list ref] wrote:
quoted
We do see the 'packet received for unknown VCC' complaint, after we
reboot the host without resetting the card. And as shown in the commit I
just referenced, we rely on the !ATM_VF_READY check in order to prevent
a use-after-free when the tasklet is sending packets up a VCC that's
just been closed.
well that behavior is just crap. why is it delivering cells for a
vpi/vci pair that is not open?
In the reboot case... Because it *was* open and the device has no way of
knowing that the host just rebooted. We don't reset the card on loading
the driver, because that would cause an ADSL resync.
Perhaps we could send a 'close all VCCs' command to the firmware though.
Nathan, could we add that to the firmware?
Or we could just respond to any unwanted incoming packet by sending a
close for that specific VCC. And be careful about potential races with
open().
In the close case... The *tasklet* is running to process an incoming
packet, finds an open and active VCC and is *about* to send a packet up
to it. Meanwhile, our close() runs and the VCC is destroyed. And then
the tasklet... oops, use-after-free and crash.
Hence commit 1f6ea6e51 which makes the tasklet refuse to process RX for
a VCC which doesn't have the ATM_VF_READY flag set. Because it knows
it's *being* closed. And the close() routine waits for any *existing*
tasklet run to finish, to make sure nobody's referencing the VCC, before
it returns and allows vcc_destroy_socket() to complete. It's the RCU
principle, basically.
quoted
You mean that (e.g.) pppoatm_push(vcc, NULL) should be waiting for any
pending TX skb (that has already been passed off to the driver) to
*complete*? How would it even do that?
i think the order of the vcc_destroy_socket() operations is a bit
wrong. it should call detach protocol (i.e. push a NULL). this should
cause the attached protocol to stop any future sends and receives, and
it CAN sleep in this context (and only this context -- generally
sending cannot sleep which is why this might seem confusing) to do
whatever it needs to do to wait for the attached protocol to clean up
queues, flush data etc.
then the driver close() should be called. this takes care of cleaning
up any pending tx or rx that is in the hardware. and of course,
close() can sleep since it will be in a interrutible context.
This is basically what Krzysztof's patch 1/7 was doing, which we've now
dropped from the series.
There are issues with *either* ordering.
The current case is that we call vcc->dev->ops->close(vcc) *first*,
before vcc->push(vcc, NULL). And that means that the device is told to
close the VCC while the protocol may still be pushing packets to it.
Hence the patches to both pppoatm and br2684 to make them check for
ATM_VF_READY and *stop* pushing packets if it's not set.
If we flip it round and tell the protocol first, then it tears down all
its data structures while the driver is still happily calling its
->pop() on transmitted skbs. Which leads to the panic in br2684_pop()
that we've *also* seen (because we weren't actually flushing the TX
packets in the driver's close(), which had the same effect). Yes, you
suggest that the protocol could keep track of the skbs it's sent down
and wait for them... but surely it's better to let the driver get called
first and *abort* them?
At this point, I think we're better off as we are (with Krzysztof's
patch 1/7 dropped, and leaving vcc->dev->ops->close() being called
before vcc->push(NULL). We've fairly much solved the issues with that
arrangement, by checking ATM_VF_READY in the protocols' ->push()
functions.
--
dwmw2