From: Nathan Williams <hidden> Date: 2012-11-30 01:18:12
On Wed, 2012-11-28 at 17:09 +0000, David Woodhouse wrote:
On Wed, 2012-11-28 at 12:04 -0500, David Miller wrote:
quoted
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.
The customer has confirmed that they haven't seen any panics. I tested
these patches on OpenWrt with Kernel 3.3.8 and couldn't get a panic:
c118dc5 solos-pci: Fix leak of skb received for unknown vcc
e539793 br2684: fix module_put() race
3656320 br2684: don't send frames on not-ready vcc
753f920 solos-pci: Wait for pending TX to complete when releasing vcc
91ab2cf pppoatm: do not inline pppoatm_may_send()
85b48fa pppoatm: drop frames to not-ready vcc
3ac1080 pppoatm: take ATM socket lock in pppoatm_send()
e41faed pppoatm: fix module_put() race
3b1a914 pppoatm: allow assign only on a connected socket
ec809bd atm: add owner of push() callback to atmvcc
ae088d6 atm: br2684: Fix excessive queue bloat
I haven't tested these ones:
230a012 pppoatm: fix missing wakeup in pppoatm_send()
1c0c800 atm: Add release_cb() callback to vcc
From: Chas Williams (CONTRACTOR) <hidden> Date: 2012-11-30 01:39:18
In message [off-list ref],David Woodhouse writes:
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.
it isnt clear to me that fixes the race entirely either.
vcc_destroy_socket() and any of the push()/sends()'s are not serialized.
while you may clear the ATM_VF_READY flag, you might not clear it soon
enough for any particular push() that is already running. so it still
seems like you are racing close() against push() at this point. the
window is greatly reduced, but it still exists.
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-30 01:57:10
On Thu, 2012-11-29 at 20:38 -0500, Chas Williams (CONTRACTOR) wrote:
it isnt clear to me that fixes the race entirely either.
vcc_destroy_socket() and any of the push()/sends()'s are not
serialized.
while you may clear the ATM_VF_READY flag, you might not clear it soon
enough for any particular push() that is already running. so it still
seems like you are racing close() against push() at this point. the
window is greatly reduced, but it still exists.
I think it's actually fixed for pppoatm by the bh_lock_sock() and the
sock_owned_by_user() check. As soon as vcc_release() calls lock_sock(),
pppoatm stops accepting packets.
It should be simple enough to do the same in br2684.
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-30 08:25:40
On Fri, 2012-11-30 at 01:57 +0000, David Woodhouse wrote:
I think it's actually fixed for pppoatm by the bh_lock_sock() and the
sock_owned_by_user() check. As soon as vcc_release() calls lock_sock(),
pppoatm stops accepting packets.
It should be simple enough to do the same in br2684.
Um... but now I come to look at it... Krzysztof, doesn't your 'pppoatm:
take ATM socket lock in pppoatm_send()' patch actually *break* the case
of sending via vcc_sendmsg()?
Why did you include the sock_owned_by_user() check in there and not just
use bh_lock_sock()?
With the sock_owned_by_user() check, it'll *always* drop packets
submitted through vcc_sendmsg(), won't it?
Admittedly, for PPPoATM and BR2684 we never do want to have packets
submitted directly from userspace that way; they should all come via the
PPP channel or the netdev respectively. So we might want to keep the
sock_owned_by_user() check because it fixes the close race, and
explicitly document it.
But it doesn't necessarily work for other protocols, so we may need a
better solution for the general case. Perhaps drop the
sock_owned_by_user() check, and put bh_lock_sock() around the beginning
of vcc_destroy_socket() where it clears ATM_VF_READY? That'll ensure
that no ->push() is *currently* operating on a skb having seen that the
VCC is still open.
Or maybe we just make the *devices* check the ATM_VF_CLOSE flag and
refuse to send the skb? Put the entire thing into their domain. Although
that may involve extra locking in the driver to synchronise send() and
close() sufficiently.
I'm still reluctant to swap the order of the device/protocol close in
vcc_destroy_socket(). I think that'll just swap one set of problems
which is now fairly well-understood and mostly solved, for another set.
In particular, I think the device needs to see the close first, because
*it* can actually abort or flush any pending TX and RX (including
synchronising with its tasklet as solos-pci does, etc.). Only then does
the protocol tear its data structures down. But I suppose the new set of
problems could be found and overcome, if Chas wants to propose an
alternative patch set...
--
dwmw2
From: Krzysztof Mazur <hidden> Date: 2012-11-30 09:54:03
On Fri, Nov 30, 2012 at 08:25:22AM +0000, David Woodhouse wrote:
On Fri, 2012-11-30 at 01:57 +0000, David Woodhouse wrote:
quoted
I think it's actually fixed for pppoatm by the bh_lock_sock() and the
sock_owned_by_user() check. As soon as vcc_release() calls lock_sock(),
pppoatm stops accepting packets.
It should be simple enough to do the same in br2684.
Um... but now I come to look at it... Krzysztof, doesn't your 'pppoatm:
take ATM socket lock in pppoatm_send()' patch actually *break* the case
of sending via vcc_sendmsg()?
no, in case of
pppoatm_send()
vcc_sendmsg()
the vcc_sendmsg() will just wait for releasing sk->sk_lock.slock.
When the vcc_sendmsg() gets lock first
vcc_sendmsg()
pppoatm_send()
The pppoatm_send() might spin for a while for sk->sk_lock.slock, but
after lock_sock() the vcc_sendmsg() releases that lock and
pppoatm_send() will acquire it and notice that locked is locked
(sock_owned_by_user() returns true) and will just block pppoatm,
and will be woken up in release_sock() (fix was fixed by your 10/17
patch).
Why did you include the sock_owned_by_user() check in there and not just
use bh_lock_sock()?
because bh_lock_sock() will succeeds even with concurrent vcc_sendmsg()
and will have some races in that case.
With the sock_owned_by_user() check, it'll *always* drop packets
submitted through vcc_sendmsg(), won't it?
No, sock_owned_by_user() is just in pppoatm_send() and instead of
dropping packets we block pppd.
Admittedly, for PPPoATM and BR2684 we never do want to have packets
submitted directly from userspace that way; they should all come via the
PPP channel or the netdev respectively. So we might want to keep the
sock_owned_by_user() check because it fixes the close race, and
explicitly document it.
It fixes also races with vcc_sendmsg(). If we really don't wont
vcc_sendmsg() with pppoatm and br2684 we must do some protection
than vcc_sendmsg() will fail instead of racing with pppoatm_send()
and crashing with some drivers that does not support concurent
->send().
But it doesn't necessarily work for other protocols, so we may need a
better solution for the general case. Perhaps drop the
sock_owned_by_user() check, and put bh_lock_sock() around the beginning
of vcc_destroy_socket() where it clears ATM_VF_READY? That'll ensure
that no ->push() is *currently* operating on a skb having seen that the
VCC is still open.
Or maybe we just make the *devices* check the ATM_VF_CLOSE flag and
refuse to send the skb? Put the entire thing into their domain. Although
that may involve extra locking in the driver to synchronise send() and
close() sufficiently.
We need some additional synchronizization with pppoatm_send(), now
we use:
tasklet_kill(&pvcc->wakeup_tasklet);
ppp_unregister_channel(&pvcc->chan);
In ppp_unregister_channel() we will synchronize with the function
calling pppoatm_send() using "downl" lock.
And this must be done in pppoatm.
I'm still reluctant to swap the order of the device/protocol close in
vcc_destroy_socket(). I think that'll just swap one set of problems
which is now fairly well-understood and mostly solved, for another set.
In particular, I think the device needs to see the close first, because
*it* can actually abort or flush any pending TX and RX (including
synchronising with its tasklet as solos-pci does, etc.). Only then does
the protocol tear its data structures down. But I suppose the new set of
problems could be found and overcome, if Chas wants to propose an
alternative patch set...
I think that the current order is good, now we have:
1. stop_sending_fames to protocol
now TX is shut down
(currently done by
set_bit(ATM_VF_CLOSE, &vcc->flags);
clear_bit(ATM_VF_READY, &vcc->flags);
)
2. close_device to device
now RX is shut down
3. device_was_closed to protocol
ugly push(NULL), but we can add some other callback.
we also can do:
1. disable RX to device
now RX is shut down
2. detach to protocol
now TX is shut down
(now protocol can fully detach because RX is disabled)
3. close_device to device
(device is not used anymore)
Krzysiek
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-30 12:10:30
On Fri, 2012-11-30 at 10:53 +0100, Krzysztof Mazur wrote:
On Fri, Nov 30, 2012 at 08:25:22AM +0000, David Woodhouse wrote:
quoted
On Fri, 2012-11-30 at 01:57 +0000, David Woodhouse wrote:
quoted
I think it's actually fixed for pppoatm by the bh_lock_sock() and the
sock_owned_by_user() check. As soon as vcc_release() calls lock_sock(),
pppoatm stops accepting packets.
It should be simple enough to do the same in br2684.
Um... but now I come to look at it... Krzysztof, doesn't your 'pppoatm:
take ATM socket lock in pppoatm_send()' patch actually *break* the case
of sending via vcc_sendmsg()?
no,
... oops, sorry. My sleep-deprived brain thought that we were calling
pppoatm_send() *from* vcc_sendmsg() with the lock held. But of course
we're not; we're calling directly into the driver. So that's OK.
In that case I think we're fine. I'll just do the same thing in
br2684_push(), fix up the comment you just corrected, and we're all
good.
I think that the current order is good, now we have:
1. stop_sending_fames to protocol
now TX is shut down
(currently done by
set_bit(ATM_VF_CLOSE, &vcc->flags);
clear_bit(ATM_VF_READY, &vcc->flags);
)
Right, with the caveat the the socket lock is required for
synchronisation on this. But that's OK. Or we *could* perhaps introduce
an explicit call into the protocol for it, if we really wanted. But I'm
inclined not to.
2. close_device to device
now RX is shut down
3. device_was_closed to protocol
ugly push(NULL), but we can add some other callback.
we also can do:
1. disable RX to device
now RX is shut down
2. detach to protocol
now TX is shut down
(now protocol can fully detach because RX is disabled)
Careful. You have to flush the TX packets which are currently in-flight.
It's not sufficient just to stop sending any more. And you have to do it
*before* the data structures are torn down.
3. close_device to device
(device is not used anymore)
Really, what we're saying is that *one* of the driver or protocol close
functions needs to be split, and we need to do DPD or PDP. Since the
device driver *can* abort/flush the TX queue and also any pending RX
being handled by a tasklet, I think it makes most sense to keep it in
the middle, with the protocol being handled first and last... which is
the current order, as long as we consider setting ATM_VF_CLOSE to be the
first part.
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-30 16:23:59
On Fri, 2012-11-30 at 12:10 +0000, David Woodhouse wrote:
In that case I think we're fine. I'll just do the same thing in
br2684_push(), fix up the comment you just corrected, and we're all
good.
OK, here's an update to me my patch 8/17 'br2684: don't send frames on
not-ready vcc'. It takes the socket lock and does fairly much the same
thing as your pppoatm version. It returns NETDEV_TX_BUSY and stops the
queue if the socket is locked, and it gets woken from the ->release_cb
callback.
I've dropped your Acked-By: since it's mostly new, but feel free to give
me a fresh one. With this I think we're done.
Unless Chas has any objections, I'll ask Dave to pull it...
From 47d5ad4c98452bcddfd00da1c659dac85202f213 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw2@infradead.org>
Date: Tue, 27 Nov 2012 23:28:36 +0000
Subject: [PATCH] br2684: don't send frames on not-ready vcc
Avoid submitting packets to a vcc which is being closed. Things go badly
wrong when the ->pop method gets later called after everything's been
torn down.
Use the ATM socket lock for synchronisation with vcc_destroy_socket(),
which clears the ATM_VF_READY bit under the same lock. Otherwise, we
could end up submitting a packet to the device driver even after its
->ops->close method has been called. And it could call the vcc's ->pop
method after the protocol has been shut down. Which leads to a panic.
Signed-off-by: David Woodhouse <redacted>
---
net/atm/br2684.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 45 insertions(+), 3 deletions(-)
@@ -378,6 +417,7 @@ static void br2684_close_vcc(struct br2684_vcc *brvcc)list_del(&brvcc->brvccs);write_unlock_irq(&devs_lock);brvcc->atmvcc->user_back=NULL;/* what about vcc->recvq ??? */+brvcc->atmvcc->release_cb=brvcc->old_release_cb;brvcc->old_push(brvcc->atmvcc,NULL);/* pass on the bad news */kfree(brvcc);module_put(THIS_MODULE);
@@ -554,9 +594,11 @@ static int br2684_regvcc(struct atm_vcc *atmvcc, void __user * arg)brvcc->encaps=(enumbr2684_encaps)be.encaps;brvcc->old_push=atmvcc->push;brvcc->old_pop=atmvcc->pop;+brvcc->old_release_cb=atmvcc->release_cb;barrier();atmvcc->push=br2684_push;atmvcc->pop=br2684_pop;+atmvcc->release_cb=br2684_release_cb;/* initialize netdev carrier state */if(atmvcc->dev->signal==ATM_PHY_SIG_LOST)
From: Krzysztof Mazur <hidden> Date: 2012-11-30 17:00:14
On Fri, Nov 30, 2012 at 04:23:46PM +0000, David Woodhouse wrote:
+static void br2684_release_cb(struct atm_vcc *atmvcc)
+{
+ struct br2684_vcc *brvcc = BR2684_VCC(atmvcc);
+
+ /*
+ * A race with br2684_xmit_vcc() might cause a spurious wakeup just
+ * after that function *stops* the queue, and qspace might actually
+ * go negative before the queue stops again. We cope with that.
+ */
We cannot race with br2684_xmit_vcc() because both br2684_xmit_vcc()
and br2684_release_cb() are called with locked sk->sk_lock.slock.
+ if (atomic_read(&brvcc->qspace) > 0)
+ netif_wake_queue(brvcc->device);
+
+ if (brvcc->old_release_cb)
+ brvcc->old_release_cb(atmvcc);
+}
Except that comment, the patch looks good:
Acked-by: Krzysztof Mazur <redacted>
Krzysiek
From: chas williams - CONTRACTOR <hidden> Date: 2012-11-30 17:14:25
On Fri, 30 Nov 2012 16:23:46 +0000
David Woodhouse [off-list ref] wrote:
On Fri, 2012-11-30 at 12:10 +0000, David Woodhouse wrote:
quoted
In that case I think we're fine. I'll just do the same thing in
br2684_push(), fix up the comment you just corrected, and we're all
good.
OK, here's an update to me my patch 8/17 'br2684: don't send frames on
not-ready vcc'. It takes the socket lock and does fairly much the same
thing as your pppoatm version. It returns NETDEV_TX_BUSY and stops the
queue if the socket is locked, and it gets woken from the ->release_cb
callback.
I've dropped your Acked-By: since it's mostly new, but feel free to give
me a fresh one. With this I think we're done.
Unless Chas has any objections, I'll ask Dave to pull it...
no objections. i think this deals with my concerns. as for splitting
the close functions, from one of your previous messages:
Really, what we're saying is that *one* of the driver or protocol close
functions needs to be split, and we need to do DPD or PDP. Since the
device driver *can* abort/flush the TX queue and also any pending RX
being handled by a tasklet, I think it makes most sense to keep it in
the middle, with the protocol being handled first and last... which is
the current order, as long as we consider setting ATM_VF_CLOSE to be the
first part.
i believe this is essentially already done with the release_cb()
implementation right? that is splitting the protocol detach/shutdown
into two parts.
From: Krzysztof Mazur <hidden> Date: 2012-11-30 17:39:41
On Fri, Nov 30, 2012 at 12:12:56PM -0500, chas williams - CONTRACTOR wrote:
quoted
Really, what we're saying is that *one* of the driver or protocol close
functions needs to be split, and we need to do DPD or PDP. Since the
device driver *can* abort/flush the TX queue and also any pending RX
being handled by a tasklet, I think it makes most sense to keep it in
the middle, with the protocol being handled first and last... which is
the current order, as long as we consider setting ATM_VF_CLOSE to be the
first part.
i believe this is essentially already done with the release_cb()
implementation right? that is splitting the protocol detach/shutdown
into two parts.
partially, release_cb() is about ATM socket locking. To avoid some races
we need to take the ATM socket lock in protocol send function
(br2684_start_xmit, pppoatm_send, ...). That functions are executed
in bh context and we cannot sleep and wait for releasing the ATM socket
lock, so we just block sending and when the ATM socket is unlocked
release_cb() is called and we re-enabling sending.
Currently the first part of detach is just:
lock_sock(sk)
(without latest "br2684: don't send frames on not-ready vcc"
the first part was
set_bit(ATM_VF_CLOSE, &vcc->flags);
clear_bit(ATM_VF_READY, &vcc->flags);
for br2684)
After that the protocol stops sending new packets so the vcc may be
fully closed by ATM driver. The protocol is still ready to process
received packets. After vcc is closed the protocol can safely detach
knowing that no new packets will be received.
Krzysiek
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-11-30 18:33:49
On Fri, 2012-11-30 at 18:00 +0100, Krzysztof Mazur wrote:
On Fri, Nov 30, 2012 at 04:23:46PM +0000, David Woodhouse wrote:
quoted
+static void br2684_release_cb(struct atm_vcc *atmvcc)
+{
+ struct br2684_vcc *brvcc = BR2684_VCC(atmvcc);
+
+ /*
+ * A race with br2684_xmit_vcc() might cause a spurious wakeup just
+ * after that function *stops* the queue, and qspace might actually
+ * go negative before the queue stops again. We cope with that.
+ */
We cannot race with br2684_xmit_vcc() because both br2684_xmit_vcc()
and br2684_release_cb() are called with locked sk->sk_lock.slock.
Ah, right. For some reason I thought the lock was already dropped when
->release_cb() was called. In that case I'll remove the comment. Thanks.
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2012-12-03 13:22:53
On Wed, 2012-11-28 at 23:33 +0100, Krzysztof Mazur wrote:
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>
Note that this one didn't make it into the tree that Dave just pulled.
Not that I didn't think it was a good idea, but it was just separate
from the other "real" fixes — and the tree had already grown into a big
enough pile from your original single patch!
In [off-list ref] you posted another patch:
I think there is another problem here. The pppoatm gets a reference
to atmvcc, but I don't see anything that protects against removal
of that vcc.
The vcc uses vcc->sk socket for reference counting, so sock_hold()
and sock_put() should be used by pppoatm.
That one I think *isn't* needed, because we have properly fixed the
races with vcc_destroy_socket(). I just wanted to check you agree...?
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
From: Krzysztof Mazur <hidden> Date: 2012-12-03 20:11:23
On Mon, Dec 03, 2012 at 01:22:41PM +0000, David Woodhouse wrote:
On Wed, 2012-11-28 at 23:33 +0100, Krzysztof Mazur wrote:
quoted
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>
Note that this one didn't make it into the tree that Dave just pulled.
Not that I didn't think it was a good idea, but it was just separate
from the other "real" fixes ??? and the tree had already grown into a big
enough pile from your original single patch!
That patch is a preparation of separate series. The current version
(far from final version) is available at:
git://git.podlesie.net/km/linux.git atm-pop
and
http://git.podlesie.net/gitweb.cgi?p=km/linux.git;a=shortlog;h=refs/heads/atm-pop
Patch 3 and especially patch 4 are far from being ready. They are also ugly
because many ATM drivers use strange coding style and I tried to use that
style because using different style for new code would be probably be even worse.
Currently there are 4 patches:
atm: introduce vcc_pop()
atm: introduce vcc_pop_skb()
atm: convert drivers to use vcc_pop*()
atm: add missing vcc_pop*() calls in drivers
The first two introduce two helpers vcc_pop() and vcc_pop_skb(). The third
should be 1:1 conversion of vcc->pop() users to vcc_pop*() interface.
The forth patch fixes some problems I've found. In all cases the bugs
occurs in error handling code, in most cases dev_kfree_skb() is used
instead of vcc_pop(), in some cases driver just returns some error
code and skb is never freed, in two cases I removed the vcc->pop()
call in code like:
static int eni_send(struct atm_vcc *vcc,struct sk_buff *skb)
{
[...]
if (!skb) {
printk(KERN_CRIT "!skb in eni_send ?\n");
if (vcc->pop) vcc->pop(vcc,skb);
return -EINVAL;
}
I don't think that we should check for !skb and even if skb == NULL
it's not a good idea to call vcc->pop() because it will crash.
Current diffstat:
drivers/atm/adummy.c | 5 +---
drivers/atm/ambassador.c | 34 ++++++++++++++--------------
drivers/atm/atmtcp.c | 15 ++++--------
drivers/atm/eni.c | 11 ++++-----
drivers/atm/firestream.c | 19 ++--------------
drivers/atm/fore200e.c | 23 ++++---------------
drivers/atm/he.c | 33 ++++++---------------------
drivers/atm/horizon.c | 31 +++++++++----------------
drivers/atm/idt77252.c | 32 +++++++-------------------
drivers/atm/iphase.c | 59 +++++++++++++-----------------------------------
drivers/atm/lanai.c | 18 ++++-----------
drivers/atm/nicstar.c | 31 ++++++++-----------------
drivers/atm/solos-pci.c | 5 +---
drivers/atm/zatm.c | 13 ++++-------
drivers/usb/atm/usbatm.c | 17 ++++----------
include/linux/atmdev.h | 16 +++++++++++++
net/atm/common.c | 15 ++++++++++++
17 files changed, 128 insertions(+), 249 deletions(-)
In [off-list ref] you posted another patch:
quoted
I think there is another problem here. The pppoatm gets a reference
to atmvcc, but I don't see anything that protects against removal
of that vcc.
The vcc uses vcc->sk socket for reference counting, so sock_hold()
and sock_put() should be used by pppoatm.
That one I think *isn't* needed, because we have properly fixed the
races with vcc_destroy_socket(). I just wanted to check you agree...?
It was never really needed, I removed it from v3.
Thanks,
Krzysiek