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
smime.p7s
Description: S/MIME cryptographic signature
