kern/184003: On state creation src_node is looked up twice.
Kajetan Staszkiewicz
vegeta at tuxpowered.net
Fri Nov 15 15:50:00 UTC 2013
>Number: 184003
>Category: kern
>Synopsis: On state creation src_node is looked up twice.
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Fri Nov 15 15:50:00 UTC 2013
>Closed-Date:
>Last-Modified:
>Originator: Kajetan Staszkiewicz
>Release: 9.1, 10.0-BETA
>Organization:
InnoGames GmbH
>Environment:
FreeBSD xxxxxxx 10.0-BETA3 FreeBSD 10.0-BETA3 #3 75c5288(lookup-src_node-only-once)-dirty: Fri Nov 15 15:31:21 CET 2013 root at lbdevel100:/usr/obj/mnt/builder/freebsd.git/sys/IGLB amd64
>Description:
When a new state is created, pf_insert_src_node is called which tries to find an existing src_node or creates a new one if none matching is found. Later, when pf_set_rt_ifp (and pf_map_addr) is called, a search for src_node is performed again, even though matching (found or new) src_node is already known.
>How-To-Repeat:
Have your FreeBSD-based loadbalancer under a SYN DDoS attack, observe 2x more src_node lookups than incoming SYN packets.
>Fix:
Do not call pf_find_src_node in pf_map_addr if source_node is given.
The attached patch is for FreeBSD 10.0-BETA3 and was not yet tested under bigger load, although the same approach works well for FreeBSD 9.1. I can provide the 9.1 patch too if requested.
Patch attached with submission follows:
# When a new state is created, pf_insert_src_node is called which tries to
# find an existing src_node or creates a new one if none matching is found.
# Later, when pf_set_rt_ifp (and pf_map_addr) is called, a search for src_node
# is performed again, even though matching (found or new) src_node is already
# known.
#
# Do not call pf_find_src_node in pf_map_addr if source_node is given.
#
# kajetan.staszkiewicz at innogames.de
# Work sponsored by InnoGames GmbH
#
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 59a349d..c58438a 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -268,7 +268,8 @@ static u_int16_t pf_get_mss(struct mbuf *, int, u_int16_t,
static u_int16_t pf_calc_mss(struct pf_addr *, sa_family_t,
int, u_int16_t);
static int pf_set_rt_ifp(struct pf_state *,
- struct pf_addr *, sa_family_t af);
+ struct pf_addr *, sa_family_t af,
+ struct pf_src_node **sn);
static int pf_check_proto_cksum(struct mbuf *, int, int,
u_int8_t, sa_family_t);
static void pf_print_state_parts(struct pf_state *,
@@ -2930,10 +2931,10 @@ pf_calc_mss(struct pf_addr *addr, sa_family_t af, int rtableid, u_int16_t offer)
}
static int
-pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af)
+pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af,
+ struct pf_src_node **sn)
{
struct pf_rule *r = s->rule.ptr;
- struct pf_src_node *sn = NULL;
int map_status = 0;
s->rt_kif = NULL;
@@ -2942,13 +2943,13 @@ pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af)
switch (af) {
#ifdef INET
case AF_INET:
- map_status = pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, &sn);
+ map_status = pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, sn);
s->rt_kif = r->rpool.cur->kif;
break;
#endif /* INET */
#ifdef INET6
case AF_INET6:
- map_status = pf_map_addr(AF_INET6, r, saddr, &s->rt_addr, NULL, &sn);
+ map_status = pf_map_addr(AF_INET6, r, saddr, &s->rt_addr, NULL, sn);
s->rt_kif = r->rpool.cur->kif;
break;
#endif /* INET6 */
@@ -3523,7 +3524,7 @@ pf_create_state(struct pf_rule *r, struct pf_rule *nr, struct pf_rule *a,
remove the state and drop the packet. It makes no sense forwarding
it if redirection mapping has faied. Do it before setting timeouts,
csfailed fails otherwise. */
- if (pf_set_rt_ifp(s, pd->src, pd->af)) {
+ if (pf_set_rt_ifp(s, pd->src, pd->af, &sn)) {
REASON_SET(&reason, PFRES_MAPFAILED);
pf_src_tree_remove_state(s);
STATE_DEC_COUNTERS(s);
diff --git a/sys/netpfil/pf/pf_lb.c b/sys/netpfil/pf/pf_lb.c
index f870bf4..137f1de 100644
--- a/sys/netpfil/pf/pf_lb.c
+++ b/sys/netpfil/pf/pf_lb.c
@@ -308,22 +308,31 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
struct pf_pool *rpool = &r->rpool;
struct pf_addr *raddr = NULL, *rmask = NULL;
+ /* Try to find a src_node if none was given and this
+ is a sticky-address rule. */
if (*sn == NULL && r->rpool.opts & PF_POOL_STICKYADDR &&
(r->rpool.opts & PF_POOL_TYPEMASK) != PF_POOL_NONE) {
*sn = pf_find_src_node(saddr, r, af, 0);
- if (*sn != NULL && !PF_AZERO(&(*sn)->raddr, af)) {
- PF_ACPY(naddr, &(*sn)->raddr, af);
- if (V_pf_status.debug >= PF_DEBUG_MISC) {
- printf("pf_map_addr: src tracking maps ");
- pf_print_host(saddr, 0, af);
- printf(" to ");
- pf_print_host(naddr, 0, af);
- printf("\n");
- }
- return (0);
+ }
+
+ /* If a src_node was found or explicitly given and it has a non-zero
+ route address, use this address. A zeroed address is found if the
+ src node was created just a moment ago in pf_create_state and it
+ needs to be filled in with routing decission calculated here. */
+ if (*sn != NULL && !PF_AZERO(&(*sn)->raddr, af)) {
+ PF_ACPY(naddr, &(*sn)->raddr, af);
+ if (V_pf_status.debug >= PF_DEBUG_MISC) {
+ printf("pf_map_addr: src tracking maps ");
+ pf_print_host(saddr, 0, af);
+ printf(" to ");
+ pf_print_host(naddr, 0, af);
+ printf("\n");
}
+ return (0);
}
+ /* Find the route using chosen algorithm. Store the found route
+ in src_node if it was given or found. */
if (rpool->cur->addr.type == PF_ADDR_NOROUTE)
return (1);
if (rpool->cur->addr.type == PF_ADDR_DYNIFTL) {
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-bugs
mailing list