Suspicious fackets_out handling

Suspicious fackets_out handling

[Ilpo Järvinen]

Hi,


While looking through the users of fackets_out, i found this from 
tcp_fragment(...):

	/* If this packet has been sent out already, we must
         * adjust the various packet counters.
         */
        if (!before(tp->snd_nxt, TCP_SKB_CB(buff)->end_seq)) {
		int diff = old_factor - tcp_skb_pcount(skb) -
                        tcp_skb_pcount(buff);

		[...snip...]

		if (diff > 0) {
                        /* Adjust Reno SACK estimate. */
                        if (!tp->rx_opt.sack_ok) {
                                tp->sacked_out -= diff;
                                if ((int)tp->sacked_out < 0)
                                        tp->sacked_out = 0;
                                tcp_sync_left_out(tp);
                        }

                        tp->fackets_out -= diff;
                        if ((int)tp->fackets_out < 0)
                                tp->fackets_out = 0;
                }
	}
	[...]

There are IMHO two problems in it. First of all, nothing ensures that the 
skb TCP is fragmenting is actually below the forwardmost sack block (and 
thus is included to the fackets_out)... What I'm not sure of though, is 
how to fix this in net-2.6(.22), it is due to the fact that there is no 
pointer/seq which can be used in testing for it like in tcp-2.6 which has 
the highest_sack. Second problem is even more obvious: if adjustment here 
is being done and the sacktag code then uses fastpath at the arrival of 
the next ACK, the sacktag code will use a stale value from 
fastpath_cnt_hint and fails to notice all that math TCP did here unless we 
start clearing fastpath_skb_hint.


-- 
 i.

Re: Suspicious fackets_out handling

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 23 Apr 2007 14:28:21 +0300 (EEST)

> There are IMHO two problems in it. First of all, nothing ensures that the 
> skb TCP is fragmenting is actually below the forwardmost sack block (and 
> thus is included to the fackets_out)...

Good catch, I agree with your analysis completely.

> What I'm not sure of though, is how to fix this in net-2.6(.22), it
> is due to the fact that there is no pointer/seq which can be used in
> testing for it like in tcp-2.6 which has the highest_sack.

We can add highest_sack to 2.6.22 in order to fix a bug like this,
if necessary.

> Second problem is even more obvious: if adjustment here is being
> done and the sacktag code then uses fastpath at the arrival of the
> next ACK, the sacktag code will use a stale value from
> fastpath_cnt_hint and fails to notice all that math TCP did here
> unless we start clearing fastpath_skb_hint.

Let's try not to clear fastpath_skb_hint here if possible :-)

Is it possible to update fastpath_cnt_hint properly perhaps?

Re: Suspicious fackets_out handling

[Ilpo Järvinen]

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

On Thu, 31 May 2007, David Miller wrote:

> From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Mon, 23 Apr 2007 14:28:21 +0300 (EEST)
> 
> > There are IMHO two problems in it. First of all, nothing ensures that the 
> > skb TCP is fragmenting is actually below the forwardmost sack block (and 
> > thus is included to the fackets_out)...

...this is so old thing already that I had to refresh my memory, 
haven't been expecting any answer for ages... :-)

> Good catch, I agree with your analysis completely.

...I later on understood (from a comment I found elsewhere) that 
fackets_out is occasionally estimated, rather than exact value (which 
could be completely fixed in tcp-2.6 due to presence of highest_sack but 
I'm also considering complete removal of it too, like you probably 
noticed).

> > What I'm not sure of though, is how to fix this in net-2.6(.22), it
> > is due to the fact that there is no pointer/seq which can be used in
> > testing for it like in tcp-2.6 which has the highest_sack.
> 
> We can add highest_sack to 2.6.22 in order to fix a bug like this,
> if necessary.

...I think we can leave it as estimate for now, it has been like that
for years and nobody has complained... :-) This problem covers really 
marginal number of cases anyway I think (isn't the diff usually
negative: old - new after fragment should be negative unless mss_now 
bites).

After I found the comments and analyzed some sacked_out code with NewReno, 
I think that usually these estimates are indicated in the code with 
tcp_dec_pcount_approx(...) but it wasn't used in this specific case 
because it inputs skb instead of int, I'll add clarification of this into 
my tcp-2.6 todo list... 

> > Second problem is even more obvious: if adjustment here is being
> > done and the sacktag code then uses fastpath at the arrival of the
> > next ACK, the sacktag code will use a stale value from
> > fastpath_cnt_hint and fails to notice all that math TCP did here
> > unless we start clearing fastpath_skb_hint.
> 
> Let's try not to clear fastpath_skb_hint here if possible :-)

Np, however, whatever we do, it wouldn't really execute that
often... :-)

> Is it possible to update fastpath_cnt_hint properly perhaps?

I think that would be valid and even accurate as it can checks skb's
seq against fastpath_skb_hint->seq. I'm in a hurry and will be a week
out of reach of internet connectivity but here's quick attempt to deal
with this cleanly. Compile tested (be extra careful with this one, it's 
done i a hurry :-)), consider to net-2.6. Considering the marginality of 
this issue, stable might really be an overkill for this one, only a
remotely valid concern comes to my mind in this case: possibility
of fackets_out > packet_out might not be dealt that cleanly everywhere 
(but I'm sure that you can come to a good decicion about it):

[PATCH] [TCP]: SACK fastpath did override adjusted fackets_out

Do same adjustment to SACK fastpath counters provided that
they're valid.

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

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 53232dd..f24dd36 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -699,6 +699,15 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len, unsigned int mss
 			tp->fackets_out -= diff;
 			if ((int)tp->fackets_out < 0)
 				tp->fackets_out = 0;
+			/* SACK fastpath might overwrite it unless dealt with */
+			if (tp->fastpath_skb_hint != NULL &&
+			    after(TCP_SKB_CB(tp->fastpath_skb_hint)->seq,
+				  TCP_SKB_CB(skb)->seq)) {
+				tp->fastpath_cnt_hint -= diff;
+				if ((int)tp->fastpath_cnt_hint < 0)
+					tp->fastpath_cnt_hint = 0;
+				  
+			}
 		}
 	}
 
-- 
1.5.0.6

Re: Suspicious fackets_out handling

[David Miller]

From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Fri, 1 Jun 2007 15:17:00 +0300 (EEST)

> On Thu, 31 May 2007, David Miller wrote:
> 
> > Is it possible to update fastpath_cnt_hint properly perhaps?
> 
> I think that would be valid and even accurate as it can checks skb's
> seq against fastpath_skb_hint->seq. I'm in a hurry and will be a week
> out of reach of internet connectivity but here's quick attempt to deal
> with this cleanly. Compile tested (be extra careful with this one, it's 
> done i a hurry :-)), consider to net-2.6. Considering the marginality of 
> this issue, stable might really be an overkill for this one, only a
> remotely valid concern comes to my mind in this case: possibility
> of fackets_out > packet_out might not be dealt that cleanly everywhere 
> (but I'm sure that you can come to a good decicion about it):
> 
> [PATCH] [TCP]: SACK fastpath did override adjusted fackets_out
> 
> Do same adjustment to SACK fastpath counters provided that
> they're valid.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Since the case is marginal and the patch does need some testing
I'm putting this into net-2.6.23 for now.