On Tue, Jun 21, 2016 at 11:24:14AM +0200, Mike Belopuhov wrote:
> So pf reused the port while some TCP segments were still in flight?
No. The old state was in FIN_WAIT_2 and the socket in TIME_WAIT.
They were idling for 25 seconds. Then a new state was created and
Nat pf_get_sport() did choose the port of the old state. The
collision was on the PF_SK_STACK side, but pf_find_state_all() is
called with PF_IN. So this is not recognized.
> But this is key_attach stage not port allocation... isn't that too
> late? When we fail the state key attachment we drop the connection.
I guess that the SYN-Retry fixes it. But only one of 1000 connections
fails so it is hard to debug. My use case is quite strange, I do
nat-to and divert-to in the same rule.
> I'm ok to add this safeguard, but can't we apply additional logic
> into the port allocation code to do a better job?
In my case I ended up with inconsistent state and socket as they
have different reuse policies. This might be the case in other
places too. Fixes in other places might avoid that situation. But
here something goes wrong, so I think here something should be
fixed.
> Does this port get allocated via pf_get_sport or is it a static port
> assignment that clashes with the port range NAT uses?
It is not a static port, pf_get_sport() chooses it.
Discussion with markus@ concluded that my diff is wrong for sloppy
states. There the sequence number is not updated in the old state.
So here is a new diff that fixes this.
ok?
bluhm
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.977
diff -u -p -r1.977 pf.c
--- net/pf.c 15 Jun 2016 11:49:34 -0000 1.977
+++ net/pf.c 21 Jun 2016 11:16:26 -0000
@@ -671,7 +671,9 @@ pf_state_key_attach(struct pf_state_key
si->s->direction != s->direction))) {
if (sk->proto == IPPROTO_TCP &&
si->s->src.state >= TCPS_FIN_WAIT_2 &&
- si->s->dst.state >= TCPS_FIN_WAIT_2) {
+ si->s->dst.state >= TCPS_FIN_WAIT_2 &&
+ ((si->s->state_flags & PFSTATE_SLOPPY) ||
+ SEQ_GT(s->src.seqlo, si->s->src.seqlo))) {
si->s->src.state = si->s->dst.state =
TCPS_CLOSED;
/* remove late or sks can go away */
