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

[David Woodhouse <dwmw2@infradead.org>]

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

On Wed, 2012-10-31 at 12:30 +0100, Krzysztof Mazur wrote:
> Yes, original patch had also the same problem with sock_owned_by_user(),
> so I just incorrectly assumed that we can do "goto nospace" after
> pppoatm_may_send(), but ppooatm_may_send() must be the last test.
> 
> So I just moved all other tests earlier and and now pppoatm_may_send()
> is also protected by ATM socket lock as you suggested earlier.

I don't think that's sufficient. When we return zero from
pppoatm_send(), the generic PPP code considers the channel to be
blocked, and it won't send any more data to it, ever, until we call
ppp_output_wakeup(). Which we do from a tasklet, triggered in
pppoatm_pop() *iff* the BLOCKED flag is set.

So we play silly buggers in pppoatm_may_send() to ensure that *if* we're
going to return zero, we make damn sure the BLOCKED flag is set and that
pppoatm_pop() is going to see that it's set. There are extensive
comments in pppoatm_pop() and pppoatm_may_send() which try to explain
this. It works because there's *always* going to be packet in flight if
we say that the sk_wmem is full, so of course there's *always* going to
be a later call to pppoatm_pop() to wake things up.

However, if you're going to return zero from pppoatm_send() when
sock_owned_by_user() is true, what guarantees that ppp_output_wakeup()
will ever be called?

-- 
dwmw2


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