Re: [PATCH net v2] net: sctp: test if association is dead in sctp_wake_up_waiters

[Neil Horman <nhorman@tuxdriver.com>]

On Wed, Apr 09, 2014 at 12:32:48PM +0200, Daniel Borkmann wrote:
> On 04/09/2014 10:09 AM, Daniel Borkmann wrote:
> >On 04/09/2014 01:10 AM, Vlad Yasevich wrote:
> > > On 04/08/2014 06:23 PM, Daniel Borkmann wrote:
> > >> In function sctp_wake_up_waiters() we need to involve a test
> > >> if the association is declared dead. If so, we don't have any
> > >> reference to a possible sibling association anymore and need
> > >> to invoke sctp_write_space() instead and normally walk the
> > >> socket's associations and notify them of new wmem space. The
> > >> reason for special casing is that, otherwise, we could run
> > >> into the following issue:
> > >>
> > >> sctp_association_free()
> > >> `-> list_del(&asoc->asocs)         <-- poisons list pointer
> > >>      asoc->base.dead = true
> > >>      sctp_outq_free(&asoc->outqueue)
> > >>      `-> __sctp_outq_teardown()
> > >>       `-> sctp_chunk_free()
> > >>        `-> consume_skb()
> > >>         `-> sctp_wfree()
> > >>          `-> sctp_wake_up_waiters() <-- dereferences poisoned pointers
> > >>                                         if asoc->ep->sndbuf_policy=0
> > >>
> > >> Therefore, only walk the list in an 'optimized' way if we find
> > >> that the current association is still active. It's also more
> > >> clean in that context to just use list_del_init() when we call
> > >> sctp_association_free(). Stress-testing seems fine now.
> > >
> > > One of the reasons that we don't use list_del_init() here is that
> > > we want to be able to trap on uninitialized/corrupt list manipulation,
> > > just like you did.  If it wasn't there, the bug would have been hidden.
> > >
> > > Please keep it there.  The rest of the patch is fine.
> >
> >Test run over night and I've seen no issues.
> >
> >But I'd still question the usage of asoc->base.dead though, I think
> >this approach of testing for asoc->base.dead is a bit racy (perhaps
> >general usage of it, imho) - at least here there's a tiny window where
> >we poison pointers before we actually declare the associaton dead.
> >
> >Also, I think even if we would have deleted ourselves from the list
> >after declaring the association dead, a different CPU accessing this
> >association via sctp_wfree() might already have gotten past the
> >asoc->base.dead test while we declare it dead in the meantime.
> 
> Ok, I think we can scratch that thought ... what happens is that parallel
> calls to sctp_sendmsg() are protected under lock_sock()/release_sock()
> pair as already stated in the code and within that lock, we are setting
> sctp_set_owner_w() for each chunk. When we call sctp_primitive_SEND(),
> still under lock, we might eventually end up in sctp_packet_transmit(),
> if I follow the path correctly, and orphan the skb in sctp_packet_set_owner_w()
> [ which basically would mean, we actually uncharge the accounted memory by
> orphaning _before_ we call dev_queue_xmit() since commit 4c3a5bdae293
> ("sctp: Don't charge for data in sndbuf again when transmitting packet")
> but that's perhaps a different story ] and set a new destructor. The
> only thing where in that context an association can be freed up by
> sctp_association_free() is if sctp_primitive_SEND() returns with error.
> So even in that case, we're still protected under lock_sock()/release_sock()
> when we flush the outq, so testing asoc->base.dead should be okay then,
> quite unintuitive though. Thus, patch seems fine, if wished, I could
> still document that in the commit message? Vlad, are we on the same page? ;)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Both to the patch, and the documentation, as its not at all clear what lock
protects the reading and writing of the dead variable.
Neil