[v2,3/3] pppoatm: protect against freeing of vcc

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.

On Thu, 29 Nov 2012 15:47:57 +0000
David Woodhouse <dwmw2@infradead.org> wrote:


> @@ -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"?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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

according to CodingStyle:

+ } else {
>  				dev_kfree_skb_irq(oldskb);
> -			}
+ }

Krzysiek
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 29 Nov 2012 16:24:29 +0000
David Woodhouse <dwmw2@infradead.org> wrote:

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

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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

On Thu, 29 Nov 2012 18:11:48 +0000
David Woodhouse <dwmw2@infradead.org> 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.

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2012-11-29 at 13:29 -0500, chas williams - CONTRACTOR wrote:
> On Thu, 29 Nov 2012 18:11:48 +0000
> David Woodhouse <dwmw2@infradead.org> 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?

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.

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

In message <1354227428.21562.230.camel@shinybook.infradead.org>,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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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

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:
> > 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:
> > 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()?
> 
> 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.