RX/close vcc race with solos/atmtcp/usbatm/he

I've had this crash reported to me...

[18842.727906] EIP: [<e082f490>] br2684_push+0x19/0x234 [br2684] SS:ESP 0068:dfb89d14 

[18845.090712] [<c13ecff3>] ? do_page_fault+0x0/0x2e1 
[18845.120042] [<e082f490>] ? br2684_push+0x19/0x234 [br2684] 
[18845.153530] [<e084fa13>] solos_bh+0x28b/0x7c8 [solos_pci] 
[18845.186488] [<e084f711>] ? solos_irq+0x2d/0x51 [solos_pci] 
[18845.219960] [<c100387b>] ? handle_irq+0x3b/0x48 
[18845.247732] [<c10265cb>] ? irq_exit+0x34/0x57 
[18845.274437] [<c1025720>] tasklet_action+0x42/0x69 
[18845.303247] [<c102643f>] __do_softirq+0x8e/0x129 
[18845.331540] [<c10264ff>] do_softirq+0x25/0x2a 
[18845.358274] [<c102664c>] _local_bh_enable_ip+0x5e/0x6a 
[18845.389677] [<c102666d>] local_bh_enable+0xb/0xe 
[18845.417944] [<e08490a8>] ppp_unregister_channel+0x32/0xbb [ppp_generic] 
[18845.458193] [<e08731ad>] pppox_unbind_sock+0x18/0x1f [pppox] 
[18845.492712] [<e087f5f7>] pppoe_device_event+0xa7/0x159 [pppoe] 
[18845.528269] [<c13ed2ff>] notifier_call_chain+0x2b/0x4a 
[18845.559674] [<c1038a62>] raw_notifier_call_chain+0xc/0xe 
[18845.592110] [<c1300867>] dev_close+0x51/0x8b 
[18845.618266] [<c1300927>] rollback_registered_many+0x86/0x15e 
[18845.652813] [<c1300ab3>] unregister_netdevice_queue+0x67/0x91 
[18845.687849] [<c1300b79>] unregister_netdev+0x14/0x1c 
[18845.718221] [<e082f4d1>] br2684_push+0x5a/0x234 [br2684] 
[18845.750676] [<e083dc21>] vcc_release+0x64/0x100 [atm] 

The problem is that the 'find_vcc' functions in these drivers are returning a vcc with the ATM_VF_READY bit cleared, because it's already in the process of being destroyed. If we fix that simple oversight, there's still a race condition because the socket can still be closed (and completely freed, afaict) between our call to find_vcc() and our call to vcc->push() in the RX tasklet.

Here's a patch for solos-pci which should fix it. We prevent the race by making the dev->ops->close() function wait for the RX tasklet to complete, so it can't still be using the vcc in question.

I think this same approach should work OK for usbatm and he. Less sure about atmtcp -- we may need some extra locking there to protect atmtcp_c_send(). And I'm ignoring eni_proc_read() for now.

Can anyone see a better approach -- short of rewriting the whole ATM layer to make the locking saner?

index c5f5186..a73f102 100644
@@ -774,7 +774,8 @@ static struct atm_vcc *find_vcc(struct atm_dev *dev, short vpi, int vci)
 	sk_for_each(s, node, head) {
 		vcc = atm_sk(s);
 		if (vcc->dev == dev && vcc->vci == vci &&
-		    vcc->vpi == vpi && vcc->qos.rxtp.traffic_class != ATM_NONE)
+		    vcc->vpi == vpi && vcc->qos.rxtp.traffic_class != ATM_NONE &&
+		    test_bit(ATM_VF_READY, &vcc->flags))
 			goto out;
 	}
 	vcc = NULL;

@@ -900,6 +901,10 @@ static void pclose(struct atm_vcc *vcc)
 	clear_bit(ATM_VF_ADDR, &vcc->flags);
 	clear_bit(ATM_VF_READY, &vcc->flags);
 
