| Message ID | 1350926091-12642-2-git-send-email-krzysiek@podlesie.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 <David.Woodhouse@intel.com>
In message <1350926091-12642-2-git-send-email-krzysiek@podlesie.net>,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. -- 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 Tue, Oct 30, 2012 at 10:26:46AM -0400, Chas Williams (CONTRACTOR) wrote: > In message <1350926091-12642-2-git-send-email-krzysiek@podlesie.net>,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 -- 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
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c index 0dcb5dc..e3b2d69 100644 --- a/net/atm/pppoatm.c +++ b/net/atm/pppoatm.c @@ -270,6 +270,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) { struct pppoatm_vcc *pvcc = chan_to_pvcc(chan); struct atm_vcc *vcc; + int ret; ATM_SKB(skb)->vcc = pvcc->atmvcc; pr_debug("(skb=0x%p, vcc=0x%p)\n", skb, pvcc->atmvcc); @@ -304,17 +305,24 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb) } vcc = ATM_SKB(skb)->vcc; + bh_lock_sock(sk_atm(vcc)); + if (sock_owned_by_user(sk_atm(vcc))) + goto nospace_unlock_sock; if (test_bit(ATM_VF_RELEASED, &vcc->flags) || test_bit(ATM_VF_CLOSE, &vcc->flags) || !test_bit(ATM_VF_READY, &vcc->flags)) - goto nospace; + goto nospace_unlock_sock; atomic_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc); ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options; pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev); - return ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb) + ret = ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb) ? DROP_PACKET : 1; + bh_unlock_sock(sk_atm(vcc)); + return ret; +nospace_unlock_sock: + bh_unlock_sock(sk_atm(vcc)); nospace: /* * We don't have space to send this SKB now, but we might have
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 <krzysiek@podlesie.net> Cc: David Woodhouse <dwmw2@infradead.org> --- net/atm/pppoatm.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)