Date: Sat, 14 Apr 2012 11:44:11 +0000 (UTC) From: Gleb Smirnoff <glebius@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r234282 - projects/pf/head/sys/contrib/pf/net Message-ID: <201204141144.q3EBiBeL028673@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: glebius Date: Sat Apr 14 11:44:10 2012 New Revision: 234282 URL: http://svn.freebsd.org/changeset/base/234282 Log: Remove the "pfugidhack". The core problem here is that we need to do in_pcblookup() from the pf(4). This leads to LOR between "pf Giant" and pcb hash locks. The lookup can be done in several cases: 1. When processing rules that specify uid/gid. 2. When logging on a rule that has "log (user)" option. 2.1 ..., when processing rulesets. 2.2 ..., at the end of pf_test(), if memory allocation failed or if packet has IP Options. In the new locking scheme, in 1 and 2.1 we would only have reader lock on rulesets. In 2.2 we might have a state lock. Since lock on rulesets is _reader_, the LOR between it and PCB locks is safe. By the way, we already have LOR between pfil(9) reader lock and PCBs. Thus, in the new locking scheme lookup in 1 and 2.1 is safe and doesn't require any hacks. In the 2.2 we avoid lookup, if we got a state. This is really a rare case, and tiny degradation of pflog(4) output can be sustained. For this pflog_packet() gets an additional argument. While we still have the "pf Giant" we unlock it in pf_socket_lookup() temporarily. While here: - Reduce argument list for pf_test_rule(). - Reduce argument list for pf_socket_lookup(), simplifing code. - Remove pid_t from pdesc. Our socket layer doesn't provide this information. Modified: projects/pf/head/sys/contrib/pf/net/if_pflog.c projects/pf/head/sys/contrib/pf/net/if_pflog.h projects/pf/head/sys/contrib/pf/net/pf.c projects/pf/head/sys/contrib/pf/net/pf_ioctl.c projects/pf/head/sys/contrib/pf/net/pf_norm.c projects/pf/head/sys/contrib/pf/net/pfvar.h Modified: projects/pf/head/sys/contrib/pf/net/if_pflog.c ============================================================================== --- projects/pf/head/sys/contrib/pf/net/if_pflog.c Sat Apr 14 11:29:32 2012 (r234281) +++ projects/pf/head/sys/contrib/pf/net/if_pflog.c Sat Apr 14 11:44:10 2012 (r234282) @@ -223,7 +223,7 @@ pflogioctl(struct ifnet *ifp, u_long cmd static int pflog_packet(struct pfi_kif *kif, struct mbuf *m, sa_family_t af, u_int8_t dir, u_int8_t reason, struct pf_rule *rm, struct pf_rule *am, - struct pf_ruleset *ruleset, struct pf_pdesc *pd) + struct pf_ruleset *ruleset, struct pf_pdesc *pd, int lookupsafe) { struct ifnet *ifn; struct pfloghdr hdr; @@ -251,19 +251,18 @@ pflog_packet(struct pfi_kif *kif, struct strlcpy(hdr.ruleset, ruleset->anchor->name, sizeof(hdr.ruleset)); } - if (rm->log & PF_LOG_SOCKET_LOOKUP && !pd->lookup.done) - /* - * XXX: This should not happen as we force an early lookup - * via debug.pfugidhack - */ - ; /* empty */ - if (pd->lookup.done > 0) { + /* + * XXXGL: we avoid pf_socket_lookup() when we are holding + * state lock, since this leads to unsafe LOR. + * These conditions are very very rare, however. + */ + if (rm->log & PF_LOG_SOCKET_LOOKUP && !pd->lookup.done && lookupsafe) + pd->lookup.done = pf_socket_lookup(dir, pd); + if (pd->lookup.done > 0) hdr.uid = pd->lookup.uid; - hdr.pid = pd->lookup.pid; - } else { + else hdr.uid = UID_MAX; - hdr.pid = NO_PID; - } + hdr.pid = NO_PID; hdr.rule_uid = rm->cuid; hdr.rule_pid = rm->cpid; hdr.dir = dir; Modified: projects/pf/head/sys/contrib/pf/net/if_pflog.h ============================================================================== --- projects/pf/head/sys/contrib/pf/net/if_pflog.h Sat Apr 14 11:29:32 2012 (r234281) +++ projects/pf/head/sys/contrib/pf/net/if_pflog.h Sat Apr 14 11:44:10 2012 (r234282) @@ -75,9 +75,9 @@ struct pf_ruleset; struct pfi_kif; struct pf_pdesc; -#define PFLOG_PACKET(i,x,a,b,c,d,e,f,g,h) do { \ +#define PFLOG_PACKET(i,x,a,b,c,d,e,f,g,h,di) do { \ if (pflog_packet_ptr != NULL) \ - pflog_packet_ptr(i,a,b,c,d,e,f,g,h); \ + pflog_packet_ptr(i,a,b,c,d,e,f,g,h,di); \ } while (0) #endif /* _KERNEL */ #endif /* _NET_IF_PFLOG_H_ */ Modified: projects/pf/head/sys/contrib/pf/net/pf.c ============================================================================== --- projects/pf/head/sys/contrib/pf/net/pf.c Sat Apr 14 11:29:32 2012 (r234281) +++ projects/pf/head/sys/contrib/pf/net/pf.c Sat Apr 14 11:44:10 2012 (r234282) @@ -229,9 +229,8 @@ static int pf_state_key_ini(void *, in static u_int32_t pf_tcp_iss(struct pf_pdesc *); static int pf_test_rule(struct pf_rule **, struct pf_state **, int, struct pfi_kif *, struct mbuf *, int, - void *, struct pf_pdesc *, struct pf_rule **, - struct pf_ruleset **, struct ifqueue *, - struct inpcb *); + struct pf_pdesc *, struct pf_rule **, + struct pf_ruleset **, struct inpcb *); static int pf_create_state(struct pf_rule *, struct pf_rule *, struct pf_rule *, struct pf_pdesc *, struct pf_src_node *, struct pf_state_key *, @@ -267,8 +266,6 @@ static void pf_route(struct mbuf **, s static void pf_route6(struct mbuf **, struct pf_rule *, int, struct ifnet *, struct pf_state *, struct pf_pdesc *); -static int pf_socket_lookup(int, struct pf_pdesc *, - struct inpcb *); static u_int8_t pf_get_wscale(struct mbuf *, int, u_int16_t, sa_family_t); static u_int16_t pf_get_mss(struct mbuf *, int, u_int16_t, @@ -292,7 +289,6 @@ static struct pf_state *pf_find_state(st static int pf_src_connlimit(struct pf_state **); static int pf_insert_src_node(struct pf_src_node **, struct pf_rule *, struct pf_addr *, sa_family_t); -static int pf_check_congestion(struct ifqueue *); static void pf_purge_expired_states(int); int in4_cksum(struct mbuf *m, u_int8_t nxt, int off, int len); @@ -2568,26 +2564,16 @@ pf_addr_inc(struct pf_addr *addr, sa_fam } #endif /* INET6 */ -static int -pf_socket_lookup(int direction, struct pf_pdesc *pd, struct inpcb *inp_arg) +int +pf_socket_lookup(int direction, struct pf_pdesc *pd) { struct pf_addr *saddr, *daddr; u_int16_t sport, dport; struct inpcbinfo *pi; struct inpcb *inp; - if (pd == NULL) - return (-1); pd->lookup.uid = UID_MAX; pd->lookup.gid = GID_MAX; - pd->lookup.pid = NO_PID; - - if (inp_arg != NULL) { - INP_LOCK_ASSERT(inp_arg); - pd->lookup.uid = inp_arg->inp_cred->cr_uid; - pd->lookup.gid = inp_arg->inp_cred->cr_groups[0]; - return (1); - } switch (pd->proto) { case IPPROTO_TCP: @@ -2619,6 +2605,7 @@ pf_socket_lookup(int direction, struct p saddr = pd->dst; daddr = pd->src; } + PF_UNLOCK(); switch (pd->af) { #ifdef INET case AF_INET: @@ -2662,6 +2649,7 @@ pf_socket_lookup(int direction, struct p pd->lookup.uid = inp->inp_cred->cr_uid; pd->lookup.gid = inp->inp_cred->cr_groups[0]; INP_RUNLOCK(inp); + PF_LOCK(); return (1); } @@ -2855,9 +2843,8 @@ pf_tcp_iss(struct pf_pdesc *pd) static int pf_test_rule(struct pf_rule **rm, struct pf_state **sm, int direction, - struct pfi_kif *kif, struct mbuf *m, int off, void *h, - struct pf_pdesc *pd, struct pf_rule **am, struct pf_ruleset **rsm, - struct ifqueue *ifq, struct inpcb *inp) + struct pfi_kif *kif, struct mbuf *m, int off, struct pf_pdesc *pd, + struct pf_rule **am, struct pf_ruleset **rsm, struct inpcb *inp) { struct pf_rule *nr = NULL; struct pf_addr * const saddr = pd->src; @@ -2878,19 +2865,13 @@ pf_test_rule(struct pf_rule **rm, struct u_int16_t bproto_sum = 0, bip_sum = 0; u_int8_t icmptype = 0, icmpcode = 0; + PF_RULES_RASSERT(); - if (direction == PF_IN && pf_check_congestion(ifq)) { - REASON_SET(&reason, PFRES_CONGEST); - return (PF_DROP); - } - - if (inp != NULL) - pd->lookup.done = pf_socket_lookup(direction, pd, inp); - else if (V_debug_pfugidhack) { - PF_UNLOCK(); - DPFPRINTF(PF_DEBUG_MISC, ("pf: unlocked lookup\n")); - pd->lookup.done = pf_socket_lookup(direction, pd, inp); - PF_LOCK(); + if (inp != NULL) { + INP_LOCK_ASSERT(inp); + pd->lookup.uid = inp->inp_cred->cr_uid; + pd->lookup.gid = inp->inp_cred->cr_groups[0]; + pd->lookup.done = 1; } switch (pd->proto) { @@ -3111,13 +3092,13 @@ pf_test_rule(struct pf_rule **rm, struct r = TAILQ_NEXT(r, entries); /* tcp/udp only. uid.op always 0 in other cases */ else if (r->uid.op && (pd->lookup.done || (pd->lookup.done = - pf_socket_lookup(direction, pd, inp), 1)) && + pf_socket_lookup(direction, pd), 1)) && !pf_match_uid(r->uid.op, r->uid.uid[0], r->uid.uid[1], pd->lookup.uid)) r = TAILQ_NEXT(r, entries); /* tcp/udp only. gid.op always 0 in other cases */ else if (r->gid.op && (pd->lookup.done || (pd->lookup.done = - pf_socket_lookup(direction, pd, inp), 1)) && + pf_socket_lookup(direction, pd), 1)) && !pf_match_gid(r->gid.op, r->gid.gid[0], r->gid.gid[1], pd->lookup.gid)) r = TAILQ_NEXT(r, entries); @@ -3162,7 +3143,7 @@ pf_test_rule(struct pf_rule **rm, struct if (rewrite) m_copyback(m, off, hdrlen, pd->hdr.any); PFLOG_PACKET(kif, h, m, af, direction, reason, r->log ? r : nr, - a, ruleset, pd); + a, ruleset, pd, 1); } if ((r->action == PF_DROP) && @@ -3588,7 +3569,7 @@ pf_test_fragment(struct pf_rule **rm, in if (r->log) PFLOG_PACKET(kif, h, m, af, direction, reason, r, a, ruleset, - pd); + pd, 1); if (r->action != PF_PASS) return (PF_DROP); @@ -5710,8 +5691,8 @@ pf_test(int dir, struct ifnet *ifp, stru a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, - m, off, h, &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } @@ -5739,8 +5720,8 @@ pf_test(int dir, struct ifnet *ifp, stru a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, - m, off, h, &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } @@ -5762,8 +5743,8 @@ pf_test(int dir, struct ifnet *ifp, stru a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, - m, off, h, &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } @@ -5785,12 +5766,13 @@ pf_test(int dir, struct ifnet *ifp, stru a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, m, off, h, - &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } done: + PF_RULES_RUNLOCK(); if (action == PF_PASS && h->ip_hl > 5 && !((s && s->state_flags & PFSTATE_ALLOWOPTS) || r->allow_opts)) { action = PF_DROP; @@ -5844,7 +5826,6 @@ done: if (s) PF_STATE_UNLOCK(s); - PF_RULES_RUNLOCK(); PF_UNLOCK(); m_tag_prepend(m, ipfwtag); @@ -5875,7 +5856,7 @@ done: else lr = r; PFLOG_PACKET(kif, h, m, AF_INET, dir, reason, lr, a, ruleset, - &pd); + &pd, (s == NULL)); } kif->pfik_bytes[0][dir == PF_OUT][action != PF_PASS] += pd.tot_len; @@ -5941,7 +5922,6 @@ done: } if (s) PF_STATE_UNLOCK(s); - PF_RULES_RUNLOCK(); PF_UNLOCK(); return (action); @@ -6125,8 +6105,8 @@ pf_test6(int dir, struct ifnet *ifp, str a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, - m, off, h, &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } @@ -6154,8 +6134,8 @@ pf_test6(int dir, struct ifnet *ifp, str a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, - m, off, h, &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } @@ -6184,8 +6164,8 @@ pf_test6(int dir, struct ifnet *ifp, str a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, - m, off, h, &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } @@ -6198,12 +6178,13 @@ pf_test6(int dir, struct ifnet *ifp, str a = s->anchor.ptr; log = s->log; } else if (s == NULL) - action = pf_test_rule(&r, &s, dir, kif, m, off, h, - &pd, &a, &ruleset, NULL, inp); + action = pf_test_rule(&r, &s, dir, kif, m, off, &pd, + &a, &ruleset, inp); break; } done: + PF_RULES_RUNLOCK(); if (n != m) { m_freem(n); n = NULL; @@ -6258,7 +6239,7 @@ done: else lr = r; PFLOG_PACKET(kif, h, m, AF_INET6, dir, reason, lr, a, ruleset, - &pd); + &pd, (s == NULL)); } kif->pfik_bytes[1][dir == PF_OUT][action != PF_PASS] += pd.tot_len; @@ -6323,15 +6304,7 @@ done: if (s) PF_STATE_UNLOCK(s); - PF_RULES_RUNLOCK(); PF_UNLOCK(); return (action); } #endif /* INET6 */ - -static int -pf_check_congestion(struct ifqueue *ifq) -{ - /* XXX_IMPORT: later */ - return (0); -} Modified: projects/pf/head/sys/contrib/pf/net/pf_ioctl.c ============================================================================== --- projects/pf/head/sys/contrib/pf/net/pf_ioctl.c Sat Apr 14 11:29:32 2012 (r234281) +++ projects/pf/head/sys/contrib/pf/net/pf_ioctl.c Sat Apr 14 11:44:10 2012 (r234282) @@ -223,11 +223,6 @@ export_pflow_t *export_pflow_ptr = NUL /* pflog */ pflog_packet_t *pflog_packet_ptr = NULL; -VNET_DEFINE(int, debug_pfugidhack); -SYSCTL_VNET_INT(_debug, OID_AUTO, pfugidhack, CTLFLAG_RW, - &VNET_NAME(debug_pfugidhack), 0, - "Enable/disable pf user/group rules mpsafe hack"); - static void init_pf_mutex(void) { @@ -1334,12 +1329,6 @@ pfioctl(struct cdev *dev, u_long cmd, ca break; } - if (!V_debug_pfugidhack && (rule->uid.op || rule->gid.op || - rule->log & PF_LOG_SOCKET_LOOKUP)) { - DPFPRINTF(PF_DEBUG_MISC, - ("pf: debug.pfugidhack enabled\n")); - V_debug_pfugidhack = 1; - } rule->rpool.cur = TAILQ_FIRST(&rule->rpool.list); rule->evaluations = rule->packets[0] = rule->packets[1] = rule->bytes[0] = rule->bytes[1] = 0; @@ -1606,14 +1595,6 @@ pfioctl(struct cdev *dev, u_long cmd, ca break; } - if (!V_debug_pfugidhack && (newrule->uid.op || - newrule->gid.op || - newrule->log & PF_LOG_SOCKET_LOOKUP)) { - DPFPRINTF(PF_DEBUG_MISC, - ("pf: debug.pfugidhack enabled\n")); - V_debug_pfugidhack = 1; - } - newrule->rpool.cur = TAILQ_FIRST(&newrule->rpool.list); newrule->evaluations = 0; newrule->packets[0] = newrule->packets[1] = 0; @@ -3765,7 +3746,6 @@ pf_load(void) CURVNET_SET(vnet_iter); V_pf_pfil_hooked = 0; V_pf_end_threads = 0; - V_debug_pfugidhack = 0; TAILQ_INIT(&V_pf_tags); TAILQ_INIT(&V_pf_qids); CURVNET_RESTORE(); Modified: projects/pf/head/sys/contrib/pf/net/pf_norm.c ============================================================================== --- projects/pf/head/sys/contrib/pf/net/pf_norm.c Sat Apr 14 11:29:32 2012 (r234281) +++ projects/pf/head/sys/contrib/pf/net/pf_norm.c Sat Apr 14 11:44:10 2012 (r234282) @@ -1126,13 +1126,15 @@ pf_normalize_ip(struct mbuf **m0, int di no_mem: REASON_SET(reason, PFRES_MEMORY); if (r != NULL && r->log) - PFLOG_PACKET(kif, h, m, AF_INET, dir, *reason, r, NULL, NULL, pd); + PFLOG_PACKET(kif, h, m, AF_INET, dir, *reason, r, NULL, NULL, + pd, 1); return (PF_DROP); drop: REASON_SET(reason, PFRES_NORM); if (r != NULL && r->log) - PFLOG_PACKET(kif, h, m, AF_INET, dir, *reason, r, NULL, NULL, pd); + PFLOG_PACKET(kif, h, m, AF_INET, dir, *reason, r, NULL, NULL, + pd, 1); return (PF_DROP); bad: @@ -1146,7 +1148,8 @@ pf_normalize_ip(struct mbuf **m0, int di REASON_SET(reason, PFRES_FRAG); if (r != NULL && r->log) - PFLOG_PACKET(kif, h, m, AF_INET, dir, *reason, r, NULL, NULL, pd); + PFLOG_PACKET(kif, h, m, AF_INET, dir, *reason, r, NULL, NULL, + pd, 1); return (PF_DROP); } @@ -1314,19 +1317,22 @@ pf_normalize_ip6(struct mbuf **m0, int d shortpkt: REASON_SET(reason, PFRES_SHORT); if (r != NULL && r->log) - PFLOG_PACKET(kif, h, m, AF_INET6, dir, *reason, r, NULL, NULL, pd); + PFLOG_PACKET(kif, h, m, AF_INET6, dir, *reason, r, NULL, NULL, + pd, 1); return (PF_DROP); drop: REASON_SET(reason, PFRES_NORM); if (r != NULL && r->log) - PFLOG_PACKET(kif, h, m, AF_INET6, dir, *reason, r, NULL, NULL, pd); + PFLOG_PACKET(kif, h, m, AF_INET6, dir, *reason, r, NULL, NULL, + pd, 1); return (PF_DROP); badfrag: REASON_SET(reason, PFRES_FRAG); if (r != NULL && r->log) - PFLOG_PACKET(kif, h, m, AF_INET6, dir, *reason, r, NULL, NULL, pd); + PFLOG_PACKET(kif, h, m, AF_INET6, dir, *reason, r, NULL, NULL, + pd, 1); return (PF_DROP); } #endif /* INET6 */ @@ -1444,7 +1450,8 @@ pf_normalize_tcp(int dir, struct pfi_kif tcp_drop: REASON_SET(&reason, PFRES_NORM); if (rm != NULL && r->log) - PFLOG_PACKET(kif, h, m, AF_INET, dir, reason, r, NULL, NULL, pd); + PFLOG_PACKET(kif, h, m, AF_INET, dir, reason, r, NULL, NULL, + pd, 1); return (PF_DROP); } Modified: projects/pf/head/sys/contrib/pf/net/pfvar.h ============================================================================== --- projects/pf/head/sys/contrib/pf/net/pfvar.h Sat Apr 14 11:29:32 2012 (r234281) +++ projects/pf/head/sys/contrib/pf/net/pfvar.h Sat Apr 14 11:44:10 2012 (r234282) @@ -924,14 +924,9 @@ struct pf_ruleset; struct pf_pdesc; typedef int pflog_packet_t(struct pfi_kif *, struct mbuf *, sa_family_t, u_int8_t, u_int8_t, struct pf_rule *, struct pf_rule *, - struct pf_ruleset *, struct pf_pdesc *); - + struct pf_ruleset *, struct pf_pdesc *, int); extern pflog_packet_t *pflog_packet_ptr; -/* pf uid hack */ -VNET_DECLARE(int, debug_pfugidhack); -#define V_debug_pfugidhack VNET(debug_pfugidhack) - #define V_pf_end_threads VNET(pf_end_threads) #endif /* _KERNEL */ @@ -1187,7 +1182,6 @@ struct pf_pdesc { int done; uid_t uid; gid_t gid; - pid_t pid; } lookup; u_int64_t tot_len; /* Make Mickey money */ union { @@ -1863,6 +1857,7 @@ int pf_routable(struct pf_addr *addr, sa int); int pf_rtlabel_match(struct pf_addr *, sa_family_t, struct pf_addr_wrap *, int); +int pf_socket_lookup(int, struct pf_pdesc *); struct pf_state_key *pf_alloc_state_key(int); void pfr_initialize(void); int pfr_match_addr(struct pfr_ktable *, struct pf_addr *, sa_family_t);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201204141144.q3EBiBeL028673>