Page MenuHomeFreeBSD
Differential D11137 Diff 142155 sys/netpfil/pf/pf.c

Changeset View

Standalone View

View Options

sys/netpfil/pf/pf.c

Show First 20 Lines • Show All 481 Lines • ▼ Show 20 Lines if (s->anchor.ptr != NULL) \
counter_u64_add(s->anchor.ptr->states_cur, -1); \ counter_u64_add(s->anchor.ptr->states_cur, -1); \
counter_u64_add(s->rule.ptr->states_cur, -1); \ counter_u64_add(s->rule.ptr->states_cur, -1); \
SLIST_FOREACH(mrm, &s->match_rules, entry) \ SLIST_FOREACH(mrm, &s->match_rules, entry) \
counter_u64_add(mrm->r->states_cur, -1); \ counter_u64_add(mrm->r->states_cur, -1); \
} while (0) } while (0)
MALLOC_DEFINE(M_PFHASH, "pf_hash", "pf(4) hash header structures"); MALLOC_DEFINE(M_PFHASH, "pf_hash", "pf(4) hash header structures");
MALLOC_DEFINE(M_PF_RULE_ITEM, "pf_krule_item", "pf(4) rule items"); MALLOC_DEFINE(M_PF_RULE_ITEM, "pf_krule_item", "pf(4) rule items");
VNET_DEFINE(struct pf_udpendpointhash *, pf_udpendpointhash);
kpUnsubmitted
Not Done
Inline Actions

I seem to be missing the allocation (and freeing) of this. Am I looking in the wrong place or is it just not part of the patch?

