Re: [PATCH] [RFC] [TCP]: Catch skb with S+L bugs earlier
From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Wed, 28 Mar 2007 17:02:47 +0300 (EEST) SACKED_ACKED and LOST are mutually exclusive, thus this condition is bug with SACK (IMHO). NewReno, however, could get enough duplicate ACKs which increment sacked_out, so it makes sense to do this kind of limitting for non-SACK TCP but not for SACK-enabled one. Perhaps the author had that in mind but did the logic accidently wrong way around? Eventually these bugs trigger traps in the tcp_clean_rtx_queue but it's much more informative to do this here (excludes some other possible bugs). Maybe this BUG_TRAP is too expensive to be included everywhere in the TCP code. Should there be some #if to surround it? Compile tested. Sadly enough I don't have time for couple of weeks to test this as it would require some setuping, and besides, my test machines are occupied currently to other work, but this might also be net-2.6 (or even stable) material if it really works (feel free to cut this paragraph or part of it if you decide to include this :-)). Signed-off-by: Ilpo Järvinen [EMAIL PROTECTED] I've applied this, thanks for your patience. I will see if it makes my workstation explode :-) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] [TCP]: Catch skb with S+L bugs earlier
On Mon, 30 Apr 2007, David Miller wrote: From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Wed, 28 Mar 2007 17:02:47 +0300 (EEST) SACKED_ACKED and LOST are mutually exclusive, thus this condition is bug with SACK (IMHO). NewReno, however, could get enough duplicate ACKs which increment sacked_out, so it makes sense to do this kind of limitting for non-SACK TCP but not for SACK-enabled one. Perhaps the author had that in mind but did the logic accidently wrong way around? Eventually these bugs trigger traps in the tcp_clean_rtx_queue but it's much more informative to do this here (excludes some other possible bugs). Maybe this BUG_TRAP is too expensive to be included everywhere in the TCP code. Should there be some #if to surround it? Compile tested. Sadly enough I don't have time for couple of weeks to test this as it would require some setuping, and besides, my test machines are occupied currently to other work, but this might also be net-2.6 (or even stable) material if it really works (feel free to cut this paragraph or part of it if you decide to include this :-)). Signed-off-by: Ilpo Järvinen [EMAIL PROTECTED] I've applied this, thanks for your patience. I will see if it makes my workstation explode :-) I think I sent an updated version later (hopefully I reach you before you push these out :-)), which made the BUG_ON unconditional (I used it instead of BUG_TRAP as it seems to be generic machinery for handling these). Mine didn't explode (neither with SACK nor without it)... :-) With SACK it's very clearly a BUG that must never happen (non-SACK seems safe but I haven't tested e.g. all those fragmentation/collapse paths). -- i.
Re: [PATCH] [RFC] [TCP]: Catch skb with S+L bugs earlier
On Mon, 30 Apr 2007, Ilpo Järvinen wrote: On Mon, 30 Apr 2007, David Miller wrote: From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Wed, 28 Mar 2007 17:02:47 +0300 (EEST) SACKED_ACKED and LOST are mutually exclusive, thus this condition is bug with SACK (IMHO). NewReno, however, could get [...snip...] I've applied this, thanks for your patience. ...heh... I was getting a bit unsure whether you still had them... I think I sent an updated version later (hopefully I reach you before you push these out :-)), which made the BUG_ON unconditional (I used it instead of BUG_TRAP as it seems to be generic machinery for handling these). Here: http://marc.info/?l=linux-netdevm=117648826715609w=2 -- i.
Re: [PATCH] [RFC] [TCP]: Catch skb with S+L bugs earlier
From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Mon, 30 Apr 2007 10:49:21 +0300 (EEST) On Mon, 30 Apr 2007, Ilpo Järvinen wrote: On Mon, 30 Apr 2007, David Miller wrote: From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Wed, 28 Mar 2007 17:02:47 +0300 (EEST) SACKED_ACKED and LOST are mutually exclusive, thus this condition is bug with SACK (IMHO). NewReno, however, could get [...snip...] I've applied this, thanks for your patience. ...heh... I was getting a bit unsure whether you still had them... I think I sent an updated version later (hopefully I reach you before you push these out :-)), which made the BUG_ON unconditional (I used it instead of BUG_TRAP as it seems to be generic machinery for handling these). Here: http://marc.info/?l=linux-netdevm=117648826715609w=2 Thanks, I'll swap in that version of the patch. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] [TCP]: Catch skb with S+L bugs earlier
On Mon, 30 Apr 2007, David Miller wrote: From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Mon, 30 Apr 2007 10:49:21 +0300 (EEST) On Mon, 30 Apr 2007, Ilpo Järvinen wrote: On Mon, 30 Apr 2007, David Miller wrote: From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Wed, 28 Mar 2007 17:02:47 +0300 (EEST) SACKED_ACKED and LOST are mutually exclusive, thus this condition is bug with SACK (IMHO). NewReno, however, could get [...snip...] I've applied this, thanks for your patience. ...heh... I was getting a bit unsure whether you still had them... I think I sent an updated version later (hopefully I reach you before you push these out :-)), which made the BUG_ON unconditional (I used it instead of BUG_TRAP as it seems to be generic machinery for handling these). Here: http://marc.info/?l=linux-netdevm=117648826715609w=2 Thanks, I'll swap in that version of the patch. Hmm, just looked a bit more, is the left_out really that necessary as a separate variable? It's always possible to calculate it in the places where it is needed and users are quite few after all (basically the packets_in_flight callers)... ...it just would save some bytes in tcp_sock... ;-) -- i.
[PATCH] [RFC] [TCP]: Catch skb with S+L bugs earlier
SACKED_ACKED and LOST are mutually exclusive, thus this
condition is bug with SACK (IMHO). NewReno, however, could get
enough duplicate ACKs which increment sacked_out, so it makes
sense to do this kind of limitting for non-SACK TCP but not for
SACK-enabled one. Perhaps the author had that in mind but did
the logic accidently wrong way around?
Eventually these bugs trigger traps in the tcp_clean_rtx_queue
but it's much more informative to do this here (excludes some
other possible bugs).
Maybe this BUG_TRAP is too expensive to be included everywhere
in the TCP code. Should there be some #if to surround it?
Compile tested. Sadly enough I don't have time for couple of
weeks to test this as it would require some setuping, and besides,
my test machines are occupied currently to other work, but this
might also be net-2.6 (or even stable) material if it really
works (feel free to cut this paragraph or part of it if you
decide to include this :-)).
Signed-off-by: Ilpo Järvinen [EMAIL PROTECTED]
---
include/net/tcp.h |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index fe1c4f0..3c8dd13 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -733,9 +733,10 @@ static inline __u32 tcp_current_ssthresh
static inline void tcp_sync_left_out(struct tcp_sock *tp)
{
- if (tp-rx_opt.sack_ok
- (tp-sacked_out = tp-packets_out - tp-lost_out))
+ if (tp-sacked_out + tp-lost_out tp-packets_out) {
+ BUG_TRAP(!tp-rx_opt.sack_ok);
tp-sacked_out = tp-packets_out - tp-lost_out;
+ }
tp-left_out = tp-sacked_out + tp-lost_out;
}
--
1.4.2
