Dear Developers and Testers,
I have been running the system with slightly different version of this patch
for 28 days (uptime). I have
patched the "3.8 release" code.
SACK numbers were correctly changed (1, 2 or 3 SACKs occrured in a packet and
all were correctly
modified). No problems were seen.
Recently, I have applied the patch that you asked to test. Again, I have
patched the official 3.8 release
source code. System is up for over 5 days. No problems occur. SACKs are
correctly modulated. There is a
network of over 70 computers passing through patched OpenBSD. "modulate state"
rule applies to all of
them and they donlowad files etc. without problems. I have checked packets with
tcpdump on both sides
of patched OpenBSD FireWall and options are correctly changed.
Therefore I am convinced that the patch is OK. I have only a few suggestions,
but they do not affect the
behaviour of patch.
I have a request, (important for me).
I would be proud to see my name in the source code. I would be proud to see my
name in the source code. I
could show it not only to all my friends and relatives ;) but maybe to some
prospective employers as well.
Is there a chance that my name is mentioned in the comments at the beginning of
pf_modulate_sack
procedure? e.g.
+/*
+ * Need to modulate the sequence numbers in the TCP SACK option
+ * SACK modulation request reported and code partially programmed by Krzysztof
Pfaff
+ */
+int
+pf_modulate_sack(struct mbuf *m, int off, struct pf_pdesc *pd,
+ struct tcphdr *th, struct pf_state_peer *dst)
+{
+ int hlen = (th->th_off << 2) - sizeof(*th), thoptlen = hlen;
..
or something like that, maybe a different phrase
Points for me are that I have found (by observing and analysing various packets
passing through pf with
tcpdump) that pf with "modulate state" does not modulate SACK option. I have
reported that on tech list and I
have made a first version of patch. It was very first version, with debug
messages and it contained some bugs
(I am a beginner in Unix programming). But the patch worked.
Mike Frantzen has presented his version of patch - similiar, but better, more
optimised and with no bugs. But I
assisted him, to test it and helped as much as I could.
Points agains me are:
- I have made a few bugs in my source code ;( ,
- I shoud not dare to ask for such honourable mentioning of my name :)
Additionally, I have suggested the following things in previous mails. These
corrections are not important, but I consider them worth mentioning:
1. position of "pf_modulate_sack" declaration -
Since the definition is just before the pf_send_tcp, I suggest that the
declaration should be also placed just before pf_send_tcp.
It is not any improvement in code, but could keep the file in order.
2.
I have already proposed to change TCPOLEN_SACKLEN to TCPOLEN_SACKRAW
TCPOLEN_SACK means (I guess): TCP option length for SACK option
TCPOLEN_SACKRAW should mean length of RAW option - with
additional 2 bytes of option code & length.
But maybe I am getting too much into unimportant details...
If there is a decision about these changes, please let me know if you want me
to prepare the correct
patch.
Most important information is that THE PATCH WORKS!
Yours sincerelly,
Krzysztof Pfaff
On 28 Jan 2006 at 12:08, Daniel Hartmeier wrote:
Date sent: Sat, 28 Jan 2006 12:08:47 +0100
From: Daniel Hartmeier <[EMAIL PROTECTED]>
To: pf@benzedrine.cx
Copies to: [EMAIL PROTECTED], [EMAIL PROTECTED], [EMAIL PROTECTED]
Subject: Call for testing: TCP SACK and modulate/synproxy state
(patch)
> There is one rather important change we'd like to commit so it makes the
> 3.9 release (which is rapidly approaching tree lock), and to do so we
> need your help testing.
>
> TCP SACK is enabled by default on OpenBSD (and most other peers you'll
> connect to), yet pf's modulate state (and, therefore, synproxy state,
> which by definition includes sequence number modulation) does NOT
> translate sequence numbers found in SACK options.
>
> This means that, basically, modulate/synproxy state cannot be reliably
> used for SACK connections, as it causes connections to randomly stall
> after a while (annoying to debug, too, because the problem doesn't occur
> during or immediately after the connection establishment, but later).
>
> Let me put it this way: modulate state and synproxy state are BROKEN and
> INCOMPATIBLE with SACK right now. Since we (and most everyone else
> you'll be filtering for) is enabling SACK by default, we NEED the patch
> below. Otherwise we could just as well remove the modulate/synproxy
> features, as they break connections in most setups.
>
> So, please help test the patch from Mike below. I'm running with it on
> production machines, so I personally trust it to be fine. But testing by
> one person is not sufficient for inclusion in the source tree right now.
>
> Apply the patch, rebuild the kernel, reboot. Watch connections using
> SACK matching modulate state rules, especially long-lifed connections
> and connections transferring large amounts of data. Check output of
> netstat -sp tcp on the connection endpoints (the SACK related counters).
>
> The full history of the issue is detailed on
>
> http://marc.theaimsgroup.com/?t=113396771300009&r=1&w=2
>
> Daniel
>
>
