lkml.org 
[lkml]   [2014]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 3.12 059/103] net: sctp: test if association is dead in sctp_wake_up_waiters
    Date
    From: Daniel Borkmann <dborkman@redhat.com>

    3.12-stable review patch. If anyone has any objections, please let me know.

    ===============

    [ Upstream commit 1e1cdf8ac78793e0875465e98a648df64694a8d0 ]

    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 when a sctp_primitive_SEND() call
    from sctp_sendmsg() fails, and tries to flush an association's
    outq, i.e. in the following way:

    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. We could also use
    list_del_init() in addition when we call sctp_association_free(),
    but as Vlad suggests, we want to trap such bugs and thus leave
    it poisoned as is.

    Why is it safe to resolve the issue by testing for asoc->base.dead?
    Parallel calls to sctp_sendmsg() are protected under socket lock,
    that is lock_sock()/release_sock(). Only within that path under
    lock held, we're setting skb/chunk owner via sctp_set_owner_w().
    Eventually, chunks are freed directly by an association still
    under that lock. So when traversing association list on destruction
    time from sctp_wake_up_waiters() via sctp_wfree(), a different
    CPU can't be running sctp_wfree() while another one calls
    sctp_association_free() as both happens under the same lock.
    Therefore, this can also not race with setting/testing against
    asoc->base.dead as we are guaranteed for this to happen in order,
    under lock. Further, Vlad says: the times we check asoc->base.dead
    is when we've cached an association pointer for later processing.
    In between cache and processing, the association may have been
    freed and is simply still around due to reference counts. We check
    asoc->base.dead under a lock, so it should always be safe to check
    and not race against sctp_association_free(). Stress-testing seems
    fine now, too.

    Fixes: cd253f9f357d ("net: sctp: wake up all assocs if sndbuf policy is per socket")
    Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
    Cc: Vlad Yasevich <vyasevic@redhat.com>
    Acked-by: Neil Horman <nhorman@tuxdriver.com>
    Acked-by: Vlad Yasevich <vyasevic@redhat.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Jiri Slaby <jslaby@suse.cz>
    ---
    net/sctp/socket.c | 6 ++++++
    1 file changed, 6 insertions(+)

    diff --git a/net/sctp/socket.c b/net/sctp/socket.c
    index a20fa7f0aedc..dc496f2bc6c5 100644
    --- a/net/sctp/socket.c
    +++ b/net/sctp/socket.c
    @@ -6575,6 +6575,12 @@ static void sctp_wake_up_waiters(struct sock *sk,
    if (asoc->ep->sndbuf_policy)
    return __sctp_write_space(asoc);

    + /* If association goes down and is just flushing its
    + * outq, then just normally notify others.
    + */
    + if (asoc->base.dead)
    + return sctp_write_space(sk);
    +
    /* Accounting for the sndbuf space is per socket, so we
    * need to wake up others, try to be fair and in case of
    * other associations, let them have a go first instead
    --
    1.9.3


    \
     
     \ /
      Last update: 2014-05-29 17:41    [W:4.053 / U:0.716 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site