Date: Sun, 3 Jun 2012 11:57:06 +0000 (UTC) From: Gleb Smirnoff <glebius@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r236512 - projects/pf/head/sys/contrib/pf/net Message-ID: <201206031157.q53Bv66A059650@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: glebius Date: Sun Jun 3 11:57:05 2012 New Revision: 236512 URL: http://svn.freebsd.org/changeset/base/236512 Log: In pf_map_addr(): - Fix "rpool->cur" update in such way, that pointer never gets a NULL value, otherwise concurrent threads may try to deref NULL. - Add a large comment explaining problems with round-robin, why most cases work, and why complicated would not. - Make some local variables more local to the case blocks, where they are used. Modified: projects/pf/head/sys/contrib/pf/net/pf_lb.c Modified: projects/pf/head/sys/contrib/pf/net/pf_lb.c ============================================================================== --- projects/pf/head/sys/contrib/pf/net/pf_lb.c Sun Jun 3 11:54:26 2012 (r236511) +++ projects/pf/head/sys/contrib/pf/net/pf_lb.c Sun Jun 3 11:57:05 2012 (r236512) @@ -350,11 +350,8 @@ int pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr, struct pf_addr *naddr, struct pf_addr *init_addr, struct pf_src_node **sn) { - unsigned char hash[16]; struct pf_pool *rpool = &r->rpool; - struct pf_addr *raddr = &rpool->cur->addr.v.a.addr; - struct pf_addr *rmask = &rpool->cur->addr.v.a.mask; - struct pf_pooladdr *acur = rpool->cur; + struct pf_addr *raddr = NULL, *rmask = NULL; if (*sn == NULL && r->rpool.opts & PF_POOL_STICKYADDR && (r->rpool.opts & PF_POOL_TYPEMASK) != PF_POOL_NONE) { @@ -452,10 +449,38 @@ pf_map_addr(sa_family_t af, struct pf_ru } break; case PF_POOL_SRCHASH: + { + unsigned char hash[16]; + pf_hash(saddr, (struct pf_addr *)&hash, &rpool->key, af); PF_POOLMASK(naddr, raddr, rmask, (struct pf_addr *)&hash, af); break; + } case PF_POOL_ROUNDROBIN: + { + struct pf_pooladdr *acur = rpool->cur; + + /* + * XXXGL: in the round-robin case we need to store + * the round-robin machine state in the rule, thus + * forwarding thread needs to modify rule. + * + * This is done w/o locking, because performance is assumed + * more importand than round-robin precision. + * + * In the simpliest case we just update the "rpool->cur" + * pointer. However, if pool contains tables or dynamic + * addresses, then "tblidx" is also used to store machine + * state. Since "tblidx" is int, concurrent access to it can't + * lead to inconsistence, only to lost of precision. + * + * Things get worse, if table contains not hosts, but + * prefixes. In this case counter also stores machine state, + * and for IPv6 address, counter can be updated atomically. + * Probably, using round-robin on a table containing IPv6 + * prefixes (or even IPv4) would cause a panic. + */ + if (rpool->cur->addr.type == PF_ADDR_TABLE) { if (!pfr_pool_get(rpool->cur->addr.p.tbl, &rpool->tblidx, &rpool->counter, af)) @@ -468,8 +493,10 @@ pf_map_addr(sa_family_t af, struct pf_ru goto get_addr; try_next: - if ((rpool->cur = TAILQ_NEXT(rpool->cur, entries)) == NULL) + if (TAILQ_NEXT(rpool->cur, entries) == NULL) rpool->cur = TAILQ_FIRST(&rpool->list); + else + rpool->cur = TAILQ_NEXT(rpool->cur, entries); if (rpool->cur->addr.type == PF_ADDR_TABLE) { rpool->tblidx = -1; if (pfr_pool_get(rpool->cur->addr.p.tbl, @@ -500,6 +527,7 @@ pf_map_addr(sa_family_t af, struct pf_ru PF_ACPY(init_addr, naddr, af); PF_AINC(&rpool->counter, af); break; + } } if (*sn != NULL) PF_ACPY(&(*sn)->raddr, naddr, af);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201206031157.q53Bv66A059650>