Re: [PATCH 4/5] [TCP]: Discard fuzzy SACK blocks

["Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3971 bytes --]

On Fri, 24 Aug 2007, David Miller wrote:

> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Mon, 20 Aug 2007 16:16:32 +0300
> 
> > SACK processing code has been a sort of russian roulette as no
> > validation of SACK blocks is previously attempted. Besides, it
> > is not very clear what all kinds of broken SACK blocks really
> > mean (e.g., one that has start and end sequence numbers
> > reversed). So now close the roulette once and for all.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> 
> Thanks a lot for coding this up, I like it a lot, applied.
> 
> I have some minor worries about the D-SACK lower bound, but
> it's probably OK and I'm just being paranoid :-)

...Please tell what is your concern rather than hinting :-), or is it 
just a hunch?...

Elsewhere we do a similar checking anyway (I might eventually end up
dropping this check in dsack as duplicate due to other planned changes 
but it's necessary still as the validation is being done in the mainloop 
after check_dsack call):

        /* D-SACK for already forgotten data... Do dumb counting. */
        if (dup_sack &&
            !after(end_seq_0, prior_snd_una) &&
            after(end_seq_0, tp->undo_marker))
                tp->undo_retrans--;

...It's natural that due to HW duplication that we get DSACK below 
undo_marker when state <= CA_CWR. In general, they almost always mean 
exactly that, a HW duplication, so instead IMHO we should account them as 
DSACK lying bit in sack_ok and disable DSACK undos for that flow (similar 
case is !EVER_RETRANS skb gets DSACKed). In theory, it could be delayed 
rexmission or something but we've already lost our state already and 
cannot verify that, so choosing the conservative dsack-lying bit there 
seems fine and should even be right thing for majority of the cases 
anyway. I was planning to do something along those lines later...

The key problem here was (in the previous version that was in tcp-2.6), 
that the whole validation business becomes rather useless, if any invalid 
SACK blocks (those that really aren't DSACK but due to bug, malicious or 
whatever reason) below snd_una gets accepted as DSACK, that's basically 
the thing I wanted to avoid as it will be significant for the half of 
the seqno space... Now there's hopefully a bit smaller window for such
garbage... :-)

Key exits from tcp_is_sackblock_valid reside before those checks anyway, 
so they shouldn't be that problematic in performance wise either. ...I was 
thinking of adding unlikely to the latter checks but wasn't too sure if 
that's wise thing to do as malicious entity could push TCP to do them 
(basically at will), Comments?

One additional note: the mainloop operates anyway only in the seqno range 
above prior_snd_una (earlier skbs already being dropped), that can't ever 
be < undo_marker (so some of the DSACK checks are not yet strictly 
necessary but I wanted to do things right from the beginning as I might 
end up re-placing validation soo ;-)). ...So our discussion currently 
probably covers seqno range that is not going to have any significance at 
all... :-) 


Maybe my "Too old" comment deserves some additional explanation:

[PATCH] [TCP]: More verbose comment to DSACK validation

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv4/tcp_input.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 8692d0b..cd187c6 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1070,7 +1070,10 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
 	if (!before(start_seq, tp->undo_marker))
 		return 1;
 
-	/* Too old */
+	/* Too old (it no longer has any significance to TCP state though
+	 * it can be valid; for more complete explanation see comment above
+	 * and similar validation done in tcp_check_dsack())
+	 */
 	if (!after(end_seq, tp->undo_marker))
 		return 0;
 
-- 
1.5.0.6