+	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
+	   tasklet has finished processing any incoming packets (and, more to
+	   the point, using the vcc pointer). */
+	tasklet_unlock_wait(&card->tlet);
 	return;
 }
 

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

I do not think this is problem with usbatm. We use custom vcc_list with custom usbatm_vcc_data entries. We remove entry on atmdev_ops->close() within tasklet_disable()/tasklet_enable() section. After that rx tasklet will not find usbatm_vcc_data for corresponding vpi, vpi.

Stanislaw

From: David Woodhouse <[email protected]>
Date: Wed, 26 May 2010 12:16:24 +0100

There is no doubt in my mind that these VCC objects need to be refcounted when used like this.

The only other alternative is to make use of something like RCU.

Perhaps. Although in the general case they're tied to the 'struct sock' and don't need to outlive it. These drivers which look up the VCC to feed incoming packets to it are the only exception to that rule that I'm aware of.

I agree. In fact the use of tasklet_unlock_wait() in my patch is what I settled on when I went looking for 'something like RCU' to solve this particular case. I was _going_ to add RCU stuff, but realised that this was sufficient.

In the close() path we clear the READY bit in the VCC, wait for the tasklet to finish using it, and only then do we destroy the VCC.

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

Nathan, did you manage to get your customer to confirm that this fixes the problem? It'd be useful to get this into 2.6.35 and -stable.

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

No, I still haven't heard back from the customer. I'll keep sending email reminders.

I've had confirmation from the customer. The patch fixed his problem.

In message <[email protected]>,David Woodhouse

...

vcc's are really sockets, so you could just increase the refcount -- sock_hold(). after you push the packet, drop the refcount, sock_put() and hopefully things will be well. however, i think the close routines dont really expect this behavior so the card driver's vcc close might need to be changed to wait around if the refcount on the vcc is > 1. or perhaps the driver independent part needs to do this.

the he driver works around this issue by holding vcc_sklist_lock around the find_vcc and ->push() which happen to occur in the same tasklet.

There are rules about where we're allowed to call sock_hold(), and I don't think our find_vcc() functions can be made to meet them.

In that case I think we might as well stick with the RCU-like solution I already implemented in the vcc close function -- which is just to wait for the tasklet to complete, thus ensuring that it's no longer using the defunct vcc.

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

In message <[email protected]>,David Woodhouse

if you are using find_vcc() then you should already have a lock on the hash table for the vccs. you can safely increment the ref count at this point.

the driver shouldnt close the vcc while the tasklet is running and using the vcc in question. i guess the safest thing is to simply do as you are doing and not close while running the tasklet.

You can still hit the oops that way -- br2684_push() is setting vcc->user_back to NULL before the final sock_put() anyway, and that's what was causing the oops.

OK. :)

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

In message <[email protected]>,David Woodhouse

i dont understand. if you do a sock_hold() in find_vcc(), and then call vcc->push() you should be able to call vcc->push() and then sock_put(). the solos driver is broken since the result find_vcc() does not do anything to keep the resulting vcc from evaporating. find_vcc() should be taking a reference on the vcc, or the lock needs to held around any usage of the results from find_vcc().

Holding the reference doesn't stop the problem. The problem is

vcc_release()
--> vcc_destroy_socket()
--> br2684_push(vcc, NULL)
sets vcc->user_back = NULL
(which it what causes the oops when try try to feed it any
subsequent packets).

Only _later_ does vcc_release() call sock_put().

It doesn't _matter_ that the tasklet is holding a reference on the socket, because it's not the sk_free() which is causing the problem.

Just making dev->ops->close() wait for the tasklet is perfectly sufficient. That call happens from vcc_destroy_socket() before the call to br2684_push(), and all is well.

dwmw2

In message <[email protected]>,David Woodhouse