kp: I seem to be missing the allocation (and freeing) of this. Am I looking in the wrong place or…
VNET_DEFINE(struct pf_keyhash *, pf_keyhash); VNET_DEFINE(struct pf_keyhash *, pf_keyhash);
VNET_DEFINE(struct pf_idhash *, pf_idhash); VNET_DEFINE(struct pf_idhash *, pf_idhash);
VNET_DEFINE(struct pf_srchash *, pf_srchash); VNET_DEFINE(struct pf_srchash *, pf_srchash);
SYSCTL_NODE(_net, OID_AUTO, pf, CTLFLAG_RW | CTLFLAG_MPSAFE, 0, SYSCTL_NODE(_net, OID_AUTO, pf, CTLFLAG_RW | CTLFLAG_MPSAFE, 0,
"pf(4)"); "pf(4)");
VNET_DEFINE(u_long, pf_hashmask); VNET_DEFINE(u_long, pf_hashmask);
VNET_DEFINE(u_long, pf_srchashmask); VNET_DEFINE(u_long, pf_srchashmask);
VNET_DEFINE_STATIC(u_long, pf_hashsize); VNET_DEFINE_STATIC(u_long, pf_hashsize);
#define V_pf_hashsize VNET(pf_hashsize) #define V_pf_hashsize VNET(pf_hashsize)
VNET_DEFINE_STATIC(u_long, pf_srchashsize); VNET_DEFINE_STATIC(u_long, pf_srchashsize);
#define V_pf_srchashsize VNET(pf_srchashsize) #define V_pf_srchashsize VNET(pf_srchashsize)
u_long pf_ioctl_maxcount = 65535; u_long pf_ioctl_maxcount = 65535;
SYSCTL_ULONG(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_VNET | CTLFLAG_RDTUN, SYSCTL_ULONG(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_VNET | CTLFLAG_RDTUN,
&VNET_NAME(pf_hashsize), 0, "Size of pf(4) states hashtable"); &VNET_NAME(pf_hashsize), 0, "Size of pf(4) states hashtable");
SYSCTL_ULONG(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_VNET | CTLFLAG_RDTUN, SYSCTL_ULONG(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_VNET | CTLFLAG_RDTUN,
&VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes hashtable"); &VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes hashtable");
SYSCTL_ULONG(_net_pf, OID_AUTO, request_maxcount, CTLFLAG_RWTUN, SYSCTL_ULONG(_net_pf, OID_AUTO, request_maxcount, CTLFLAG_RWTUN,
&pf_ioctl_maxcount, 0, "Maximum number of tables, addresses, ... in a single ioctl() call"); &pf_ioctl_maxcount, 0, "Maximum number of tables, addresses, ... in a single ioctl() call");
kpUnsubmitted
Done
Inline Actions

That'll want an entry in the pf(4) man page.

kp: That'll want an entry in the pf(4) man page.
VNET_DEFINE(void *, pf_swi_cookie); VNET_DEFINE(void *, pf_swi_cookie);
VNET_DEFINE(struct intr_event *, pf_swi_ie); VNET_DEFINE(struct intr_event *, pf_swi_ie);
VNET_DEFINE(uint32_t, pf_hashseed); VNET_DEFINE(uint32_t, pf_hashseed);
#define V_pf_hashseed VNET(pf_hashseed) #define V_pf_hashseed VNET(pf_hashseed)
static void static void
▲ Show 20 Lines • Show All 163 Lines • ▼ Show 20 Linespf_hashsrc(struct pf_addr *addr, sa_family_t af)
switch (af) { switch (af) {
case AF_INET: case AF_INET:
h = murmur3_32_hash32((uint32_t *)&addr->v4, h = murmur3_32_hash32((uint32_t *)&addr->v4,
sizeof(addr->v4)/sizeof(uint32_t), V_pf_hashseed); sizeof(addr->v4)/sizeof(uint32_t), V_pf_hashseed);
break; break;
case AF_INET6: case AF_INET6:
h = murmur3_32_hash32((uint32_t *)&addr->v6, h = murmur3_32_hash32((uint32_t *)&addr->v6,
sizeof(addr->v6)/sizeof(uint32_t), V_pf_hashseed); sizeof(addr->v6)/sizeof(uint32_t), V_pf_hashseed);
glebiusUnsubmitted
Not Done
Inline Actions

Please use standard C keyword "inline" instead of "__inline" gcc-ism.

glebius: Please use standard C keyword "inline" instead of "__inline" gcc-ism.
break; break;
default: default:
panic("%s: unknown address family %u", __func__, af); panic("%s: unknown address family %u", __func__, af);
} }
return (h & V_pf_srchashmask); return (h & V_pf_srchashmask);
} }
#ifdef ALTQ #ifdef ALTQ
static int static int
pf_state_hash(struct pf_kstate *s) pf_state_hash(struct pf_kstate *s)
{ {
u_int32_t hv = (intptr_t)s / sizeof(*s); u_int32_t hv = (intptr_t)s / sizeof(*s);
hv ^= crc32(&s->src, sizeof(s->src)); hv ^= crc32(&s->src, sizeof(s->src));
hv ^= crc32(&s->dst, sizeof(s->dst)); hv ^= crc32(&s->dst, sizeof(s->dst));
if (hv == 0) if (hv == 0)
kpUnsubmitted
Done
Inline Actions

Shouldn't that be V_pf_udpendpointhashmask ?

kp: Shouldn't that be V_pf_udpendpointhashmask ?
hv = 1; hv = 1;
return (hv); return (hv);
} }
#endif #endif
static __inline void static __inline void
pf_set_protostate(struct pf_kstate *s, int which, u_int8_t newstate) pf_set_protostate(struct pf_kstate *s, int which, u_int8_t newstate)
{ {
▲ Show 20 Lines • Show All 369 Lines • ▼ Show 20 Linespf_initialize(void)
struct pf_srchash *sh; struct pf_srchash *sh;
u_int i; u_int i;
if (V_pf_hashsize == 0 || !powerof2(V_pf_hashsize)) if (V_pf_hashsize == 0 || !powerof2(V_pf_hashsize))
V_pf_hashsize = PF_HASHSIZ; V_pf_hashsize = PF_HASHSIZ;
if (V_pf_srchashsize == 0 || !powerof2(V_pf_srchashsize)) if (V_pf_srchashsize == 0 || !powerof2(V_pf_srchashsize))
V_pf_srchashsize = PF_SRCHASHSIZ; V_pf_srchashsize = PF_SRCHASHSIZ;
V_pf_hashseed = arc4random(); V_pf_hashseed = arc4random();
kpUnsubmitted
Done
Inline Actions

Do we want to default to the same size as the state table, or maybe something smaller?

It's hardly scientific, but my local gateway has about twice as much TCP as UDP, and on my server box it's more like 4 to 1, so I'd go with half or even a quarter of PF_HASHSIZ as a default.

kp: Do we want to default to the same size as the state table, or maybe something smaller? It's…
/* States and state keys storage. */ /* States and state keys storage. */
V_pf_state_z = uma_zcreate("pf states", sizeof(struct pf_kstate), V_pf_state_z = uma_zcreate("pf states", sizeof(struct pf_kstate),
NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, 0); NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, 0);
V_pf_limits[PF_LIMIT_STATES].zone = V_pf_state_z; V_pf_limits[PF_LIMIT_STATES].zone = V_pf_state_z;
uma_zone_set_max(V_pf_state_z, PFSTATE_HIWAT); uma_zone_set_max(V_pf_state_z, PFSTATE_HIWAT);
uma_zone_set_warning(V_pf_state_z, "PF states limit reached"); uma_zone_set_warning(V_pf_state_z, "PF states limit reached");
Show All 16 Lines if (V_pf_keyhash == NULL || V_pf_idhash == NULL) {
V_pf_keyhash = mallocarray(V_pf_hashsize, V_pf_keyhash = mallocarray(V_pf_hashsize,
sizeof(struct pf_keyhash), M_PFHASH, M_WAITOK | M_ZERO); sizeof(struct pf_keyhash), M_PFHASH, M_WAITOK | M_ZERO);
V_pf_idhash = mallocarray(V_pf_hashsize, V_pf_idhash = mallocarray(V_pf_hashsize,
sizeof(struct pf_idhash), M_PFHASH, M_WAITOK | M_ZERO); sizeof(struct pf_idhash), M_PFHASH, M_WAITOK | M_ZERO);
} }
V_pf_hashmask = V_pf_hashsize - 1; V_pf_hashmask = V_pf_hashsize - 1;
for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask; for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask;
i++, kh++, ih++) { i++, kh++, ih++) {
kpUnsubmitted
Done
Inline Actions

I'd leave this out.

kp: I'd leave this out.
mtx_init(&kh->lock, "pf_keyhash", NULL, MTX_DEF | MTX_DUPOK); mtx_init(&kh->lock, "pf_keyhash", NULL, MTX_DEF | MTX_DUPOK);
mtx_init(&ih->lock, "pf_idhash", NULL, MTX_DEF); mtx_init(&ih->lock, "pf_idhash", NULL, MTX_DEF);
mtx_init(&uh->lock, "pf_udpendpointhash", NULL,
kpUnsubmitted
Not Done
Inline Actions

Oh dear god. Another lock for the data path?
This is worrying.

At the very least it needs thorough documentation w.r.t what it locks and what the lock order expectations are.

kp: Oh dear god. Another lock for the data path? This is worrying. At the very least it needs…
MTX_DEF | MTX_DUPOK);
kpUnsubmitted
Not Done
Inline Actions

MTX_DUPOK

Ick. Probably required for the same reason we need it for pf_keyhash, but ick nonetheless.

kp: > MTX_DUPOK Ick. Probably required for the same reason we need it for pf_keyhash, but ick…
} }
/* Source nodes. */ /* Source nodes. */
V_pf_sources_z = uma_zcreate("pf source nodes", V_pf_sources_z = uma_zcreate("pf source nodes",
sizeof(struct pf_ksrc_node), NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, sizeof(struct pf_ksrc_node), NULL, NULL, NULL, NULL, UMA_ALIGN_PTR,
0); 0);
V_pf_limits[PF_LIMIT_SRC_NODES].zone = V_pf_sources_z; V_pf_limits[PF_LIMIT_SRC_NODES].zone = V_pf_sources_z;
uma_zone_set_max(V_pf_sources_z, PFSNODE_HIWAT); uma_zone_set_max(V_pf_sources_z, PFSNODE_HIWAT);
Show All 20 Linespf_initialize(void)
TAILQ_INIT(&V_pf_altqs[2]); TAILQ_INIT(&V_pf_altqs[2]);
TAILQ_INIT(&V_pf_altqs[3]); TAILQ_INIT(&V_pf_altqs[3]);
TAILQ_INIT(&V_pf_pabuf); TAILQ_INIT(&V_pf_pabuf);
V_pf_altqs_active = &V_pf_altqs[0]; V_pf_altqs_active = &V_pf_altqs[0];
V_pf_altq_ifs_active = &V_pf_altqs[1]; V_pf_altq_ifs_active = &V_pf_altqs[1];
V_pf_altqs_inactive = &V_pf_altqs[2]; V_pf_altqs_inactive = &V_pf_altqs[2];
V_pf_altq_ifs_inactive = &V_pf_altqs[3]; V_pf_altq_ifs_inactive = &V_pf_altqs[3];
/* Send & overload+flush queues. */ /* Send & overload+flush queues. */
kpUnsubmitted
Done
Inline Actions

