svn commit: r234282 - projects/pf/head/sys/contrib/pf/net
Gleb Smirnoff
glebius at FreeBSD.org
Sat Apr 14 11:44:11 UTC 2012
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);
More information about the svn-src-projects
mailing list