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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1351678578.8774.8.camel@shinybook.infradead.org>
Date:	Wed, 31 Oct 2012 10:16:18 +0000
From:	David Woodhouse <dwmw2@...radead.org>
To:	Krzysztof Mazur <krzysiek@...lesie.net>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of
 vcc

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

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (6171 bytes)