hmm... perhaps this routine needs to take the vcc_sklist_lock because it is going to modify the vcc. or we need to use locking on the vcc itself. it seems like the lock in vcc_remove_socket() just needs move up one routine to encompass this whole 'closing' operation. the vcc is going away. we dont want people to be able to find it and use it. it is not enough to just flip the flags on the vcc obviously.

you took a reference to an object inside a hashed list and didnt do anything to prevent the object from leaving the hashed list. that is stil not correct IMHO.

Or move the ->push(vcc, NULL) and anything else which destroys the state, so that it happens later. Use a real socket destructor function which will be called from sk_free() after the last sock_put().

Yeah yeah, but I fixed that already with the RCU-like approach of synchronising with the tasklet on dev->ops->close(). So I don't _need_ the reference.

dwmw2

We were seeing faults in the solos-pci receive tasklet when packets arrived for a VCC which was currently being closed:

[18842.727906] EIP: [<e082f490>] br2684_push+0x19/0x234 [br2684] SS:ESP 0068:dfb89d14 

[18845.090712] [<c13ecff3>] ? do_page_fault+0x0/0x2e1 
[18845.120042] [<e082f490>] ? br2684_push+0x19/0x234 [br2684] 
[18845.153530] [<e084fa13>] solos_bh+0x28b/0x7c8 [solos_pci] 
[18845.186488] [<e084f711>] ? solos_irq+0x2d/0x51 [solos_pci] 
[18845.219960] [<c100387b>] ? handle_irq+0x3b/0x48 
[18845.247732] [<c10265cb>] ? irq_exit+0x34/0x57 
[18845.274437] [<c1025720>] tasklet_action+0x42/0x69 
[18845.303247] [<c102643f>] __do_softirq+0x8e/0x129 
[18845.331540] [<c10264ff>] do_softirq+0x25/0x2a 
[18845.358274] [<c102664c>] _local_bh_enable_ip+0x5e/0x6a 
[18845.389677] [<c102666d>] local_bh_enable+0xb/0xe 
[18845.417944] [<e08490a8>] ppp_unregister_channel+0x32/0xbb [ppp_generic] 
[18845.458193] [<e08731ad>] pppox_unbind_sock+0x18/0x1f [pppox] 

This patch uses an RCU-inspired approach to fix it. In the RX tasklet's find_vcc() function we first refuse to use a VCC which already has the ATM_VF_READY bit cleared. And in the VCC close function, we synchronise with the tasklet to ensure that it can't still be using the VCC before we continue and allow the VCC to be destroyed.

---
Nathan, you probably still ought to work out why your customer's setup keeps disconnecting and closing the connection -- under normal operation on a stable DSL line, this race should almost never have been triggered.

index c5f5186..a73f102 100644
@@ -774,7 +774,8 @@ static struct atm_vcc *find_vcc(struct atm_dev *dev, short vpi, int vci)
 	sk_for_each(s, node, head) {
 		vcc = atm_sk(s);
 		if (vcc->dev == dev && vcc->vci == vci &&
-		    vcc->vpi == vpi && vcc->qos.rxtp.traffic_class != ATM_NONE)
+		    vcc->vpi == vpi && vcc->qos.rxtp.traffic_class != ATM_NONE &&
+		    test_bit(ATM_VF_READY, &vcc->flags))
 			goto out;
 	}
 	vcc = NULL;

@@ -900,6 +901,10 @@ static void pclose(struct atm_vcc *vcc)
 	clear_bit(ATM_VF_ADDR, &vcc->flags);
 	clear_bit(ATM_VF_READY, &vcc->flags);
 
+	/* Hold up vcc_destroy_socket() (our caller) until solos_bh() in the
+	   tasklet has finished processing any incoming packets (and, more to
+	   the point, using the vcc pointer). */
+	tasklet_unlock_wait(&card->tlet);
 	return;
 }
 

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

From: David Woodhouse <[email protected]>
Date: Fri, 06 Aug 2010 17:17:36 +0100

...

Applied, thanks David.