lkml.org 
[lkml]   [2008]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH] [TCP]: Fix fackets corruption during transitional state

    Ingrediments:

    - Sacktag can call tcp_fragment() for those skbs that will get
    discard in tcp_clean_rtx_queue(), even though some of the
    other things are "optimized away".
    - snd_una is advanced very early in tcp_ack()
    - tcp_highest_sack_seq() uses tp->snd_una when sacked_out is
    zero

    These together make it possible to get fackets_out adjusted
    while sacked_out remains zero (it can then later on be
    incremented as well but then incorrect fackets_out is already
    getting happily used). The corruption can occur only when no
    SACK info is previously known and a DSACK below snd_una arrives.
    I think an old ACK with some SACK info could do the same as
    well.

    Significant corruption is not likely to happen in practice
    (explains the rareness of reports) because it requires quite
    complex things to end up into a situation where fragmentation due
    to arriving SACK block does actually cause packets_out (and the
    related fackets_out) adjustment to become necessary.

    In addition to above, we fix fackets_out in fastretrans_alert if
    sacked_out remained zero (at the same place where the WARN_ON
    triggers), so some SACK info above snd_una must arrive too to
    make the incorrect state to have some persistency.

    In general, this causes mainly off-by-one error in fackets_out
    (though it might occur "twice in a row" to get it more off).
    That can cause premature entry to recovery and minor miscounts
    of other state that depends on fackets_out amount. However,
    quite often we'd immediately hit the WARN_ON trap and fix the
    invariant. Even if persistent for a short duration, threat of
    this bug is quite low, couple of extra packets at most,
    sometimes it could have even made the recovery to begin more
    timely, which normally isn't done to avoid spurious recoveries
    due to potential reordering (which didn't occur near the majority
    of the recoveries anyway).

    To be honest, so far I've not been able to find out a scenario
    where it would be caused by something that isn't either buggy or
    malicious SACK block (not that it wouldn't exists, I'm just too
    dumb to find that :-)). Win in malicious case would really be
    close to zero, besides spamming the log.

    We could optimize fragmenting of soon to be discarded part away
    from sacktag as well (it might still be necessary for DSACKs
    though, so not much gain currently). However, I don't want to do
    such, more scary thing now due to stable potential of this fix,
    and such optimization approach requires more thinking to not
    introduce other problems.

    Heh, it helped nothing to verify whether those fackets_out
    touching functions successfully kept invariants intact (did
    that at least twenty times and never found anything) because
    the function they depended on weas breaking the other thing
    (I though that it would be an invariant too) during update
    snd_una in tcp_ack->sacktag->clean_rtx_queue period.

    Reported by couple of people in the past and finally Andy
    Furniss <lists@andyfurniss.entadsl.com> who had a
    reproducable setup for this (with some amount of waiting of
    course :-)) and was able to try with my debug patches.

    Should fix Bugzilla #10346.

    Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
    Reported-by: Andy Furniss <lists@andyfurniss.entadsl.com>
    Reported-by: Georgi Chorbadzhiyski <gf@unixsol.org>
    Cc: Rafael J. Wysocki <rjw@sisk.pl>
    Cc: Arjan van de Ven <arjan@infradead.org>
    ---
    net/ipv4/tcp_output.c | 9 ++++++++-
    1 files changed, 8 insertions(+), 1 deletions(-)

    diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
    index d29ef79..8c1578b 100644
    --- a/net/ipv4/tcp_output.c
    +++ b/net/ipv4/tcp_output.c
    @@ -783,7 +783,14 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
    tcp_dec_pcount_approx_int(&tp->sacked_out, diff);
    tcp_verify_left_out(tp);
    }
    - tcp_adjust_fackets_out(sk, skb, diff);
    +
    + /* Sacktag may use tcp_fragment for skb with seq < snd_una
    + * which is incompatible what tcp_highest_sack_seq() used in
    + * tcp_adjust_fackets_out assumes. Under non-trivial
    + * conditions that would cause problems.
    + */
    + if (tp->sacked_out)
    + tcp_adjust_fackets_out(sk, skb, diff);
    }

    /* Link BUFF into the send queue. */
    --
    1.5.2.2
    \
     
     \ /
      Last update: 2008-05-01 22:33    [W:5.205 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site