From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David S. Miller" Subject: Re: TCP window tracking has bad side effects Date: Fri, 10 Dec 2004 12:32:52 -0800 Message-ID: <20041210123252.6b2b82ed.davem@davemloft.net> References: <20041201110253.GA9536@suse.de> <20041201153903.GA16807@suse.de> <20041209162656.GA9569@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netfilter-devel@lists.netfilter.org, ludwig.nussel@suse.de Return-path: To: Jozsef Kadlecsik In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: netfilter-devel-bounces@lists.netfilter.org Errors-To: netfilter-devel-bounces@lists.netfilter.org List-Id: netfilter-devel.vger.kernel.org On Fri, 10 Dec 2004 11:03:29 +0100 (CET) Jozsef Kadlecsik wrote: > Hi Ludwing, > > On Thu, 9 Dec 2004, Ludwig Nussel wrote: > > > Seems to work. The client machine no longer drops the packet and > > properly sends RST. > > Thank you for the bug and test reports. Attached is the patch for kernel > inclusion by Dave: Patrick rewived the previous one and noted that a > condition was errorneously dropped - it's back now and a typo in logging > is fixed as well. If you run the previous code on a production machine, > please replace it with the new one. Jozsef, btw, can I ask you to stop using whatever patch generating tool you use that removes trailing spaces? It makes it so that I have to apply your patches by hand. For example: > @@ -552,8 +552,8 @@ > * Both sides must send the Window Scale option > * to enable window scaling in either direction. > */ > - if (!(sender->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE > - && receiver->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE)) > + if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE > + && receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE)) > sender->td_scale = > receiver->td_scale = 0; > } else { In the sources, there is a space at the end of the line with the "sender->td_scale =" assignment statement. Yet in your patch, it is not there. Same thing here: > @@ -876,7 +880,7 @@ > WRITE_UNLOCK(&tcp_lock); > if (LOG_INVALID(IPPROTO_TCP)) > nf_log_packet(PF_INET, 0, skb, NULL, NULL, > - "ip_ct_tcp: invalid SYN (ignored) "); > + "ip_ct_tcp: invalid packet ignored "); > return NF_ACCEPT; > case TCP_CONNTRACK_MAX: > /* Invalid packet */ The "nf_log_packet(..." line should have a trailing space, yet you are patching against a line that does not have it. And finally it occurs a third time in this hunk. > @@ -901,11 +905,12 @@ > break; > case TCP_CONNTRACK_CLOSE: > if (index == TCP_RST_SET > - && test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status) > - && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET > + && ((test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status) > + && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET) > + || conntrack->proto.tcp.last_index == TCP_ACK_SET) > && after(ntohl(th->ack_seq), > conntrack->proto.tcp.last_seq)) { > - /* Ignore RST closing down invalid SYN > + /* Ignore RST closing down invalid SYN or ACK > we had let trough. */ > WRITE_UNLOCK(&tcp_lock); > if (LOG_INVALID(IPPROTO_TCP)) The line with the "/* Ignore ..." comment should have a trailing space, yet it does not in what your patch is against. I've read recently that one of the commonly used patch generating tools removes the training spaces. But whatever it is, please stop doing this :-)