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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

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.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]