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

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(-)

On Tue, 2012-10-30 at 20:52 +0100, Krzysztof Mazur wrote:
> 
> --- 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;

Does this break the pvcc->blocked handling that coordinates with
pppoatm_pop()?

If we have one packet in flight, so pppoatm_may_send() permits a new one
to be queued... but they're *large* packets to sk_wmem_alloc doesn't
permit it. Immediately after the check, pppoatm_pop() runs and leaves
the queue empty. We return zero, blocking the queue… which never gets
woken because we didn't set the BLOCKED flag and thus the tasklet never
runs.

In fact, I think we need the BLOCKED handling for the
sock_owned_by_user() case too? When the VCC is actually closed, I
suppose that's not recoverable and we don't care about waking the queue
anyway? But any time we end up returning zero from pppoatm_send(), we
*need* to ensure that a wakeup will happen in future unless the socket
is actually dead.