On Tue, Jun 21, 2016 at 02:45:42PM +0200, Mike Belopuhov wrote:
> You're testing the sequence number
> of the new state with an existing one which has seen some
> traffic.. Are you sure this is correct?
This is exactly what the stack does to distinguish between packets
that belong to an old connection and between a new one.
> But
> since they don't belong to the same connection, this is
> inherently incorrect.
Here is my logical error. The receiving TCP stack only thinks it
is a reuse of an existing connection. In this case, the sender is
reponsible to increase the initial sequence number. But the sender
has a different port or even a different IP. It is only NAT reusing
the port and address which confuses the stack.
So my diff fixes it because now pf state key attach and tcp reuse
are confused in the same way.
> Unless I'm wrong, I have to retract my OK and ask you to fix
> the sport bit instead.
Yes, fixing it in pf_get_sport() is more correct. I will try
to make a diff.
It would help a lot debugging such problems, if we would have more
log messages. So extend the existing log to the reuse case in
pf_state_key_attach() ?
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 13:50:01 -0000
@@ -669,34 +669,34 @@ pf_state_key_attach(struct pf_state_key
si->s->key[PF_SK_STACK]->af &&
sk->af == si->s->key[PF_SK_STACK]->af &&
si->s->direction != s->direction))) {
+ int reuse = 0;
+
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)
+ reuse = 1;
+ if (pf_status.debug >= LOG_NOTICE) {
+ log(LOG_NOTICE,
+ "pf: %s key attach %s on %s: ",
+ (idx == PF_SK_WIRE) ?
+ "wire" : "stack",
+ reuse ? "reuse" : "failed",
+ s->kif->pfik_name);
+ pf_print_state_parts(s,
+ (idx == PF_SK_WIRE) ? sk : NULL,
+ (idx == PF_SK_STACK) ? sk : NULL);
+ addlog(", existing: ");
+ pf_print_state_parts(si->s,
+ (idx == PF_SK_WIRE) ? sk : NULL,
+ (idx == PF_SK_STACK) ? sk : NULL);
+ addlog("\n");
+ }
+ if (reuse) {
si->s->src.state = si->s->dst.state =
TCPS_CLOSED;
/* remove late or sks can go away */
olds = si->s;
} else {
- if (pf_status.debug >= LOG_NOTICE) {
- log(LOG_NOTICE,
- "pf: %s key attach "
- "failed on %s: ",
- (idx == PF_SK_WIRE) ?
- "wire" : "stack",
- s->kif->pfik_name);
- pf_print_state_parts(s,
- (idx == PF_SK_WIRE) ?
- sk : NULL,
- (idx == PF_SK_STACK) ?
- sk : NULL);
- addlog(", existing: ");
- pf_print_state_parts(si->s,
- (idx == PF_SK_WIRE) ?
- sk : NULL,
- (idx == PF_SK_STACK) ?
- sk : NULL);
- addlog("\n");
- }
pool_put(&pf_state_key_pl, sk);
return (-1); /* collision! */
}
