Date: Thu, 12 Apr 2012 21:56:30 +0200 From: =?ISO-8859-1?Q?Ermal_Lu=E7i?= <eri@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r234187 - projects/pf/head/sys/contrib/pf/net Message-ID: <CAPBZQG3ntZC2SuPPZBG%2BLpvixFv=rumAk0J9bSxS36YucmP4ng@mail.gmail.com> In-Reply-To: <201204121556.q3CFu4nH035176@svn.freebsd.org> References: <201204121556.q3CFu4nH035176@svn.freebsd.org>
index | next in thread | previous in thread | raw e-mail
You do understand that some of these function are part of core functionality of pf(4) as synproxy etc?! On Thu, Apr 12, 2012 at 5:56 PM, Gleb Smirnoff <glebius@freebsd.org> wrote: > Author: glebius > Date: Thu Apr 12 15:56:04 2012 > New Revision: 234187 > URL: http://svn.freebsd.org/changeset/base/234187 > > Log: > To avoid unsafe lock dropping and decouple stack in pf_send_tcp() > and pf_send_icmp() create a queue for pf-generated packets and > an swi, that would service them. > > Modified: > 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/pfvar.h > > Modified: projects/pf/head/sys/contrib/pf/net/pf.c > ============================================================================== > --- projects/pf/head/sys/contrib/pf/net/pf.c Thu Apr 12 14:49:25 2012 (r234186) > +++ projects/pf/head/sys/contrib/pf/net/pf.c Thu Apr 12 15:56:04 2012 (r234187) > @@ -53,7 +53,9 @@ __FBSDID("$FreeBSD$"); > > #include <sys/param.h> > #include <sys/systm.h> > +#include <sys/bus.h> > #include <sys/mbuf.h> > +#include <sys/interrupt.h> > #include <sys/filio.h> > #include <sys/socket.h> > #include <sys/socketvar.h> > @@ -114,8 +116,6 @@ __FBSDID("$FreeBSD$"); > #include <sys/ucred.h> > #include <security/mac/mac_framework.h> > > -extern int ip_optcopy(struct ip *, struct ip *); > - > #define DPFPRINTF(n, x) if (V_pf_status.debug >= (n)) printf x > > /* > @@ -152,6 +152,41 @@ struct pf_anchor_stackframe { > VNET_DEFINE(struct pf_anchor_stackframe, pf_anchor_stack[64]); > #define V_pf_anchor_stack VNET(pf_anchor_stack) > > +/* > + * Queue for pf_intr() sends. > + */ > +MALLOC_DEFINE(M_PFTEMP, "pf temp", "pf(4) temporary allocations"); > +struct pf_send_entry { > + STAILQ_ENTRY(pf_send_entry) pfse_next; > + struct mbuf *pfse_m; > + enum { > + PFSE_IP, > + PFSE_IP6, > + PFSE_ICMP, > + PFSE_ICMP6, > + } pfse_type; > + union { > + struct route ro; > + struct { > + int type; > + int code; > + int mtu; > + } icmpopts; > + } u; > +#define pfse_ro u.ro > +#define pfse_icmp_type u.icmpopts.type > +#define pfse_icmp_code u.icmpopts.code > +#define pfse_icmp_mtu u.icmpopts.mtu > +}; > + > +STAILQ_HEAD(pf_send_head, pf_send_entry); > +static VNET_DEFINE(struct pf_send_head, pf_sendqueue); > +#define V_pf_sendqueue VNET(pf_sendqueue) > + > +static struct mtx pf_sendqueue_mtx; > +#define PF_QUEUE_LOCK() mtx_lock(&pf_sendqueue_mtx); > +#define PF_QUEUE_UNLOCK() mtx_unlock(&pf_sendqueue_mtx); > + > VNET_DEFINE(uma_zone_t, pf_src_tree_z); > VNET_DEFINE(uma_zone_t, pf_rule_z); > VNET_DEFINE(uma_zone_t, pf_pooladdr_z); > @@ -321,6 +356,8 @@ VNET_DEFINE(struct pf_keyhash *, pf_keyh > VNET_DEFINE(struct pf_idhash *, pf_idhash); > VNET_DEFINE(u_long, pf_hashmask); > > +VNET_DEFINE(void *, pf_swi_cookie); > + > RB_GENERATE(pf_src_tree, pf_src_node, entry, pf_src_compare); > > static __inline int > @@ -684,6 +721,10 @@ pf_initialize() > V_pf_altqs_active = &V_pf_altqs[0]; > V_pf_altqs_inactive = &V_pf_altqs[1]; > > + /* Send queue. */ > + STAILQ_INIT(&V_pf_sendqueue); > + mtx_init(&pf_sendqueue_mtx, "pf send queue", NULL, MTX_DEF); > + > /* XXXGL: sort this out */ > V_pf_rule_z = uma_zcreate("pf rules", sizeof(struct pf_rule), > NULL, NULL, NULL, NULL, UMA_ALIGN_PTR, 0); > @@ -707,6 +748,7 @@ pf_cleanup() > { > struct pf_keyhash *kh; > struct pf_idhash *ih; > + struct pf_send_entry *pfse, *next; > u_int i; > > for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask; > @@ -721,6 +763,12 @@ pf_cleanup() > free(V_pf_keyhash, M_PFHASH); > free(V_pf_idhash, M_PFHASH); > > + STAILQ_FOREACH_SAFE(pfse, &V_pf_sendqueue, pfse_next, next) { > + m_freem(pfse->pfse_m); > + free(pfse, M_PFTEMP); > + } > + mtx_destroy(&pf_sendqueue_mtx); > + > uma_zdestroy(V_pf_src_tree_z); > uma_zdestroy(V_pf_rule_z); > uma_zdestroy(V_pf_state_z); > @@ -1185,6 +1233,55 @@ second_run: > > /* END state table stuff */ > > +static void > +pf_send(struct pf_send_entry *pfse) > +{ > + > + PF_QUEUE_LOCK(); > + STAILQ_INSERT_TAIL(&V_pf_sendqueue, pfse, pfse_next); > + PF_QUEUE_UNLOCK(); > + swi_sched(V_pf_swi_cookie, 0); > +} > + > +void > +pf_intr(void *v) > +{ > + struct pf_send_head queue; > + struct pf_send_entry *pfse, *next; > + struct pf_sen > + > + CURVNET_SET((struct vnet *)v); > + > + PF_QUEUE_LOCK(); > + queue = V_pf_sendqueue; > + STAILQ_INIT(&V_pf_sendqueue); > + PF_QUEUE_UNLOCK(); > + > + STAILQ_FOREACH_SAFE(pfse, &queue, pfse_next, next) { > + switch (pfse->pfse_type) { > + case PFSE_IP: > + ip_output(pfse->pfse_m, NULL, NULL, 0, NULL, NULL); > + break; > + case PFSE_IP6: > + ip6_output(pfse->pfse_m, NULL, NULL, 0, NULL, NULL, > + NULL); > + break; > + case PFSE_ICMP: > + icmp_error(pfse->pfse_m, pfse->pfse_icmp_type, > + pfse->pfse_icmp_code, 0, pfse->pfse_icmp_mtu); > + break; > + case PFSE_ICMP6: > + icmp6_error(pfse->pfse_m, pfse->pfse_icmp_type, > + pfse->pfse_icmp_code, pfse->pfse_icmp_mtu); > + break; > + default: > + panic("%s: unknown type", __func__); > + } > + free(pfse, M_PFTEMP); > + } > + > + CURVNET_RESTORE(); > +} > > void > pf_purge_thread(void *v) > @@ -1951,6 +2048,7 @@ pf_send_tcp(struct mbuf *replyto, const > u_int8_t flags, u_int16_t win, u_int16_t mss, u_int8_t ttl, int tag, > u_int16_t rtag, struct ifnet *ifp) > { > + struct pf_send_entry *pfse; > struct mbuf *m; > int len, tlen; > #ifdef INET > @@ -1963,27 +2061,8 @@ pf_send_tcp(struct mbuf *replyto, const > char *opt; > struct pf_mtag *pf_mtag; > > - KASSERT( > -#ifdef INET > - af == AF_INET > -#else > - 0 > -#endif > - || > -#ifdef INET6 > - af == AF_INET6 > -#else > - 0 > -#endif > - , ("Unsupported AF %d", af)); > len = 0; > th = NULL; > -#ifdef INET > - h = NULL; > -#endif > -#ifdef INET6 > - h6 = NULL; > -#endif > > /* maximum segment size tcp option */ > tlen = sizeof(struct tcphdr); > @@ -2001,16 +2080,24 @@ pf_send_tcp(struct mbuf *replyto, const > len = sizeof(struct ip6_hdr) + tlen; > break; > #endif /* INET6 */ > + default: > + panic("%s: unsupported af %d", __func__, af); > } > > - /* create outgoing mbuf */ > + /* Allocate outgoing queue entry, mbuf and mbuf tag. */ > + pfse = malloc(sizeof(*pfse), M_PFTEMP, M_NOWAIT); > + if (pfse == NULL) > + return; > m = m_gethdr(M_NOWAIT, MT_HEADER); > - if (m == NULL) > + if (m == NULL) { > + free(pfse, M_PFTEMP); > return; > + } > #ifdef MAC > mac_netinet_firewall_send(m); > #endif > if ((pf_mtag = pf_get_mtag(m)) == NULL) { > + free(pfse, M_PFTEMP); > m_freem(m); > return; > } > @@ -2096,9 +2183,8 @@ pf_send_tcp(struct mbuf *replyto, const > h->ip_len = len; > h->ip_ttl = ttl ? ttl : V_ip_defttl; > h->ip_sum = 0; > - PF_UNLOCK(); > - ip_output(m, NULL, NULL, 0, NULL, NULL); > - PF_LOCK(); > + > + pfse->pfse_type = PFSE_IP; > break; > #endif /* INET */ > #ifdef INET6 > @@ -2110,29 +2196,36 @@ pf_send_tcp(struct mbuf *replyto, const > h6->ip6_vfc |= IPV6_VERSION; > h6->ip6_hlim = IPV6_DEFHLIM; > > - PF_UNLOCK(); > - ip6_output(m, NULL, NULL, 0, NULL, NULL, NULL); > - PF_LOCK(); > + pfse->pfse_type = PFSE_IP6; > break; > #endif /* INET6 */ > } > + pfse->pfse_m = m; > + pf_send(pfse); > } > > static void > pf_send_icmp(struct mbuf *m, u_int8_t type, u_int8_t code, sa_family_t af, > struct pf_rule *r) > { > - struct mbuf *m0; > -#ifdef INET > - struct ip *ip; > -#endif > + struct pf_send_entry *pfse; > + struct mbuf *m0; > struct pf_mtag *pf_mtag; > > - if ((m0 = m_copypacket(m, M_NOWAIT)) == NULL) > + /* Allocate outgoing queue entry, mbuf and mbuf tag. */ > + pfse = malloc(sizeof(*pfse), M_PFTEMP, M_NOWAIT); > + if (pfse == NULL) > + return; > + > + if ((m0 = m_copypacket(m, M_NOWAIT)) == NULL) { > + free(pfse, M_PFTEMP); > return; > + } > > - if ((pf_mtag = pf_get_mtag(m0)) == NULL) > + if ((pf_mtag = pf_get_mtag(m0)) == NULL) { > + free(pfse, M_PFTEMP); > return; > + } > /* XXX: revisit */ > m0->m_flags |= M_SKIP_FIREWALL; > > @@ -2153,23 +2246,28 @@ pf_send_icmp(struct mbuf *m, u_int8_t ty > switch (af) { > #ifdef INET > case AF_INET: > + { > + struct ip *ip; > + > /* icmp_error() expects host byte ordering */ > ip = mtod(m0, struct ip *); > NTOHS(ip->ip_len); > NTOHS(ip->ip_off); > - PF_UNLOCK(); > - icmp_error(m0, type, code, 0, 0); > - PF_LOCK(); > + > + pfse->pfse_type = PFSE_ICMP; > break; > + } > #endif /* INET */ > #ifdef INET6 > case AF_INET6: > - PF_UNLOCK(); > - icmp6_error(m0, type, code, 0); > - PF_LOCK(); > + pfse->pfse_type = PFSE_ICMP6; > break; > #endif /* INET6 */ > } > + pfse->pfse_m = m0; > + pfse->pfse_icmp_type = type; > + pfse->pfse_icmp_code = code; > + pf_send(pfse); > } > > /* > > Modified: projects/pf/head/sys/contrib/pf/net/pf_ioctl.c > ============================================================================== > --- projects/pf/head/sys/contrib/pf/net/pf_ioctl.c Thu Apr 12 14:49:25 2012 (r234186) > +++ projects/pf/head/sys/contrib/pf/net/pf_ioctl.c Thu Apr 12 15:56:04 2012 (r234187) > @@ -52,10 +52,12 @@ __FBSDID("$FreeBSD$"); > > #include <sys/param.h> > #include <sys/systm.h> > +#include <sys/bus.h> > #include <sys/mbuf.h> > #include <sys/endian.h> > #include <sys/filio.h> > #include <sys/fcntl.h> > +#include <sys/interrupt.h> > #include <sys/socket.h> > #include <sys/socketvar.h> > #include <sys/kernel.h> > @@ -248,6 +250,7 @@ static int > pfattach(void) > { > u_int32_t *my_timeout = V_pf_default_rule.timeout; > + int error; > > pf_initialize(); > pfr_initialize(); > @@ -300,9 +303,14 @@ pfattach(void) > /* XXX do our best to avoid a conflict */ > V_pf_status.hostid = arc4random(); > > - if (kproc_create(pf_purge_thread, curvnet, NULL, 0, 0, "pfpurge")) > + if ((error = kproc_create(pf_purge_thread, curvnet, NULL, 0, 0, > + "pf purge")) != 0) > + /* XXXGL: leaked all above. */ > + return (error); > + if ((error = swi_add(NULL, "pf send", pf_intr, curvnet, SWI_NET, > + INTR_MPSAFE, &V_pf_swi_cookie)) != 0) > /* XXXGL: leaked all above. */ > - return (ENXIO); > + return (error); > > m_addr_chg_pf_p = pf_pkt_addr_changed; > > @@ -3779,6 +3787,7 @@ pf_unload(void) > V_pf_status.running = 0; > PF_UNLOCK(); > m_addr_chg_pf_p = NULL; > + swi_remove(V_pf_swi_cookie); > error = dehook_pf(); > if (error) { > /* > > Modified: projects/pf/head/sys/contrib/pf/net/pfvar.h > ============================================================================== > --- projects/pf/head/sys/contrib/pf/net/pfvar.h Thu Apr 12 14:49:25 2012 (r234186) > +++ projects/pf/head/sys/contrib/pf/net/pfvar.h Thu Apr 12 15:56:04 2012 (r234187) > @@ -1715,6 +1715,9 @@ VNET_DECLARE(u_long, pf_hashmask); > > #define PF_IDHASH(s) (be64toh((s)->id) % (V_pf_hashmask + 1)) > > +VNET_DECLARE(void *, pf_swi_cookie); > +#define V_pf_swi_cookie VNET(pf_swi_cookie) > + > TAILQ_HEAD(pf_poolqueue, pf_pool); > VNET_DECLARE(struct pf_poolqueue, pf_pools[2]); > #define V_pf_pools VNET(pf_pools) > @@ -1774,6 +1777,7 @@ VNET_DECLARE(uma_zone_t, pfi_addr_z); > #define V_pfi_addr_z VNET(pfi_addr_z) > > extern void pf_purge_thread(void *); > +extern void pf_intr(void *); > extern void pf_purge_expired_src_nodes(void); > > extern void pf_unlink_state(struct pf_state *, u_int); -- Ermalhelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPBZQG3ntZC2SuPPZBG%2BLpvixFv=rumAk0J9bSxS36YucmP4ng>