And given that we have a hardcoded default for the udp hash size we probably want a define (at least, if we end up choosing a different default for it than PF_HASHSIZ).

kp: And given that we have a hardcoded default for the udp hash size we probably want a define (at…
STAILQ_INIT(&V_pf_sendqueue); STAILQ_INIT(&V_pf_sendqueue);
SLIST_INIT(&V_pf_overloadqueue); SLIST_INIT(&V_pf_overloadqueue);
TASK_INIT(&V_pf_overloadtask, 0, pf_overload_task, curvnet); TASK_INIT(&V_pf_overloadtask, 0, pf_overload_task, curvnet);
/* Unlinked, but may be referenced rules. */ /* Unlinked, but may be referenced rules. */
TAILQ_INIT(&V_pf_unlinked_rules); TAILQ_INIT(&V_pf_unlinked_rules);
kpUnsubmitted
Done
Inline Actions

<= V_pf_udpendpointhashmask ?

Arguably another reason for having a different default size for the udp hash table is that we'd encounter issues like this in default configurations. Now we're only going to see this when a user twiddles the setting.

kp: <= V_pf_udpendpointhashmask ? Arguably another reason for having a different default size for…
} }
void void
pf_mtag_cleanup(void) pf_mtag_cleanup(void)
{ {
uma_zdestroy(pf_mtag_z); uma_zdestroy(pf_mtag_z);
} }
void void
pf_cleanup(void) pf_cleanup(void)
{ {
struct pf_keyhash *kh; struct pf_keyhash *kh;
struct pf_idhash *ih; struct pf_idhash *ih;
struct pf_srchash *sh; struct pf_srchash *sh;
struct pf_send_entry *pfse, *next; struct pf_send_entry *pfse, *next;
u_int i; u_int i;
for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask; for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask;
i++, kh++, ih++) { i++, kh++, ih++) {
kpUnsubmitted
Done
Inline Actions

Is that right? What happens if V_pf_hashmask != V_pf_udpendpointhashmask?

kp: Is that right? What happens if V_pf_hashmask != V_pf_udpendpointhashmask?
KASSERT(LIST_EMPTY(&kh->keys), ("%s: key hash not empty", KASSERT(LIST_EMPTY(&kh->keys), ("%s: key hash not empty",
__func__)); __func__));
KASSERT(LIST_EMPTY(&ih->states), ("%s: id hash not empty", KASSERT(LIST_EMPTY(&ih->states), ("%s: id hash not empty",
__func__)); __func__));
mtx_destroy(&kh->lock); mtx_destroy(&kh->lock);
mtx_destroy(&ih->lock); mtx_destroy(&ih->lock);
} }
free(V_pf_keyhash, M_PFHASH); free(V_pf_keyhash, M_PFHASH);
▲ Show 20 Lines • Show All 522 Lines • ▼ Show 20 Linessecond_run:
return (ret); return (ret);
} }
/* /*
* FIXME * FIXME
* This routine is inefficient -- locks the state only to unlock immediately on * This routine is inefficient -- locks the state only to unlock immediately on
* return. * return.
* It is racy -- after the state is unlocked nothing stops other threads from * It is racy -- after the state is unlocked nothing stops other threads from
glebiusUnsubmitted
Not Done
Inline Actions

Please follow style(9), do not initialize variables in declaration. They rule may be violated sometimes, but calling function in initializer is too much!

glebius: Please follow style(9), do not initialize variables in declaration. They rule may be violated…
* removing it. * removing it.
*/ */
bool bool
pf_find_state_all_exists(const struct pf_state_key_cmp *key, u_int dir) pf_find_state_all_exists(const struct pf_state_key_cmp *key, u_int dir)
{ {
struct pf_kstate *s; struct pf_kstate *s;
s = pf_find_state_all(key, dir, NULL); s = pf_find_state_all(key, dir, NULL);
if (s != NULL) { if (s != NULL) {
PF_STATE_UNLOCK(s); PF_STATE_UNLOCK(s);
return (true); return (true);
} }
glebiusUnsubmitted
Not Done
Inline Actions

Please put return values into braces, to follow style(9). This refers to all new returns in this patch.

glebius: Please put return values into braces, to follow style(9). This refers to all new returns in…
return (false); return (false);
} }
struct pf_udp_mapping *
pf_udp_mapping_create(sa_family_t af, struct pf_addr *src_addr, uint16_t src_port,
struct pf_addr *nat_addr, uint16_t nat_port)
{
struct pf_udp_mapping *mapping;
mapping = uma_zalloc(V_pf_udp_mapping_z, M_NOWAIT | M_ZERO);
if (mapping == NULL)
return NULL;
kpUnsubmitted
Not Done
Inline Actions

style(9)

kp: style(9)
kpUnsubmitted
Not Done
Inline Actions

return (NULL); please.

kp: return (NULL); please.
PF_ACPY(&mapping->endpoints[0].addr, src_addr, af);
mapping->endpoints[0].port = src_port;
mapping->endpoints[0].af = af;
mapping->endpoints[0].mapping = mapping;
PF_ACPY(&mapping->endpoints[1].addr, nat_addr, af);
mapping->endpoints[1].port = nat_port;
mapping->endpoints[1].af = af;
mapping->endpoints[1].mapping = mapping;
refcount_init(&mapping->refs, 1);
return (mapping);
}
int
pf_udp_mapping_insert(struct pf_udp_mapping *mapping)
{
struct pf_udpendpointhash *h0, *h1;
struct pf_udp_endpoint *endpoint;
int ret = 1;
kpUnsubmitted
Done
Inline Actions

I'd default to EEXIST, or maybe just switch to a bool return value.

kp: I'd default to EEXIST, or maybe just switch to a bool return value.
h0 = &V_pf_udpendpointhash[pf_hashudpendpoint(&mapping->endpoints[0])];
h1 = &V_pf_udpendpointhash[pf_hashudpendpoint(&mapping->endpoints[1])];
if (h0 == h1) {
PF_HASHROW_LOCK(h0);
} else if (h0 < h1) {
PF_HASHROW_LOCK(h0);
PF_HASHROW_LOCK(h1);
} else {
PF_HASHROW_LOCK(h1);
PF_HASHROW_LOCK(h0);
}
LIST_FOREACH(endpoint, &h0->endpoints, entry) {
if (bcmp(endpoint, &mapping->endpoints[0],
sizeof(struct pf_udp_endpoint_cmp)) == 0)
break;
}
if (endpoint != NULL)
goto cleanup;
LIST_FOREACH(endpoint, &h1->endpoints, entry) {
glebiusUnsubmitted
Not Done
Inline Actions

Please put empty line after declarations.

glebius: Please put empty line after declarations.
if (bcmp(endpoint, &mapping->endpoints[1],
sizeof(struct pf_udp_endpoint_cmp)) == 0)
break;
}
if (endpoint != NULL)
goto cleanup;
LIST_INSERT_HEAD(&h0->endpoints, &mapping->endpoints[0], entry);
LIST_INSERT_HEAD(&h1->endpoints, &mapping->endpoints[1], entry);
ret = 0;
cleanup:
if (h0 != h1) {
kpUnsubmitted
Done
Inline Actions

Seems to have wound up with spaces to rather than tabs here.

kp: Seems to have wound up with spaces to rather than tabs here.
PF_HASHROW_UNLOCK(h0);
PF_HASHROW_UNLOCK(h1);
} else {
PF_HASHROW_UNLOCK(h0);
}
return (ret);
}
void
pf_udp_mapping_release(struct pf_udp_mapping *mapping)
{
/* refcount is synchronized on the source endpoint's row lock */
kpUnsubmitted
Done
Inline Actions

I like that that's documented, but wonder if it wouldn't be more useful to put that in the struct definition instead. Or possibly both here and there.

kp: I like that that's documented, but wonder if it wouldn't be more useful to put that in the…
struct pf_udpendpointhash *h0, *h1;
h0 = &V_pf_udpendpointhash[pf_hashudpendpoint(&mapping->endpoints[0])];
PF_HASHROW_LOCK(h0);
if (refcount_release(&mapping->refs)) {
LIST_REMOVE(&mapping->endpoints[0], entry);
PF_HASHROW_UNLOCK(h0);
h1 = &V_pf_udpendpointhash[pf_hashudpendpoint(&mapping->endpoints[1])];
PF_HASHROW_LOCK(h1);
kpUnsubmitted
Not Done
Inline Actions

Is there a risk of races because we remove separate entries (why are there two?) non-atomically?

kp: Is there a risk of races because we remove separate entries (why are there two?) non-atomically?
LIST_REMOVE(&mapping->endpoints[1], entry);
PF_HASHROW_UNLOCK(h1);
uma_zfree(V_pf_udp_mapping_z, mapping);
} else {
PF_HASHROW_UNLOCK(h0);
}
}
struct pf_udp_mapping *
pf_udp_mapping_find(struct pf_udp_endpoint_cmp *key)
{
struct pf_udpendpointhash *uh;
struct pf_udp_endpoint *endpoint;
uh = &V_pf_udpendpointhash[pf_hashudpendpoint((struct pf_udp_endpoint*)key)];
PF_HASHROW_LOCK(uh);
LIST_FOREACH(endpoint, &uh->endpoints, entry) {
if (bcmp(endpoint, key, sizeof(struct pf_udp_endpoint_cmp)) == 0 &&
bcmp(endpoint, &endpoint->mapping->endpoints[0],
sizeof(struct pf_udp_endpoint_cmp)) == 0)
break;
}
if (endpoint == NULL) {
PF_HASHROW_UNLOCK(uh);
return NULL;
kpUnsubmitted
Done
Inline Actions

style(9).

kp: style(9).
}
refcount_acquire(&endpoint->mapping->refs);
PF_HASHROW_UNLOCK(uh);
return (endpoint->mapping);
}
/* END state table stuff */ /* END state table stuff */
static void static void
pf_send(struct pf_send_entry *pfse) pf_send(struct pf_send_entry *pfse)
{ {
PF_SENDQ_LOCK(); PF_SENDQ_LOCK();
STAILQ_INSERT_TAIL(&V_pf_sendqueue, pfse, pfse_next); STAILQ_INSERT_TAIL(&V_pf_sendqueue, pfse, pfse_next);
▲ Show 20 Lines • Show All 2,856 Lines • ▼ Show 20 Linespf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, struct pfi_kkif *kif,
int tag = -1; int tag = -1;
int asd = 0; int asd = 0;
int match = 0; int match = 0;
int state_icmp = 0, icmp_dir, multi; int state_icmp = 0, icmp_dir, multi;
u_int16_t sport = 0, dport = 0, virtual_type, virtual_id; u_int16_t sport = 0, dport = 0, virtual_type, virtual_id;
u_int16_t bproto_sum = 0, bip_sum = 0; u_int16_t bproto_sum = 0, bip_sum = 0;
u_int8_t icmptype = 0, icmpcode = 0; u_int8_t icmptype = 0, icmpcode = 0;
struct pf_kanchor_stackframe anchor_stack[PF_ANCHOR_STACKSIZE]; struct pf_kanchor_stackframe anchor_stack[PF_ANCHOR_STACKSIZE];
kpUnsubmitted
Done
Inline Actions

Align *udp_mapping with the other variables with a tab.

kp: Align *udp_mapping with the other variables with a tab.
PF_RULES_RASSERT(); PF_RULES_RASSERT();
if (inp != NULL) { if (inp != NULL) {
INP_LOCK_ASSERT(inp); INP_LOCK_ASSERT(inp);
pd->lookup.uid = inp->inp_cred->cr_uid; pd->lookup.uid = inp->inp_cred->cr_uid;
pd->lookup.gid = inp->inp_cred->cr_groups[0]; pd->lookup.gid = inp->inp_cred->cr_groups[0];
pd->lookup.done = 1; pd->lookup.done = 1;
} }
▲ Show 20 Lines • Show All 347 Lines • ▼ Show 20 Lines#endif /* INET */
if (!state_icmp && (r->keep_state || nr != NULL || if (!state_icmp && (r->keep_state || nr != NULL ||
(pd->flags & PFDESC_TCP_NORM))) { (pd->flags & PFDESC_TCP_NORM))) {
int action; int action;
action = pf_create_state(r, nr, a, pd, nsn, nk, sk, m, off, action = pf_create_state(r, nr, a, pd, nsn, nk, sk, m, off,
sport, dport, &rewrite, kif, sm, tag, bproto_sum, bip_sum, sport, dport, &rewrite, kif, sm, tag, bproto_sum, bip_sum,
hdrlen, &match_rules); hdrlen, &match_rules);
if (action != PF_PASS) { if (action != PF_PASS) {
if (action == PF_DROP && if (action == PF_DROP &&
kpUnsubmitted
Done
Inline Actions

I wonder if we shouldn't put the NULL check in pf_udp_mapping_release() instead.
Similar to free() and such, it'd be safe to do pf_udp_mapping_release(NULL), so we wouldn't have to do this check everywhere we call it.

It looks like there are at least three callers, so that'd save a couple of lines of code too.

kp: I wonder if we shouldn't put the NULL check in pf_udp_mapping_release() instead. Similar to…
(r->rule_flag & PFRULE_RETURN)) (r->rule_flag & PFRULE_RETURN))
pf_return(r, nr, pd, sk, off, m, th, kif, pf_return(r, nr, pd, sk, off, m, th, kif,
bproto_sum, bip_sum, hdrlen, &reason, bproto_sum, bip_sum, hdrlen, &reason,
pd->act.rtableid); pd->act.rtableid);
return (action); return (action);
} }
} else { } else {
while ((ri = SLIST_FIRST(&match_rules))) { while ((ri = SLIST_FIRST(&match_rules))) {
Show All 26 Linescleanup:
while ((ri = SLIST_FIRST(&match_rules))) { while ((ri = SLIST_FIRST(&match_rules))) {
SLIST_REMOVE_HEAD(&match_rules, entry); SLIST_REMOVE_HEAD(&match_rules, entry);
free(ri, M_PF_RULE_ITEM); free(ri, M_PF_RULE_ITEM);
} }
uma_zfree(V_pf_state_key_z, sk); uma_zfree(V_pf_state_key_z, sk);
uma_zfree(V_pf_state_key_z, nk); uma_zfree(V_pf_state_key_z, nk);
return (PF_DROP); return (PF_DROP);
} }
kpUnsubmitted
Done
Inline Actions

Doesn't need the NULL check any more.

kp: Doesn't need the NULL check any more.
static int static int
pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a,
struct pf_pdesc *pd, struct pf_ksrc_node *nsn, struct pf_state_key *nk, struct pf_pdesc *pd, struct pf_ksrc_node *nsn, struct pf_state_key *nk,
struct pf_state_key *sk, struct mbuf *m, int off, u_int16_t sport, struct pf_state_key *sk, struct mbuf *m, int off, u_int16_t sport,
u_int16_t dport, int *rewrite, struct pfi_kkif *kif, struct pf_kstate **sm, u_int16_t dport, int *rewrite, struct pfi_kkif *kif, struct pf_kstate **sm,
int tag, u_int16_t bproto_sum, u_int16_t bip_sum, int hdrlen, int tag, u_int16_t bproto_sum, u_int16_t bip_sum, int hdrlen,
struct pf_krule_slist *match_rules) struct pf_krule_slist *match_rules)
▲ Show 20 Lines • Show All 4,378 Lines • Show Last 20 Lines