Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc

[Krzysztof Mazur <krzysiek@podlesie.net>]

On Tue, Oct 30, 2012 at 08:07:25PM +0100, Krzysztof Mazur wrote:
> On Tue, Oct 30, 2012 at 09:37:48AM +0000, David Woodhouse wrote:
> > 
> > Should we be locking it earlier, so that the atm_may_send() call is also
> > covered by the lock?
> 
> Yes, but only to protect against concurent vcc_sendmsg().
> 
> > 
> > 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>
> > 
> 

David, I think we should also fix the issue with sk_sndbuf < MTU,
which is described in comment in pppoatm_may_send() added by
your "pppoatm: Fix excessive queue bloat" patch.

The vcc_sendmsg() already does that.

Krzysiek

-- >8 --
Subject: [PATCH] pppoatm: fix sending packets when sk_sndbuf < MTU

Now pppoatm_send() works, when sk_sndbuf is smaller than MTU. This
issue was already pointed in comment:

	/*
	 * It's not clear that we need to bother with using atm_may_send()
	 * to check we don't exceed sk->sk_sndbuf. If userspace sets a
	 * value of sk_sndbuf which is lower than the MTU, we're going to
	 * block for ever. But the code always did that before we introduced
	 * the packet count limit, so...
	 */

The test is copied from alloc_tx() which is used by vcc_sendmsg().

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 net/atm/pppoatm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 4cc81b5..f25536b 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -306,12 +306,9 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
 
 	/*
 	 * It's not clear that we need to bother with using atm_may_send()
-	 * to check we don't exceed sk->sk_sndbuf. If userspace sets a
-	 * value of sk_sndbuf which is lower than the MTU, we're going to
-	 * block for ever. But the code always did that before we introduced
-	 * the packet count limit, so...
+	 * to check we don't exceed sk->sk_sndbuf.
 	 */
-	if (!atm_may_send(vcc, skb->truesize))
+	if (sk_wmem_alloc_get(sk_atm(vcc)) && !atm_may_send(vcc, skb->truesize))
 		goto nospace_unlock_sock;
 
 	atomic_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
-- 
1.8.0.172.g62af90c