Hello,
On Wed, Feb 08, 2023 at 09:42:11PM -0600, joshua stein wrote:
</snip>
> $ for i in `seq 5` ; do nc 192.168.1.240 22 & done
> [2] 68892
> [3] 6303
> [4] 63554
> [5] 87833
> [6] 49997
> $ SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
>
> vm:~$ doas pfctl -sr
> block return all
> pass out all flags S/SA
> pass in on egress inet6 proto tcp from any to ::1 port = 22 flags S/SA keep
> state (source-track rule, max-src-conn 3)
> pass in on egress inet proto tcp from any to 127.0.0.1 port = 22 flags S/SA
> keep state (source-track rule, max-src-conn 3)
> pass in on egress inet proto tcp from any to 192.168.1.240 port = 22 flags
> S/SA keep state (source-track rule, max-src-conn 3)
>
> This is with:
>
> OpenBSD 7.2-current (GENERIC.MP) #2014: Tue Feb 7 16:24:04 MST 2023
> dera...@arm64.openbsd.org:/usr/src/sys/arch/arm64/compile/GENERIC.MP
I gave it a try after doing a sysupgrade to:
penBSD 7.2-current (GENERIC.MP) #1025: Wed Feb 8 19:16:09 MST 2023
it still works for me as expected:
disk$ for i in `seq 5` ; do nc 192.168.2.175 22 & done
[1] 51566
[2] 78983
[3] 77864
[4] 37474
[5] 98599
disk$ SSH-2.0-OpenSSH_9.2
SSH-2.0-OpenSSH_9.2
SSH-2.0-OpenSSH_9.2
my connection arrives over iwn0 interface which is in egress group
so our environments are almost identical.
</snip>
>
> > > diff --git sys/net/pf.c sys/net/pf.c
> > > index 8cb1326a160..89703feab12 100644
> > > --- sys/net/pf.c
> > > +++ sys/net/pf.c
> > > @@ -481,12 +481,10 @@ pf_src_connlimit(struct pf_state **stp)
> > > if ((sn = pf_get_src_node((*stp), PF_SN_NONE)) == NULL)
> > > return (0);
> > >
> > > - sn->conn++;
> > > - (*stp)->src.tcp_est = 1;
> > > pf_add_threshold(&sn->conn_rate);
> > >
> > > if ((*stp)->rule.ptr->max_src_conn &&
> > > - (*stp)->rule.ptr->max_src_conn < sn->conn) {
> > > + sn->conn >= (*stp)->rule.ptr->max_src_conn) {
> > > pf_status.lcounters[LCNT_SRCCONN]++;
> > > bad++;
> > > }
> > > @@ -497,8 +495,11 @@ pf_src_connlimit(struct pf_state **stp)
> > > bad++;
> > > }
> > >
> > > - if (!bad)
> > > + if (!bad) {
> > > + sn->conn++;
> > > + (*stp)->src.tcp_est = 1;
> > > return (0);
> > > + }
> > >
> > > if ((*stp)->rule.ptr->overload_tbl) {
> > > struct pfr_addr p;
> >
> > it seems to me the change to pf_src_connlimit() does
> > not alter behavior. I think change to pf_src_connlimit()
> > can be dropped.
>
> But don't we not want to increment the source node's connection
> count since we're not going to accept the connection (in the !bad
> case)? I'm not sure what kind of bookkeeping that would screw up.
>
what we currently do is we always bump connection count
for source node we found. then we are going to check limit
(*stp)->rule.ptr->max_src_conn < sn->conn
if the limit is exceeded we mark as closed and expired (timeout
PFTM_PURGE). We also report that to caller which should close the
connection.
your change stops counting connections as soon as limit is
reached. So now I see there is a change in behavior. I've missed
that yesterday. I'm not able to tell if we want go that way or not.
</snip>
> > currently we do conn limit check in step (4). Your change moves this
> > to earlier step (3) (given I understand things right here).
> > It's awfully early here I need sleep on this.
>
> Yes, that was my understanding too. We wait until the remote has
> done enough work to be a valid connection but then block it before
> sending the final ack.
>
> > can you give it a try with your slightly modified diff? just drop
> > changes to pf_src_connlimit() and keep those in pf_tcp_track_full() which
> > I believe is the only relevant part.
>
> Yes, it still works, and only allows me 3 connections with the final
> 2 timing out as expected:
>
> $ for i in `seq 5` ; do nc 192.168.1.240 22 & done
> [2] 10193
> [3] 30197
> [4] 72235
> [5] 69900
> [6] 99044
> $ SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> SSH-2.0-OpenSSH_9.1
> [5] - exit 1 nc 192.168.1.240 22
> $
> [6] + exit 1 nc 192.168.1.240 22
>
the only explanation why it does not work for you is latency. The packets which
match state run as a readers. so if we call pf_set_protostate() before we
actually check the limit then it might be the cause here. I admit it's
rather a speculation.
can you give a try to diff below?
thanks and
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 8cb1326a160..f81b0c793ce 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -4919,14 +4919,14 @@ pf_tcp_track_full(struct pf_pdesc *pd, struct pf_state
**stp, u_short *reason,
pf_set_protostate(*stp, psrc, TCPS_CLOSING);
if (th->th_flags & TH_ACK) {
if (dst->state == TCPS_SYN_SENT) {
- pf_set_protostate(*stp, pdst,
- TCPS_ESTABLISHED);
if (src->state == TCPS_ESTABLISHED &&
!SLIST_EMPTY(&(*stp)->src_nodes) &&
pf_src_connlimit(stp)) {
REASON_SET(reason, PFRES_SRCLIMIT);
return (PF_DROP);
}
+ pf_set_protostate(*stp, pdst,
+ TCPS_ESTABLISHED);
} else if (dst->state == TCPS_CLOSING)
pf_set_protostate(*stp, pdst,
TCPS_FIN_WAIT_2);
