From owner-freebsd-jail@FreeBSD.ORG Sun Jun 2 12:52:12 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 6D67FCC9 for ; Sun, 2 Jun 2013 12:52:12 +0000 (UTC) (envelope-from gofj-freebsd-jail@m.gmane.org) Received: from plane.gmane.org (plane.gmane.org [80.91.229.3]) by mx1.freebsd.org (Postfix) with ESMTP id 2C8B61185 for ; Sun, 2 Jun 2013 12:52:11 +0000 (UTC) Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1Uj7la-0000KW-G3 for freebsd-jail@freebsd.org; Sun, 02 Jun 2013 14:52:10 +0200 Received: from 105-236-93-112.access.mtnbusiness.co.za ([105.236.93.112]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sun, 02 Jun 2013 14:52:10 +0200 Received: from lists by 105-236-93-112.access.mtnbusiness.co.za with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sun, 02 Jun 2013 14:52:10 +0200 X-Injected-Via-Gmane: http://gmane.org/ To: freebsd-jail@freebsd.org From: Mogamat Abrahams Subject: Re: Cant reach Jailed services from internet. Date: Sun, 2 Jun 2013 12:51:54 +0000 (UTC) Lines: 58 Message-ID: References: <20130528145629.X55451@sola.nimnet.asn.au> <20130528080719.GA11195@eik.bme.hu> <51A5F743.7080307@a1poweruser.com> <51A758FF.4080402@a1poweruser.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: sea.gmane.org User-Agent: Loom/3.14 (http://gmane.org/) X-Loom-IP: 105.236.93.112 (Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.94 Safari/537.36) X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Jun 2013 12:52:12 -0000 Joe writes: > Your 67.205.xx.xx ip address looks like a dynamic ip address that you > use dhcp to automatically obtain all the network configuration > information needed by your host. Static ip addresses don't work that > way. You have to manually configure the static network. If I remember > correctly, for a block of 3 assignable ip addresses you need a block of > 5 from your provider. The first and last ip address are used to config > the network. This address was provided and I manually configured the nic. > You never said if you have a firewall on your host. The firewall rules > maybe dropping unsolicited inbound traffic for those 174 prefixed ip > addresses. Try putting a pass all log from that NIC rule or just a log > all rule or turn off the firewall all together and see what happens. > Verify your NAT is not trying to NAT unsolicited inbound traffic for > those 174 prefixed ip addresses. I had no firewall installed on the machine as we were still setting up and usually only add firewalling last. Here is something interesting though, since compiling a custom kernel and including: device<><------>pf device<><------>pflog nooptions<----->sctp options><------>VIMAGE device ><------>epair device ><------>if_bridge options><------>NULLFS #firewall options MROUTING # Multicast routing options IPFIREWALL #firewall options IPFIREWALL_VERBOSE #enable logging to syslogd(8) options IPFIREWALL_VERBOSE_LIMIT=100 #limit verbosity options IPFIREWALL_DEFAULT_TO_ACCEPT #allow everything by default options IPFIREWALL_FORWARD #packet destination changes options ACCEPT_FILTER_DATA options ACCEPT_FILTER_DNS options ACCEPT_FILTER_HTTP options ZERO_COPY_SOCKETS My JAILS now both receive and respond to traffic! This was the only change i remember making. Just running on firewall_type="OPEN" and have not even defined any other rules. So the problem seems solved, however still not sure what fixed it....!! Is NAT a requirement for Jail networking where the default gateway is not on the same subnet as the Jail? From owner-freebsd-jail@FreeBSD.ORG Sun Jun 2 17:34:53 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 9080C5E8 for ; Sun, 2 Jun 2013 17:34:53 +0000 (UTC) (envelope-from fbsd8@a1poweruser.com) Received: from mail-03.name-services.com (mail-03.name-services.com [69.64.155.195]) by mx1.freebsd.org (Postfix) with ESMTP id 7BDF8110B for ; Sun, 2 Jun 2013 17:34:53 +0000 (UTC) Received: from [10.0.10.1] ([173.88.196.224]) by mail-03.name-services.com with Microsoft SMTPSVC(6.0.3790.4675); Sun, 2 Jun 2013 10:34:48 -0700 Message-ID: <51AB8236.7030706@a1poweruser.com> Date: Sun, 02 Jun 2013 13:34:46 -0400 From: Fbsd8 User-Agent: Thunderbird 2.0.0.17 (Windows/20080914) MIME-Version: 1.0 To: Mogamat Abrahams Subject: Re: Cant reach Jailed services from internet. References: <20130528145629.X55451@sola.nimnet.asn.au> <20130528080719.GA11195@eik.bme.hu> <51A5F743.7080307@a1poweruser.com> <51A758FF.4080402@a1poweruser.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 02 Jun 2013 17:34:48.0874 (UTC) FILETIME=[7B11C8A0:01CE5FB7] X-Sender: fbsd8@a1poweruser.com X-Authenticated-Sender: fbsd8@a1poweruser.com X-EchoSenderHash: [fbsd8]-[a1poweruser*com] Cc: freebsd-jail@freebsd.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Jun 2013 17:34:53 -0000 Mogamat Abrahams wrote: > Joe writes: > > >> Your 67.205.xx.xx ip address looks like a dynamic ip address that you >> use dhcp to automatically obtain all the network configuration >> information needed by your host. Static ip addresses don't work that >> way. You have to manually configure the static network. If I remember >> correctly, for a block of 3 assignable ip addresses you need a block of >> 5 from your provider. The first and last ip address are used to config >> the network. > This address was provided and I manually configured the nic. > >> You never said if you have a firewall on your host. The firewall rules >> maybe dropping unsolicited inbound traffic for those 174 prefixed ip >> addresses. Try putting a pass all log from that NIC rule or just a log >> all rule or turn off the firewall all together and see what happens. >> Verify your NAT is not trying to NAT unsolicited inbound traffic for >> those 174 prefixed ip addresses. > > I had no firewall installed on the machine as we were still setting up and > usually only add firewalling last. Here is something interesting though, > since compiling a custom kernel and > including: > > device<><------>pf > device<><------>pflog > nooptions<----->sctp > options><------>VIMAGE > device ><------>epair > device ><------>if_bridge > options><------>NULLFS > > #firewall > > options MROUTING # Multicast routing > > options IPFIREWALL #firewall > options IPFIREWALL_VERBOSE #enable logging to syslogd(8) > options IPFIREWALL_VERBOSE_LIMIT=100 #limit verbosity > options IPFIREWALL_DEFAULT_TO_ACCEPT #allow everything by default > options IPFIREWALL_FORWARD #packet destination changes > > options ACCEPT_FILTER_DATA > options ACCEPT_FILTER_DNS > options ACCEPT_FILTER_HTTP > options ZERO_COPY_SOCKETS > > > My JAILS now both receive and respond to traffic! This was the only change i > remember making. > Just running on firewall_type="OPEN" and have not even defined any other > rules. > > So the problem seems solved, however still not sure what fixed it....!! Is > NAT a requirement > for Jail networking where the default gateway is not on the same subnet as > the Jail? > > Mogamat Abrahams It's customary to post your solution as the last post in this thread. Since you have so many kernel options included it would be nice to know which one really made the difference. BY process of limitation nooptions sctp problem was fixed in 8.1-release device pf your not using this firewall device pflog options ACCEPT_FILTER_DATA These 4 have never been talked about options ACCEPT_FILTER_DNS before in vnet context. Not likely options ACCEPT_FILTER_HTTP to have any bearing on your problem. options ZERO_COPY_SOCKETS Since your problem was happening with both if_bridge/epair and netgraph vnet networks seems unlikely that device epair device if_bridge compiled into the kernel has any bearing on your problem. My money is on options MROUTING # Multicast routing May I suggest you remove the above kernel options and recompile with modules. If it works then you know what kernel option is the solution to a vnet jail receiving inbound traffic. Then post the if_bridge/epair commands you used to create your vnet/vimage inbound and outbound network. Your solution post provides an answer (solution) for people who search the list email archives who have the same problem. Doing this is how you repay the people who help you on this list. From owner-freebsd-jail@FreeBSD.ORG Mon Jun 3 11:06:47 2013 Return-Path: Delivered-To: freebsd-jail@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 8D6875D2 for ; Mon, 3 Jun 2013 11:06:47 +0000 (UTC) (envelope-from owner-bugmaster@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) by mx1.freebsd.org (Postfix) with ESMTP id 7F43913B6 for ; Mon, 3 Jun 2013 11:06:47 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.7/8.14.7) with ESMTP id r53B6l50015064 for ; Mon, 3 Jun 2013 11:06:47 GMT (envelope-from owner-bugmaster@FreeBSD.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.7/8.14.7/Submit) id r53B6l0h015062 for freebsd-jail@FreeBSD.org; Mon, 3 Jun 2013 11:06:47 GMT (envelope-from owner-bugmaster@FreeBSD.org) Date: Mon, 3 Jun 2013 11:06:47 GMT Message-Id: <201306031106.r53B6l0h015062@freefall.freebsd.org> X-Authentication-Warning: freefall.freebsd.org: gnats set sender to owner-bugmaster@FreeBSD.org using -f From: FreeBSD bugmaster To: freebsd-jail@FreeBSD.org Subject: Current problem reports assigned to freebsd-jail@FreeBSD.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jun 2013 11:06:47 -0000 Note: to view an individual PR, use: http://www.freebsd.org/cgi/query-pr.cgi?pr=(number). The following is a listing of current problems submitted by FreeBSD users. These represent problem reports covering all versions including experimental development code and obsolete releases. S Tracker Resp. Description -------------------------------------------------------------------------------- o bin/178302 jail jail(8): unknown parameter: ip6.addr when kernel compi o kern/176112 jail [jail] [panic] kernel panic when starting jails o kern/176092 jail [jail] [panic] Starting a jail on my releng/9.1 kernel o kern/174902 jail [jail] jail should provide validator for jail names o kern/174436 jail [jail] Jails with numbers as names don't work o bin/173469 jail [jail] regression: security.jail.sysvipc_allowed=1 no o kern/169751 jail [jail] reading routing information does not work in ja o bin/167911 jail new jail(8) problem with removal, ifconfg -alias and k o kern/159918 jail [jail] inter-jail communication failure o kern/156111 jail [jail] procstat -b not supported in jail o misc/155765 jail [patch] `buildworld' does not honors WITHOUT_JAIL o conf/154246 jail [jail] [patch] Bad symlink created if devfs mount poin s conf/142972 jail [jail] [patch] Support JAILv2 and vnet in rc.d/jail o conf/141317 jail [patch] uncorrect jail stop in /etc/rc.d/jail o kern/133265 jail [jail] is there a solution how to run nfs client in ja o kern/119842 jail [smbfs] [jail] "Bad address" with smbfs inside a jail o bin/99566 jail [jail] [patch] fstat(1) according to specified jid 17 problems total. From owner-freebsd-jail@FreeBSD.ORG Mon Jun 3 11:58:46 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 3E003C5A; Mon, 3 Jun 2013 11:58:46 +0000 (UTC) (envelope-from nvass@gmx.com) Received: from mout.gmx.net (mout.gmx.net [212.227.17.22]) by mx1.freebsd.org (Postfix) with ESMTP id B9E771914; Mon, 3 Jun 2013 11:58:45 +0000 (UTC) Received: from [192.168.44.85] ([217.115.146.100]) by mail.gmx.com (mrgmx103) with ESMTPSA (Nemesis) id 0LyVcA-1UMu8v0b1a-015qGr; Mon, 03 Jun 2013 13:58:39 +0200 Message-ID: <51AC84EE.6020009@gmx.com> Date: Mon, 03 Jun 2013 13:58:38 +0200 From: Nikos Vassiliadis User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: "freebsd-pf@freebsd.org" , freebsd-virtualization@freebsd.org Subject: pf + vimage patch Content-Type: multipart/mixed; boundary="------------020601030809060207010704" X-Provags-ID: V03:K0:PhTuxnXAoOCGPr7+w0Z7PuTn7Di6qw8RkjxEUG/7G5y/bX7Tzas P0cSTnXl1i71RD8baKQ+ANJOXhTjY3jP4uXdkL01Nc8N6Nkr3ftQ+rcbb7qjneFXQWvzdT5 /X6XcNWEbNJlkrsGfXwtV1wYebS87s7R4pRvyJNHl3FBLJj3XNgcCI00NGAOooDhDacuTvc OIgrOd70iLNDhhnKy9nBg== X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jun 2013 11:58:46 -0000 This is a multi-part message in MIME format. --------------020601030809060207010704 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi, Please review this patch. It fixes some problems with pf and vimage. For the time being only pf works. ALTQ, pflog, pfsync are not changed nor tested but as time permits, I'll work on them. Basic packet filtering functionality per VNET should be ok. Thanks in advance for reviewing, Nikos --------------020601030809060207010704 Content-Type: text/x-patch; name="pf.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="pf.diff" Index: sys/net/pfvar.h =================================================================== --- sys/net/pfvar.h (revision 251294) +++ sys/net/pfvar.h (working copy) @@ -901,7 +901,6 @@ struct pf_ruleset *, struct pf_pdesc *, int); extern pflog_packet_t *pflog_packet_ptr; -#define V_pf_end_threads VNET(pf_end_threads) #endif /* _KERNEL */ #define PFSYNC_FLAG_SRCNODE 0x04 Index: sys/netpfil/pf/pf.c =================================================================== --- sys/netpfil/pf/pf.c (revision 251294) +++ sys/netpfil/pf/pf.c (working copy) @@ -300,8 +300,6 @@ int in4_cksum(struct mbuf *m, u_int8_t nxt, int off, int len); -VNET_DECLARE(int, pf_end_threads); - VNET_DEFINE(struct pf_limit, pf_limits[PF_LIMIT_MAX]); #define PACKET_LOOPED(pd) ((pd)->pf_mtag && \ @@ -359,15 +357,13 @@ SYSCTL_NODE(_net, OID_AUTO, pf, CTLFLAG_RW, 0, "pf(4)"); -VNET_DEFINE(u_long, pf_hashsize); -#define V_pf_hashsize VNET(pf_hashsize) -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_RDTUN, - &VNET_NAME(pf_hashsize), 0, "Size of pf(4) states hashtable"); +u_long pf_hashsize; +SYSCTL_UINT(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_RDTUN, + &pf_hashsize, 0, "Size of pf(4) states hashtable"); -VNET_DEFINE(u_long, pf_srchashsize); -#define V_pf_srchashsize VNET(pf_srchashsize) -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, - &VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes hashtable"); +u_long pf_srchashsize; +SYSCTL_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, + &pf_srchashsize, 0, "Size of pf(4) source nodes hashtable"); VNET_DEFINE(void *, pf_swi_cookie); @@ -698,12 +694,12 @@ struct pf_srchash *sh; u_int i; - TUNABLE_ULONG_FETCH("net.pf.states_hashsize", &V_pf_hashsize); - if (V_pf_hashsize == 0 || !powerof2(V_pf_hashsize)) - V_pf_hashsize = PF_HASHSIZ; - TUNABLE_ULONG_FETCH("net.pf.source_nodes_hashsize", &V_pf_srchashsize); - if (V_pf_srchashsize == 0 || !powerof2(V_pf_srchashsize)) - V_pf_srchashsize = PF_HASHSIZ / 4; + TUNABLE_ULONG_FETCH("net.pf.states_hashsize", &pf_hashsize); + if (pf_hashsize == 0 || !powerof2(pf_hashsize)) + pf_hashsize = PF_HASHSIZ; + TUNABLE_ULONG_FETCH("net.pf.source_nodes_hashsize", &pf_srchashsize); + if (pf_srchashsize == 0 || !powerof2(pf_srchashsize)) + pf_srchashsize = PF_HASHSIZ / 4; V_pf_hashseed = arc4random(); @@ -717,11 +713,11 @@ V_pf_state_key_z = uma_zcreate("pf state keys", sizeof(struct pf_state_key), pf_state_key_ctor, NULL, NULL, NULL, UMA_ALIGN_PTR, 0); - V_pf_keyhash = malloc(V_pf_hashsize * sizeof(struct pf_keyhash), + V_pf_keyhash = malloc(pf_hashsize * sizeof(struct pf_keyhash), M_PFHASH, M_WAITOK | M_ZERO); - V_pf_idhash = malloc(V_pf_hashsize * sizeof(struct pf_idhash), + V_pf_idhash = malloc(pf_hashsize * sizeof(struct pf_idhash), M_PFHASH, M_WAITOK | M_ZERO); - V_pf_hashmask = V_pf_hashsize - 1; + V_pf_hashmask = pf_hashsize - 1; for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask; i++, kh++, ih++) { mtx_init(&kh->lock, "pf_keyhash", NULL, MTX_DEF); @@ -735,9 +731,9 @@ 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_warning(V_pf_sources_z, "PF source nodes limit reached"); - V_pf_srchash = malloc(V_pf_srchashsize * sizeof(struct pf_srchash), + V_pf_srchash = malloc(pf_srchashsize * sizeof(struct pf_srchash), M_PFHASH, M_WAITOK|M_ZERO); - V_pf_srchashmask = V_pf_srchashsize - 1; + V_pf_srchashmask = pf_srchashsize - 1; for (i = 0, sh = V_pf_srchash; i <= V_pf_srchashmask; i++, sh++) mtx_init(&sh->lock, "pf_srchash", NULL, MTX_DEF); @@ -757,13 +753,17 @@ STAILQ_INIT(&V_pf_sendqueue); SLIST_INIT(&V_pf_overloadqueue); TASK_INIT(&V_pf_overloadtask, 0, pf_overload_task, &V_pf_overloadqueue); - mtx_init(&pf_sendqueue_mtx, "pf send queue", NULL, MTX_DEF); - mtx_init(&pf_overloadqueue_mtx, "pf overload/flush queue", NULL, - MTX_DEF); + if (IS_DEFAULT_VNET(curvnet)) { + mtx_init(&pf_sendqueue_mtx, "pf send queue", NULL, MTX_DEF); + mtx_init(&pf_overloadqueue_mtx, "pf overload/flush queue", NULL, + MTX_DEF); + } /* Unlinked, but may be referenced rules. */ TAILQ_INIT(&V_pf_unlinked_rules); - mtx_init(&pf_unlnkdrules_mtx, "pf unlinked rules", NULL, MTX_DEF); + if (IS_DEFAULT_VNET(curvnet)) + mtx_init(&pf_unlnkdrules_mtx, "pf unlinked rules", NULL, MTX_DEF); + } void @@ -1309,68 +1309,35 @@ pf_purge_thread(void *v) { u_int idx = 0; + VNET_ITERATOR_DECL(vnet_iter); - CURVNET_SET((struct vnet *)v); - for (;;) { - PF_RULES_RLOCK(); - rw_sleep(pf_purge_thread, &pf_rules_lock, 0, "pftm", hz / 10); + tsleep(pf_purge_thread, PWAIT, "pftm", hz / 10); + VNET_LIST_RLOCK(); + VNET_FOREACH(vnet_iter) { + CURVNET_SET(vnet_iter); - if (V_pf_end_threads) { - /* - * To cleanse up all kifs and rules we need - * two runs: first one clears reference flags, - * then pf_purge_expired_states() doesn't - * raise them, and then second run frees. - */ - PF_RULES_RUNLOCK(); - pf_purge_unlinked_rules(); - pfi_kif_purge(); - - /* - * Now purge everything. - */ - pf_purge_expired_states(0, V_pf_hashmask); - pf_purge_expired_fragments(); - pf_purge_expired_src_nodes(); - - /* - * Now all kifs & rules should be unreferenced, - * thus should be successfully freed. - */ - pf_purge_unlinked_rules(); - pfi_kif_purge(); - - /* - * Announce success and exit. - */ - PF_RULES_RLOCK(); - V_pf_end_threads++; - PF_RULES_RUNLOCK(); - wakeup(pf_purge_thread); - kproc_exit(0); - } - PF_RULES_RUNLOCK(); - /* Process 1/interval fraction of the state table every run. */ idx = pf_purge_expired_states(idx, V_pf_hashmask / - (V_pf_default_rule.timeout[PFTM_INTERVAL] * 10)); + (V_pf_default_rule.timeout[PFTM_INTERVAL] * 10)); /* Purge other expired types every PFTM_INTERVAL seconds. */ if (idx == 0) { - /* - * Order is important: - * - states and src nodes reference rules - * - states and rules reference kifs - */ - pf_purge_expired_fragments(); - pf_purge_expired_src_nodes(); - pf_purge_unlinked_rules(); - pfi_kif_purge(); + /* + * Order is important: + * - states and src nodes reference rules + * - states and rules reference kifs + */ + pf_purge_expired_fragments(); + pf_purge_expired_src_nodes(); + pf_purge_unlinked_rules(); + pfi_kif_purge(); } + CURVNET_RESTORE(); + } + VNET_LIST_RUNLOCK(); } /* not reached */ - CURVNET_RESTORE(); } u_int32_t Index: sys/netpfil/pf/pf_if.c =================================================================== --- sys/netpfil/pf/pf_if.c (revision 251294) +++ sys/netpfil/pf/pf_if.c (working copy) @@ -110,7 +110,8 @@ V_pfi_buffer = malloc(V_pfi_buffer_max * sizeof(*V_pfi_buffer), PFI_MTYPE, M_WAITOK); - mtx_init(&pfi_unlnkdkifs_mtx, "pf unlinked interfaces", NULL, MTX_DEF); + if (IS_DEFAULT_VNET(curvnet)) + mtx_init(&pfi_unlnkdkifs_mtx, "pf unlinked interfaces", NULL, MTX_DEF); kif = malloc(sizeof(*kif), PFI_MTYPE, M_WAITOK); PF_RULES_WLOCK(); @@ -124,18 +125,20 @@ pfi_attach_ifnet(ifp); IFNET_RUNLOCK(); - pfi_attach_cookie = EVENTHANDLER_REGISTER(ifnet_arrival_event, - pfi_attach_ifnet_event, NULL, EVENTHANDLER_PRI_ANY); - pfi_detach_cookie = EVENTHANDLER_REGISTER(ifnet_departure_event, - pfi_detach_ifnet_event, NULL, EVENTHANDLER_PRI_ANY); - pfi_attach_group_cookie = EVENTHANDLER_REGISTER(group_attach_event, - pfi_attach_group_event, curvnet, EVENTHANDLER_PRI_ANY); - pfi_change_group_cookie = EVENTHANDLER_REGISTER(group_change_event, - pfi_change_group_event, curvnet, EVENTHANDLER_PRI_ANY); - pfi_detach_group_cookie = EVENTHANDLER_REGISTER(group_detach_event, - pfi_detach_group_event, curvnet, EVENTHANDLER_PRI_ANY); - pfi_ifaddr_event_cookie = EVENTHANDLER_REGISTER(ifaddr_event, - pfi_ifaddr_event, NULL, EVENTHANDLER_PRI_ANY); + if (IS_DEFAULT_VNET(curvnet)) { + pfi_attach_cookie = EVENTHANDLER_REGISTER(ifnet_arrival_event, + pfi_attach_ifnet_event, NULL, EVENTHANDLER_PRI_ANY); + pfi_detach_cookie = EVENTHANDLER_REGISTER(ifnet_departure_event, + pfi_detach_ifnet_event, NULL, EVENTHANDLER_PRI_ANY); + pfi_attach_group_cookie = EVENTHANDLER_REGISTER(group_attach_event, + pfi_attach_group_event, curvnet, EVENTHANDLER_PRI_ANY); + pfi_change_group_cookie = EVENTHANDLER_REGISTER(group_change_event, + pfi_change_group_event, curvnet, EVENTHANDLER_PRI_ANY); + pfi_detach_group_cookie = EVENTHANDLER_REGISTER(group_detach_event, + pfi_detach_group_event, curvnet, EVENTHANDLER_PRI_ANY); + pfi_ifaddr_event_cookie = EVENTHANDLER_REGISTER(ifaddr_event, + pfi_ifaddr_event, NULL, EVENTHANDLER_PRI_ANY); + } } void Index: sys/netpfil/pf/pf_ioctl.c =================================================================== --- sys/netpfil/pf/pf_ioctl.c (revision 251294) +++ sys/netpfil/pf/pf_ioctl.c (working copy) @@ -183,7 +183,6 @@ static volatile VNET_DEFINE(int, pf_pfil_hooked); #define V_pf_pfil_hooked VNET(pf_pfil_hooked) -VNET_DEFINE(int, pf_end_threads); struct rwlock pf_rules_lock; @@ -254,10 +253,13 @@ /* XXX do our best to avoid a conflict */ V_pf_status.hostid = arc4random(); - if ((error = kproc_create(pf_purge_thread, curvnet, NULL, 0, 0, - "pf purge")) != 0) - /* XXXGL: leaked all above. */ - return (error); + if (IS_DEFAULT_VNET(curvnet)) { + 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. */ @@ -3631,24 +3633,22 @@ static int pf_load(void) { - int error; - VNET_ITERATOR_DECL(vnet_iter); + rw_init(&pf_rules_lock, "pf rulesets"); + pf_dev = make_dev(&pf_cdevsw, 0, 0, 0, 0600, PF_NAME); - VNET_LIST_RLOCK(); - VNET_FOREACH(vnet_iter) { - CURVNET_SET(vnet_iter); - V_pf_pfil_hooked = 0; - V_pf_end_threads = 0; - TAILQ_INIT(&V_pf_tags); - TAILQ_INIT(&V_pf_qids); - CURVNET_RESTORE(); - } - VNET_LIST_RUNLOCK(); + return (0); +} - rw_init(&pf_rules_lock, "pf rulesets"); +static int +vnet_pf_init(void) +{ + int error; - pf_dev = make_dev(&pf_cdevsw, 0, 0, 0, 0600, PF_NAME); + V_pf_pfil_hooked = 0; + TAILQ_INIT(&V_pf_tags); + TAILQ_INIT(&V_pf_qids); + if ((error = pfattach()) != 0) return (error); @@ -3676,11 +3676,6 @@ } PF_RULES_WLOCK(); shutdown_pf(); - V_pf_end_threads = 1; - while (V_pf_end_threads < 2) { - wakeup_one(pf_purge_thread); - rw_sleep(pf_purge_thread, &pf_rules_lock, 0, "pftmo", 0); - } pf_normalize_cleanup(); pfi_cleanup(); pfr_cleanup(); @@ -3727,3 +3722,6 @@ DECLARE_MODULE(pf, pf_mod, SI_SUB_PSEUDO, SI_ORDER_FIRST); MODULE_VERSION(pf, PF_MODVER); + +VNET_SYSINIT(vnet_pf_init, SI_SUB_PROTO_IFATTACHDOMAIN, SI_ORDER_ANY - 255, + vnet_pf_init, NULL); Index: sys/netpfil/pf/pf_norm.c =================================================================== --- sys/netpfil/pf/pf_norm.c (revision 251294) +++ sys/netpfil/pf/pf_norm.c (working copy) @@ -163,7 +163,8 @@ uma_zone_set_max(V_pf_frent_z, PFFRAG_FRENT_HIWAT); uma_zone_set_warning(V_pf_frent_z, "PF frag entries limit reached"); - mtx_init(&pf_frag_mtx, "pf fragments", NULL, MTX_DEF); + if (IS_DEFAULT_VNET(curvnet)) + mtx_init(&pf_frag_mtx, "pf fragments", NULL, MTX_DEF); TAILQ_INIT(&V_pf_fragqueue); TAILQ_INIT(&V_pf_cachequeue); Index: sys/netpfil/pf/pf_table.c =================================================================== --- sys/netpfil/pf/pf_table.c (revision 251294) +++ sys/netpfil/pf/pf_table.c (working copy) @@ -183,10 +183,14 @@ static RB_PROTOTYPE(pfr_ktablehead, pfr_ktable, pfrkt_tree, pfr_ktable_compare); static RB_GENERATE(pfr_ktablehead, pfr_ktable, pfrkt_tree, pfr_ktable_compare); -struct pfr_ktablehead pfr_ktables; +VNET_DEFINE(struct pfr_ktablehead, pfr_ktables); +#define V_pfr_ktables VNET(pfr_ktables) + struct pfr_table pfr_nulltable; -int pfr_ktable_cnt; +VNET_DEFINE(int, pfr_ktable_cnt); +#define V_pfr_ktable_cnt VNET(pfr_ktable_cnt) + void pfr_initialize(void) { @@ -1082,7 +1086,7 @@ return (ENOENT); SLIST_INIT(&workq); - RB_FOREACH(p, pfr_ktablehead, &pfr_ktables) { + RB_FOREACH(p, pfr_ktablehead, &V_pfr_ktables) { if (pfr_skip_table(filter, p, flags)) continue; if (!strcmp(p->pfrkt_anchor, PF_RESERVED_ANCHOR)) @@ -1117,7 +1121,7 @@ flags & PFR_FLAG_USERIOCTL)) senderr(EINVAL); key.pfrkt_flags |= PFR_TFLAG_ACTIVE; - p = RB_FIND(pfr_ktablehead, &pfr_ktables, &key); + p = RB_FIND(pfr_ktablehead, &V_pfr_ktables, &key); if (p == NULL) { p = pfr_create_ktable(&key.pfrkt_t, tzero, 1); if (p == NULL) @@ -1133,7 +1137,7 @@ /* find or create root table */ bzero(key.pfrkt_anchor, sizeof(key.pfrkt_anchor)); - r = RB_FIND(pfr_ktablehead, &pfr_ktables, &key); + r = RB_FIND(pfr_ktablehead, &V_pfr_ktables, &key); if (r != NULL) { p->pfrkt_root = r; goto _skip; @@ -1189,7 +1193,7 @@ if (pfr_validate_table(&key.pfrkt_t, 0, flags & PFR_FLAG_USERIOCTL)) return (EINVAL); - p = RB_FIND(pfr_ktablehead, &pfr_ktables, &key); + p = RB_FIND(pfr_ktablehead, &V_pfr_ktables, &key); if (p != NULL && (p->pfrkt_flags & PFR_TFLAG_ACTIVE)) { SLIST_FOREACH(q, &workq, pfrkt_workq) if (!pfr_ktable_compare(p, q)) @@ -1228,7 +1232,7 @@ *size = n; return (0); } - RB_FOREACH(p, pfr_ktablehead, &pfr_ktables) { + RB_FOREACH(p, pfr_ktablehead, &V_pfr_ktables) { if (pfr_skip_table(filter, p, flags)) continue; if (n-- <= 0) @@ -1263,7 +1267,7 @@ return (0); } SLIST_INIT(&workq); - RB_FOREACH(p, pfr_ktablehead, &pfr_ktables) { + RB_FOREACH(p, pfr_ktablehead, &V_pfr_ktables) { if (pfr_skip_table(filter, p, flags)) continue; if (n-- <= 0) @@ -1295,7 +1299,7 @@ bcopy(tbl + i, &key.pfrkt_t, sizeof(key.pfrkt_t)); if (pfr_validate_table(&key.pfrkt_t, 0, 0)) return (EINVAL); - p = RB_FIND(pfr_ktablehead, &pfr_ktables, &key); + p = RB_FIND(pfr_ktablehead, &V_pfr_ktables, &key); if (p != NULL) { SLIST_INSERT_HEAD(&workq, p, pfrkt_workq); xzero++; @@ -1327,7 +1331,7 @@ if (pfr_validate_table(&key.pfrkt_t, 0, flags & PFR_FLAG_USERIOCTL)) return (EINVAL); - p = RB_FIND(pfr_ktablehead, &pfr_ktables, &key); + p = RB_FIND(pfr_ktablehead, &V_pfr_ktables, &key); if (p != NULL && (p->pfrkt_flags & PFR_TFLAG_ACTIVE)) { p->pfrkt_nflags = (p->pfrkt_flags | setflag) & ~clrflag; @@ -1369,7 +1373,7 @@ if (rs == NULL) return (ENOMEM); SLIST_INIT(&workq); - RB_FOREACH(p, pfr_ktablehead, &pfr_ktables) { + RB_FOREACH(p, pfr_ktablehead, &V_pfr_ktables) { if (!(p->pfrkt_flags & PFR_TFLAG_INACTIVE) || pfr_skip_table(trs, p, 0)) continue; @@ -1414,7 +1418,7 @@ return (EBUSY); tbl->pfrt_flags |= PFR_TFLAG_INACTIVE; SLIST_INIT(&tableq); - kt = RB_FIND(pfr_ktablehead, &pfr_ktables, (struct pfr_ktable *)tbl); + kt = RB_FIND(pfr_ktablehead, &V_pfr_ktables, (struct pfr_ktable *)tbl); if (kt == NULL) { kt = pfr_create_ktable(tbl, 0, 1); if (kt == NULL) @@ -1427,7 +1431,7 @@ /* find or create root table */ bzero(&key, sizeof(key)); strlcpy(key.pfrkt_name, tbl->pfrt_name, sizeof(key.pfrkt_name)); - rt = RB_FIND(pfr_ktablehead, &pfr_ktables, &key); + rt = RB_FIND(pfr_ktablehead, &V_pfr_ktables, &key); if (rt != NULL) { kt->pfrkt_root = rt; goto _skip; @@ -1504,7 +1508,7 @@ if (rs == NULL || !rs->topen || ticket != rs->tticket) return (0); SLIST_INIT(&workq); - RB_FOREACH(p, pfr_ktablehead, &pfr_ktables) { + RB_FOREACH(p, pfr_ktablehead, &V_pfr_ktables) { if (!(p->pfrkt_flags & PFR_TFLAG_INACTIVE) || pfr_skip_table(trs, p, 0)) continue; @@ -1540,7 +1544,7 @@ return (EBUSY); SLIST_INIT(&workq); - RB_FOREACH(p, pfr_ktablehead, &pfr_ktables) { + RB_FOREACH(p, pfr_ktablehead, &V_pfr_ktables) { if (!(p->pfrkt_flags & PFR_TFLAG_INACTIVE) || pfr_skip_table(trs, p, 0)) continue; @@ -1686,7 +1690,7 @@ PF_RULES_ASSERT(); if (flags & PFR_FLAG_ALLRSETS) - return (pfr_ktable_cnt); + return (V_pfr_ktable_cnt); if (filter->pfrt_anchor[0]) { rs = pf_find_ruleset(filter->pfrt_anchor); return ((rs != NULL) ? rs->tables : -1); @@ -1719,8 +1723,8 @@ PF_RULES_WASSERT(); - RB_INSERT(pfr_ktablehead, &pfr_ktables, kt); - pfr_ktable_cnt++; + RB_INSERT(pfr_ktablehead, &V_pfr_ktables, kt); + V_pfr_ktable_cnt++; if (kt->pfrkt_root != NULL) if (!kt->pfrkt_root->pfrkt_refcnt[PFR_REFCNT_ANCHOR]++) pfr_setflags_ktable(kt->pfrkt_root, @@ -1751,14 +1755,14 @@ if (!(newf & PFR_TFLAG_ACTIVE)) newf &= ~PFR_TFLAG_USRMASK; if (!(newf & PFR_TFLAG_SETMASK)) { - RB_REMOVE(pfr_ktablehead, &pfr_ktables, kt); + RB_REMOVE(pfr_ktablehead, &V_pfr_ktables, kt); if (kt->pfrkt_root != NULL) if (!--kt->pfrkt_root->pfrkt_refcnt[PFR_REFCNT_ANCHOR]) pfr_setflags_ktable(kt->pfrkt_root, kt->pfrkt_root->pfrkt_flags & ~PFR_TFLAG_REFDANCHOR); pfr_destroy_ktable(kt, 1); - pfr_ktable_cnt--; + V_pfr_ktable_cnt--; return; } if (!(newf & PFR_TFLAG_ACTIVE) && kt->pfrkt_cnt) { @@ -1883,7 +1887,7 @@ pfr_lookup_table(struct pfr_table *tbl) { /* struct pfr_ktable start like a struct pfr_table */ - return (RB_FIND(pfr_ktablehead, &pfr_ktables, + return (RB_FIND(pfr_ktablehead, &V_pfr_ktables, (struct pfr_ktable *)tbl)); } --------------020601030809060207010704-- From owner-freebsd-jail@FreeBSD.ORG Wed Jun 5 08:52:27 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 2F7C3C15; Wed, 5 Jun 2013 08:52:27 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from mail-bk0-x236.google.com (mail-bk0-x236.google.com [IPv6:2a00:1450:4008:c01::236]) by mx1.freebsd.org (Postfix) with ESMTP id 3774D1C64; Wed, 5 Jun 2013 08:52:26 +0000 (UTC) Received: by mail-bk0-f54.google.com with SMTP id it19so693182bkc.13 for ; Wed, 05 Jun 2013 01:52:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=RhwcelCQpqAxDJ72aASwgKzRrHLb5Sq5Q1IaIK54REo=; b=muEOfzhTxJbfoJ3AGV4M0fafYBKSTDhyhUvVnL/Hn8wkkL2SpUTZRfBv3+5rvjjXSQ 4SLZK6QAUWZuXQMI+vU7nodCU+cQogf0NSwyIL/6tqth8acRNI0mklaoFE3n7O67CC/e j/E+qzH6ET/9GJbFrURL2dQJNWrn7kEFdXZPwQfQhzv/YBZRbGDp4iBiTrL+Nsyy5VNz 7bvj+Y3f91NJ5HZeO3u7AtOeG0Cpta9xW+SCLEVpACJC3ZA3kYsg7GmUgbz3EJgRV9N7 iIxd2iiFDBJloS7HtD46Cz4XUCbFvqFFmt+RbT16twSIUYZ2bFpkaMAC2ChAhHVEfd0p 0aSA== X-Received: by 10.204.62.137 with SMTP id x9mr9224354bkh.90.1370422344859; Wed, 05 Jun 2013 01:52:24 -0700 (PDT) Received: from localhost ([178.150.115.244]) by mx.google.com with ESMTPSA id jm15sm25045160bkb.13.2013.06.05.01.52.22 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 05 Jun 2013 01:52:23 -0700 (PDT) Sender: Mikolaj Golub Date: Wed, 5 Jun 2013 11:52:20 +0300 From: Mikolaj Golub To: Nikos Vassiliadis Subject: Re: pf + vimage patch Message-ID: <20130605085219.GA53217@gmail.com> References: <51AC84EE.6020009@gmx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51AC84EE.6020009@gmx.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-jail@freebsd.org, Gleb Smirnoff , freebsd-virtualization@freebsd.org, freebsd-pf@freebsd.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Jun 2013 08:52:27 -0000 Hi, On Mon, Jun 03, 2013 at 01:58:38PM +0200, Nikos Vassiliadis wrote: > Hi, > > Please review this patch. It fixes some problems with pf and vimage. > For the time being only pf works. ALTQ, pflog, pfsync are not changed > nor tested but as time permits, I'll work on them. Basic packet > filtering functionality per VNET should be ok. > Thank you for working on this. I'd really like to see pf and vimage integrated, as it looks like one of the main stoppers to have vimage in GENERIC. I hoped more knowledgeable people would comment on this though. Anyway, not being familiar with pf, here is a couple of things I would recommend to make your patch more attractive for potential reviewers: 1) It looks like the patch can be split on several parts. A log message to every change describing why it is needed and what problem solves would be very helpful. As a tool to maintain such changes I personally prefer git. 2) You use spaces for indentation, where tabs should be. This adds unnecessary noise and makes the patch less readable. Also someone will need to fix this when eventually committing to the tree. 3) When generating diff from svn, adding -x-pu options will make the diff more readable. Also see some questions/comments below. > Index: sys/net/pfvar.h > =================================================================== > --- sys/net/pfvar.h (revision 251294) > +++ sys/net/pfvar.h (working copy) > @@ -901,7 +901,6 @@ > struct pf_ruleset *, struct pf_pdesc *, int); > extern pflog_packet_t *pflog_packet_ptr; > > -#define V_pf_end_threads VNET(pf_end_threads) > #endif /* _KERNEL */ > > #define PFSYNC_FLAG_SRCNODE 0x04 > Index: sys/netpfil/pf/pf.c > =================================================================== > --- sys/netpfil/pf/pf.c (revision 251294) > +++ sys/netpfil/pf/pf.c (working copy) > @@ -300,8 +300,6 @@ > > int in4_cksum(struct mbuf *m, u_int8_t nxt, int off, int len); > > -VNET_DECLARE(int, pf_end_threads); > - > VNET_DEFINE(struct pf_limit, pf_limits[PF_LIMIT_MAX]); > > #define PACKET_LOOPED(pd) ((pd)->pf_mtag && \ > @@ -359,15 +357,13 @@ > > SYSCTL_NODE(_net, OID_AUTO, pf, CTLFLAG_RW, 0, "pf(4)"); > > -VNET_DEFINE(u_long, pf_hashsize); > -#define V_pf_hashsize VNET(pf_hashsize) > -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_RDTUN, > - &VNET_NAME(pf_hashsize), 0, "Size of pf(4) states hashtable"); > +u_long pf_hashsize; > +SYSCTL_UINT(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_RDTUN, > + &pf_hashsize, 0, "Size of pf(4) states hashtable"); > > -VNET_DEFINE(u_long, pf_srchashsize); > -#define V_pf_srchashsize VNET(pf_srchashsize) > -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, > - &VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes hashtable"); > +u_long pf_srchashsize; > +SYSCTL_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, > + &pf_srchashsize, 0, "Size of pf(4) source nodes hashtable"); > Why do you have to devirtualize these variables? Are per vnet hashtables sizes not useful or do they cause issues? > VNET_DEFINE(void *, pf_swi_cookie); > > @@ -698,12 +694,12 @@ > struct pf_srchash *sh; > u_int i; > > - TUNABLE_ULONG_FETCH("net.pf.states_hashsize", &V_pf_hashsize); > - if (V_pf_hashsize == 0 || !powerof2(V_pf_hashsize)) > - V_pf_hashsize = PF_HASHSIZ; > - TUNABLE_ULONG_FETCH("net.pf.source_nodes_hashsize", &V_pf_srchashsize); > - if (V_pf_srchashsize == 0 || !powerof2(V_pf_srchashsize)) > - V_pf_srchashsize = PF_HASHSIZ / 4; > + TUNABLE_ULONG_FETCH("net.pf.states_hashsize", &pf_hashsize); > + if (pf_hashsize == 0 || !powerof2(pf_hashsize)) > + pf_hashsize = PF_HASHSIZ; > + TUNABLE_ULONG_FETCH("net.pf.source_nodes_hashsize", &pf_srchashsize); > + if (pf_srchashsize == 0 || !powerof2(pf_srchashsize)) > + pf_srchashsize = PF_HASHSIZ / 4; > > V_pf_hashseed = arc4random(); > > @@ -717,11 +713,11 @@ > V_pf_state_key_z = uma_zcreate("pf state keys", > sizeof(struct pf_state_key), pf_state_key_ctor, NULL, NULL, NULL, > UMA_ALIGN_PTR, 0); > - V_pf_keyhash = malloc(V_pf_hashsize * sizeof(struct pf_keyhash), > + V_pf_keyhash = malloc(pf_hashsize * sizeof(struct pf_keyhash), > M_PFHASH, M_WAITOK | M_ZERO); > - V_pf_idhash = malloc(V_pf_hashsize * sizeof(struct pf_idhash), > + V_pf_idhash = malloc(pf_hashsize * sizeof(struct pf_idhash), > M_PFHASH, M_WAITOK | M_ZERO); > - V_pf_hashmask = V_pf_hashsize - 1; > + V_pf_hashmask = pf_hashsize - 1; > for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask; > i++, kh++, ih++) { > mtx_init(&kh->lock, "pf_keyhash", NULL, MTX_DEF); > @@ -735,9 +731,9 @@ > 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_warning(V_pf_sources_z, "PF source nodes limit reached"); > - V_pf_srchash = malloc(V_pf_srchashsize * sizeof(struct pf_srchash), > + V_pf_srchash = malloc(pf_srchashsize * sizeof(struct pf_srchash), > M_PFHASH, M_WAITOK|M_ZERO); > - V_pf_srchashmask = V_pf_srchashsize - 1; > + V_pf_srchashmask = pf_srchashsize - 1; > for (i = 0, sh = V_pf_srchash; i <= V_pf_srchashmask; i++, sh++) > mtx_init(&sh->lock, "pf_srchash", NULL, MTX_DEF); > > @@ -757,13 +753,17 @@ > STAILQ_INIT(&V_pf_sendqueue); > SLIST_INIT(&V_pf_overloadqueue); > TASK_INIT(&V_pf_overloadtask, 0, pf_overload_task, &V_pf_overloadqueue); > - mtx_init(&pf_sendqueue_mtx, "pf send queue", NULL, MTX_DEF); > - mtx_init(&pf_overloadqueue_mtx, "pf overload/flush queue", NULL, > - MTX_DEF); > + if (IS_DEFAULT_VNET(curvnet)) { > + mtx_init(&pf_sendqueue_mtx, "pf send queue", NULL, MTX_DEF); > + mtx_init(&pf_overloadqueue_mtx, "pf overload/flush queue", NULL, > + MTX_DEF); > + } > > /* Unlinked, but may be referenced rules. */ > TAILQ_INIT(&V_pf_unlinked_rules); > - mtx_init(&pf_unlnkdrules_mtx, "pf unlinked rules", NULL, MTX_DEF); > + if (IS_DEFAULT_VNET(curvnet)) > + mtx_init(&pf_unlnkdrules_mtx, "pf unlinked rules", NULL, MTX_DEF); > + > } "if (IS_DEFAULT_VNET(curvnet))" constructions look a little ugly for me. Isn't possible to split these initialization functions on two parts: one (not "virtualized") to run by pf_load() and the other by vnet_pf_init()? > > void > @@ -1309,68 +1309,35 @@ > pf_purge_thread(void *v) > { > u_int idx = 0; > + VNET_ITERATOR_DECL(vnet_iter); > > - CURVNET_SET((struct vnet *)v); > - > for (;;) { > - PF_RULES_RLOCK(); > - rw_sleep(pf_purge_thread, &pf_rules_lock, 0, "pftm", hz / 10); > + tsleep(pf_purge_thread, PWAIT, "pftm", hz / 10); > + VNET_LIST_RLOCK(); > + VNET_FOREACH(vnet_iter) { > + CURVNET_SET(vnet_iter); > > - if (V_pf_end_threads) { > - /* > - * To cleanse up all kifs and rules we need > - * two runs: first one clears reference flags, > - * then pf_purge_expired_states() doesn't > - * raise them, and then second run frees. > - */ > - PF_RULES_RUNLOCK(); > - pf_purge_unlinked_rules(); > - pfi_kif_purge(); > - > - /* > - * Now purge everything. > - */ > - pf_purge_expired_states(0, V_pf_hashmask); > - pf_purge_expired_fragments(); > - pf_purge_expired_src_nodes(); > - > - /* > - * Now all kifs & rules should be unreferenced, > - * thus should be successfully freed. > - */ > - pf_purge_unlinked_rules(); > - pfi_kif_purge(); > - > - /* > - * Announce success and exit. > - */ > - PF_RULES_RLOCK(); > - V_pf_end_threads++; > - PF_RULES_RUNLOCK(); > - wakeup(pf_purge_thread); > - kproc_exit(0); > - } Running only one instance of pf_purge_thread, which iterates over all vnets looks like a good idea to me, but do we really not need this clean up stuff for our only thread? Don't you have issues e.g. on pf module unload? > - PF_RULES_RUNLOCK(); > - > /* Process 1/interval fraction of the state table every run. */ > idx = pf_purge_expired_states(idx, V_pf_hashmask / > - (V_pf_default_rule.timeout[PFTM_INTERVAL] * 10)); > + (V_pf_default_rule.timeout[PFTM_INTERVAL] * 10)); > > /* Purge other expired types every PFTM_INTERVAL seconds. */ > if (idx == 0) { > - /* > - * Order is important: > - * - states and src nodes reference rules > - * - states and rules reference kifs > - */ > - pf_purge_expired_fragments(); > - pf_purge_expired_src_nodes(); > - pf_purge_unlinked_rules(); > - pfi_kif_purge(); > + /* > + * Order is important: > + * - states and src nodes reference rules > + * - states and rules reference kifs > + */ > + pf_purge_expired_fragments(); > + pf_purge_expired_src_nodes(); > + pf_purge_unlinked_rules(); > + pfi_kif_purge(); This is a good example of unnecessary whitespace noise. > } > + CURVNET_RESTORE(); > + } > + VNET_LIST_RUNLOCK(); > } > /* not reached */ > - CURVNET_RESTORE(); > } > -- Mikolaj Golub From owner-freebsd-jail@FreeBSD.ORG Thu Jun 6 10:36:09 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id AD93465C; Thu, 6 Jun 2013 10:36:09 +0000 (UTC) (envelope-from nvass@gmx.com) Received: from mout.gmx.net (mout.gmx.net [212.227.17.22]) by mx1.freebsd.org (Postfix) with ESMTP id 593C51816; Thu, 6 Jun 2013 10:36:09 +0000 (UTC) Received: from [172.31.0.42] ([85.216.121.245]) by mail.gmx.com (mrgmx102) with ESMTPSA (Nemesis) id 0Meutp-1V0Akm46gr-00OU2F; Thu, 06 Jun 2013 12:36:08 +0200 Message-ID: <51B065F5.4080209@gmx.com> Date: Thu, 06 Jun 2013 12:35:33 +0200 From: Nikos Vassiliadis User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Mikolaj Golub Subject: Re: pf + vimage patch References: <51AC84EE.6020009@gmx.com> <20130605085219.GA53217@gmail.com> In-Reply-To: <20130605085219.GA53217@gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:D++XOwS5R0lcMZ6V04weTtl/e1jFrUz3b2Q08P1soMzHg95wMGR SUvuUkPK5aAZIvOoScAfo8OQyRIXUFRvDrV8xf7fMp4w3V+OcjFx2q+Cp/02Y2H1ZUy5f49 dSUvretEMHOaLoN+BRt06i/RTffplPnrT3jWkOCgyOmdlc1jdSzcricwNb0qM0es0O4YdHb pLy1jCyEhqMsPMYPrxqRQ== Cc: "Bjoern A. Zeeb" , freebsd-jail@freebsd.org, Gleb Smirnoff , freebsd-virtualization@freebsd.org, freebsd-pf@freebsd.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jun 2013 10:36:09 -0000 Hi, Comments below. On 06/05/2013 10:52 AM, Mikolaj Golub wrote: > 1) It looks like the patch can be split on several parts. A log > message to every change describing why it is needed and what problem > solves would be very helpful. As a tool to maintain such changes I > personally prefer git. I'll try to break it in parts. It should be easy now to break it. Getting familiar with git will need some time so I'll handle it myself this time. > 2) You use spaces for indentation, where tabs should be. This adds > unnecessary noise and makes the patch less readable. Also someone > will need to fix this when eventually committing to the tree. Yes, I wrongly assumed that pf didn't follow style(9) strictly. Will fix that. > 3) When generating diff from svn, adding -x-pu options will make the > diff more readable. Yes indeed, thanks! > Also see some questions/comments below. > >> Index: sys/net/pfvar.h >> =================================================================== >> --- sys/net/pfvar.h (revision 251294) >> +++ sys/net/pfvar.h (working copy) >> @@ -901,7 +901,6 @@ >> struct pf_ruleset *, struct pf_pdesc *, int); >> extern pflog_packet_t *pflog_packet_ptr; >> >> -#define V_pf_end_threads VNET(pf_end_threads) >> #endif /* _KERNEL */ >> >> #define PFSYNC_FLAG_SRCNODE 0x04 >> Index: sys/netpfil/pf/pf.c >> =================================================================== >> --- sys/netpfil/pf/pf.c (revision 251294) >> +++ sys/netpfil/pf/pf.c (working copy) >> @@ -300,8 +300,6 @@ >> >> int in4_cksum(struct mbuf *m, u_int8_t nxt, int off, int len); >> >> -VNET_DECLARE(int, pf_end_threads); >> - >> VNET_DEFINE(struct pf_limit, pf_limits[PF_LIMIT_MAX]); >> >> #define PACKET_LOOPED(pd) ((pd)->pf_mtag && \ >> @@ -359,15 +357,13 @@ >> >> SYSCTL_NODE(_net, OID_AUTO, pf, CTLFLAG_RW, 0, "pf(4)"); >> >> -VNET_DEFINE(u_long, pf_hashsize); >> -#define V_pf_hashsize VNET(pf_hashsize) >> -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_RDTUN, >> - &VNET_NAME(pf_hashsize), 0, "Size of pf(4) states hashtable"); >> +u_long pf_hashsize; >> +SYSCTL_UINT(_net_pf, OID_AUTO, states_hashsize, CTLFLAG_RDTUN, >> + &pf_hashsize, 0, "Size of pf(4) states hashtable"); >> >> -VNET_DEFINE(u_long, pf_srchashsize); >> -#define V_pf_srchashsize VNET(pf_srchashsize) >> -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, >> - &VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes hashtable"); >> +u_long pf_srchashsize; >> +SYSCTL_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, >> + &pf_srchashsize, 0, "Size of pf(4) source nodes hashtable"); >> > > Why do you have to devirtualize these variables? Are per vnet > hashtables sizes not useful or do they cause issues? Per VNET variables are not useful for pf_hashsize and pf_srchashsize since these values are RO and cannot be modified at runtime. >> VNET_DEFINE(void *, pf_swi_cookie); >> >> @@ -698,12 +694,12 @@ >> struct pf_srchash *sh; >> u_int i; >> >> - TUNABLE_ULONG_FETCH("net.pf.states_hashsize", &V_pf_hashsize); >> - if (V_pf_hashsize == 0 || !powerof2(V_pf_hashsize)) >> - V_pf_hashsize = PF_HASHSIZ; >> - TUNABLE_ULONG_FETCH("net.pf.source_nodes_hashsize", &V_pf_srchashsize); >> - if (V_pf_srchashsize == 0 || !powerof2(V_pf_srchashsize)) >> - V_pf_srchashsize = PF_HASHSIZ / 4; >> + TUNABLE_ULONG_FETCH("net.pf.states_hashsize", &pf_hashsize); >> + if (pf_hashsize == 0 || !powerof2(pf_hashsize)) >> + pf_hashsize = PF_HASHSIZ; >> + TUNABLE_ULONG_FETCH("net.pf.source_nodes_hashsize", &pf_srchashsize); >> + if (pf_srchashsize == 0 || !powerof2(pf_srchashsize)) >> + pf_srchashsize = PF_HASHSIZ / 4; >> >> V_pf_hashseed = arc4random(); >> >> @@ -717,11 +713,11 @@ >> V_pf_state_key_z = uma_zcreate("pf state keys", >> sizeof(struct pf_state_key), pf_state_key_ctor, NULL, NULL, NULL, >> UMA_ALIGN_PTR, 0); >> - V_pf_keyhash = malloc(V_pf_hashsize * sizeof(struct pf_keyhash), >> + V_pf_keyhash = malloc(pf_hashsize * sizeof(struct pf_keyhash), >> M_PFHASH, M_WAITOK | M_ZERO); >> - V_pf_idhash = malloc(V_pf_hashsize * sizeof(struct pf_idhash), >> + V_pf_idhash = malloc(pf_hashsize * sizeof(struct pf_idhash), >> M_PFHASH, M_WAITOK | M_ZERO); >> - V_pf_hashmask = V_pf_hashsize - 1; >> + V_pf_hashmask = pf_hashsize - 1; >> for (i = 0, kh = V_pf_keyhash, ih = V_pf_idhash; i <= V_pf_hashmask; >> i++, kh++, ih++) { >> mtx_init(&kh->lock, "pf_keyhash", NULL, MTX_DEF); >> @@ -735,9 +731,9 @@ >> 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_warning(V_pf_sources_z, "PF source nodes limit reached"); >> - V_pf_srchash = malloc(V_pf_srchashsize * sizeof(struct pf_srchash), >> + V_pf_srchash = malloc(pf_srchashsize * sizeof(struct pf_srchash), >> M_PFHASH, M_WAITOK|M_ZERO); >> - V_pf_srchashmask = V_pf_srchashsize - 1; >> + V_pf_srchashmask = pf_srchashsize - 1; >> for (i = 0, sh = V_pf_srchash; i <= V_pf_srchashmask; i++, sh++) >> mtx_init(&sh->lock, "pf_srchash", NULL, MTX_DEF); >> >> @@ -757,13 +753,17 @@ >> STAILQ_INIT(&V_pf_sendqueue); >> SLIST_INIT(&V_pf_overloadqueue); >> TASK_INIT(&V_pf_overloadtask, 0, pf_overload_task, &V_pf_overloadqueue); >> - mtx_init(&pf_sendqueue_mtx, "pf send queue", NULL, MTX_DEF); >> - mtx_init(&pf_overloadqueue_mtx, "pf overload/flush queue", NULL, >> - MTX_DEF); >> + if (IS_DEFAULT_VNET(curvnet)) { >> + mtx_init(&pf_sendqueue_mtx, "pf send queue", NULL, MTX_DEF); >> + mtx_init(&pf_overloadqueue_mtx, "pf overload/flush queue", NULL, >> + MTX_DEF); >> + } >> >> /* Unlinked, but may be referenced rules. */ >> TAILQ_INIT(&V_pf_unlinked_rules); >> - mtx_init(&pf_unlnkdrules_mtx, "pf unlinked rules", NULL, MTX_DEF); >> + if (IS_DEFAULT_VNET(curvnet)) >> + mtx_init(&pf_unlnkdrules_mtx, "pf unlinked rules", NULL, MTX_DEF); >> + >> } > > "if (IS_DEFAULT_VNET(curvnet))" constructions look a little ugly for > me. Isn't possible to split these initialization functions on two > parts: one (not "virtualized") to run by pf_load() and the other by > vnet_pf_init()? It is indeed ugly. I was trying not to diverse to much from the original code. I will do it properly. >> >> void >> @@ -1309,68 +1309,35 @@ >> pf_purge_thread(void *v) >> { >> u_int idx = 0; >> + VNET_ITERATOR_DECL(vnet_iter); >> >> - CURVNET_SET((struct vnet *)v); >> - >> for (;;) { >> - PF_RULES_RLOCK(); >> - rw_sleep(pf_purge_thread, &pf_rules_lock, 0, "pftm", hz / 10); >> + tsleep(pf_purge_thread, PWAIT, "pftm", hz / 10); >> + VNET_LIST_RLOCK(); >> + VNET_FOREACH(vnet_iter) { >> + CURVNET_SET(vnet_iter); >> >> - if (V_pf_end_threads) { >> - /* >> - * To cleanse up all kifs and rules we need >> - * two runs: first one clears reference flags, >> - * then pf_purge_expired_states() doesn't >> - * raise them, and then second run frees. >> - */ >> - PF_RULES_RUNLOCK(); >> - pf_purge_unlinked_rules(); >> - pfi_kif_purge(); >> - >> - /* >> - * Now purge everything. >> - */ >> - pf_purge_expired_states(0, V_pf_hashmask); >> - pf_purge_expired_fragments(); >> - pf_purge_expired_src_nodes(); >> - >> - /* >> - * Now all kifs & rules should be unreferenced, >> - * thus should be successfully freed. >> - */ >> - pf_purge_unlinked_rules(); >> - pfi_kif_purge(); >> - >> - /* >> - * Announce success and exit. >> - */ >> - PF_RULES_RLOCK(); >> - V_pf_end_threads++; >> - PF_RULES_RUNLOCK(); >> - wakeup(pf_purge_thread); >> - kproc_exit(0); >> - } > > Running only one instance of pf_purge_thread, which iterates over all > vnets looks like a good idea to me, but do we really not need this > clean up stuff for our only thread? Don't you have issues e.g. on pf > module unload? module unload is broken:( Maybe it can be fixed at a (bit) later date? >> - PF_RULES_RUNLOCK(); >> - >> /* Process 1/interval fraction of the state table every run. */ >> idx = pf_purge_expired_states(idx, V_pf_hashmask / >> - (V_pf_default_rule.timeout[PFTM_INTERVAL] * 10)); >> + (V_pf_default_rule.timeout[PFTM_INTERVAL] * 10)); >> >> /* Purge other expired types every PFTM_INTERVAL seconds. */ >> if (idx == 0) { >> - /* >> - * Order is important: >> - * - states and src nodes reference rules >> - * - states and rules reference kifs >> - */ >> - pf_purge_expired_fragments(); >> - pf_purge_expired_src_nodes(); >> - pf_purge_unlinked_rules(); >> - pfi_kif_purge(); >> + /* >> + * Order is important: >> + * - states and src nodes reference rules >> + * - states and rules reference kifs >> + */ >> + pf_purge_expired_fragments(); >> + pf_purge_expired_src_nodes(); >> + pf_purge_unlinked_rules(); >> + pfi_kif_purge(); > > This is a good example of unnecessary whitespace noise. will fix these. >> } >> + CURVNET_RESTORE(); >> + } >> + VNET_LIST_RUNLOCK(); >> } >> /* not reached */ >> - CURVNET_RESTORE(); >> } >> > Thank you for your comments. I was informed by bz@ that there is a security issue with pf being exposed in a jail, so I'll start from there and then will continue with your comments. Nikos From owner-freebsd-jail@FreeBSD.ORG Thu Jun 6 12:24:17 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 14C80278; Thu, 6 Jun 2013 12:24:17 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from mail-bk0-x22c.google.com (mail-bk0-x22c.google.com [IPv6:2a00:1450:4008:c01::22c]) by mx1.freebsd.org (Postfix) with ESMTP id E611F1DE6; Thu, 6 Jun 2013 12:24:15 +0000 (UTC) Received: by mail-bk0-f44.google.com with SMTP id r7so1570276bkg.31 for ; Thu, 06 Jun 2013 05:24:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=eIZGnFUwokq+esK0tN2UdK4DZfTSBmtRPjlpxKUkiq0=; b=tiXxjS0oPRWCZjUWM041S7SLylnm9facT8wCnDGt5gNO54dPQWTrqTh2DDDBAxmfIV lnIU+Fu3R2b9PcADqugtJxT4U2GZfmDV0F0X0FOxPtKbkeinb4LzHteEtxf2gaEn8c04 MXNl2K29EcP6b9jr6ZpS9alpaoEvh8usMLP+f2ozljqhsYjV2J4XF5x2NTVdsIGC+YZD uN5yRngJQPgXiNcCZBsrhUX8jkZRJXpPvThK0u6yB3g72u9vRpjHnEnr8sOpOrBB4QpW NVWjc2teC25pPMeFeuCFHDsFHEJTfqOQDo8Nl53E/Mrw6UJxyrEkWkb1ewFD6qr0SkcA LpbA== X-Received: by 10.204.72.137 with SMTP id m9mr634498bkj.122.1370521454805; Thu, 06 Jun 2013 05:24:14 -0700 (PDT) Received: from localhost ([178.150.115.244]) by mx.google.com with ESMTPSA id fz10sm27756318bkc.9.2013.06.06.05.24.12 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 06 Jun 2013 05:24:13 -0700 (PDT) Sender: Mikolaj Golub Date: Thu, 6 Jun 2013 15:24:10 +0300 From: Mikolaj Golub To: Nikos Vassiliadis Subject: Re: pf + vimage patch Message-ID: <20130606122409.GA10459@gmail.com> References: <51AC84EE.6020009@gmx.com> <20130605085219.GA53217@gmail.com> <51B065F5.4080209@gmx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51B065F5.4080209@gmx.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: "Bjoern A. Zeeb" , freebsd-jail@freebsd.org, Gleb Smirnoff , freebsd-virtualization@freebsd.org, freebsd-pf@freebsd.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jun 2013 12:24:17 -0000 On Thu, Jun 06, 2013 at 12:35:33PM +0200, Nikos Vassiliadis wrote: > >> -VNET_DEFINE(u_long, pf_srchashsize); > >> -#define V_pf_srchashsize VNET(pf_srchashsize) > >> -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, > >> - &VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes hashtable"); > >> +u_long pf_srchashsize; > >> +SYSCTL_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, > >> + &pf_srchashsize, 0, "Size of pf(4) source nodes hashtable"); > >> > > > > Why do you have to devirtualize these variables? Are per vnet > > hashtables sizes not useful or do they cause issues? > > Per VNET variables are not useful for pf_hashsize and pf_srchashsize > since these values are RO and cannot be modified at runtime. Indeed. I missed RDTUN flag. > module unload is broken:( Maybe it can be fixed at a (bit) later date? I don't think Gleb will be happy with this. Some time ago he removed some vimage related stuff to prevent crashing on module unload (see r229849). Actually your patch looks like a partial revert of that commit. So I think you need to think about this issue from start. At least it should not crash non-vimage kernel and there should be understanding how to fix it for vimage kernel. Your approach with keeping only one purge thread might make it simpler. -- Mikolaj Golub From owner-freebsd-jail@FreeBSD.ORG Thu Jun 6 12:28:58 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 45AA838E; Thu, 6 Jun 2013 12:28:58 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.69.10]) by mx1.freebsd.org (Postfix) with ESMTP id 629C61E29; Thu, 6 Jun 2013 12:28:57 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.6/8.14.6) with ESMTP id r56CSuta046057; Thu, 6 Jun 2013 16:28:56 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.6/8.14.6/Submit) id r56CSuQL046056; Thu, 6 Jun 2013 16:28:56 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Thu, 6 Jun 2013 16:28:55 +0400 From: Gleb Smirnoff To: Mikolaj Golub Subject: Re: pf + vimage patch Message-ID: <20130606122855.GC14667@glebius.int.ru> References: <51AC84EE.6020009@gmx.com> <20130605085219.GA53217@gmail.com> <51B065F5.4080209@gmx.com> <20130606122409.GA10459@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline In-Reply-To: <20130606122409.GA10459@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-jail@freebsd.org, "Bjoern A. Zeeb" , freebsd-virtualization@freebsd.org, freebsd-pf@freebsd.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jun 2013 12:28:58 -0000 On Thu, Jun 06, 2013 at 03:24:10PM +0300, Mikolaj Golub wrote: M> > >> -VNET_DEFINE(u_long, pf_srchashsize); M> > >> -#define V_pf_srchashsize VNET(pf_srchashsize) M> > >> -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, M> > >> - &VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes hashtable"); M> > >> +u_long pf_srchashsize; M> > >> +SYSCTL_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, CTLFLAG_RDTUN, M> > >> + &pf_srchashsize, 0, "Size of pf(4) source nodes hashtable"); M> > >> M> > > M> > > Why do you have to devirtualize these variables? Are per vnet M> > > hashtables sizes not useful or do they cause issues? M> > M> > Per VNET variables are not useful for pf_hashsize and pf_srchashsize M> > since these values are RO and cannot be modified at runtime. M> M> Indeed. I missed RDTUN flag. M> M> > module unload is broken:( Maybe it can be fixed at a (bit) later date? M> M> I don't think Gleb will be happy with this. Some time ago he removed M> some vimage related stuff to prevent crashing on module unload (see M> r229849). Actually your patch looks like a partial revert of that M> commit. So I think you need to think about this issue from start. At M> least it should not crash non-vimage kernel and there should be M> understanding how to fix it for vimage kernel. Your approach with M> keeping only one purge thread might make it simpler. True. It is very much appreciated that you are working on vimage + pf, but breaking module unload isn't an option. When hacking on a part of kernel, having a possibility to avoid a reboot after each compile is very important. -- Totus tuus, Glebius. From owner-freebsd-jail@FreeBSD.ORG Thu Jun 6 13:30:44 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id D14C6BE3; Thu, 6 Jun 2013 13:30:44 +0000 (UTC) (envelope-from titi5187@gmail.com) Received: from mail-ie0-x22b.google.com (mail-ie0-x22b.google.com [IPv6:2607:f8b0:4001:c03::22b]) by mx1.freebsd.org (Postfix) with ESMTP id 765361198; Thu, 6 Jun 2013 13:30:44 +0000 (UTC) Received: by mail-ie0-f171.google.com with SMTP id s9so7105216iec.30 for ; Thu, 06 Jun 2013 06:30:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=GSpRu686kn7cS675oheBZtUm+9tcakEOjGvbZuLM0C0=; b=hpYiz3ocl2invRhNmAVQ450cz/9DVr0t64JMHDT9tZRPZ6wVMTgXj20YOFrx7SzFqN WhkSo6UkhAILyYyCpRGcIIxdzLTc/lqWCq7LmSUI7tKm0tmUlXgI1lR2cuNdQ9f14BKz v7TQ+UWXVAEDCU65W5J4OwhOGbqoQtxCiNV+xoM7HnY+kCzRi8Ydw9Vu4gKVdwDwTHFY lZf7om2JcUPu0R1PrWRjO/qpD+HyZ4wKtrbX+/uhMRJ/gKoKCjbi4P9/6cXf/BQPFJ5F wa0XPHCqmBAaRVkFkQnreAq54HwDmfPmdQ/k+16g/wufoIH44XoNlttwTLSrNM/LaqGz vwYw== MIME-Version: 1.0 X-Received: by 10.50.47.105 with SMTP id c9mr5808925ign.50.1370525444206; Thu, 06 Jun 2013 06:30:44 -0700 (PDT) Received: by 10.64.89.71 with HTTP; Thu, 6 Jun 2013 06:30:44 -0700 (PDT) In-Reply-To: <20130606122855.GC14667@glebius.int.ru> References: <51AC84EE.6020009@gmx.com> <20130605085219.GA53217@gmail.com> <51B065F5.4080209@gmx.com> <20130606122409.GA10459@gmail.com> <20130606122855.GC14667@glebius.int.ru> Date: Thu, 6 Jun 2013 15:30:44 +0200 Message-ID: Subject: Re: pf + vimage patch From: Thibault Noel To: Gleb Smirnoff Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.14 Cc: Mikolaj Golub , "Bjoern A. Zeeb" , freebsd-jail@freebsd.org, freebsd-virtualization@freebsd.org, freebsd-pf@freebsd.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jun 2013 13:30:44 -0000 Hi, I quite agree with Gleb Smirnoff on the possibility of avoiding a reboot after each compile. I don't now if it's a good moment to speak of this, but I observed that if pfsync is active in the kernel, it will use its state table even if nothing is set in the rc.conf. I think that if no parameters is written in the rc.conf (syncpeer, etc. ..) it will not be usefull to create lock to block the state table thus it will slow pf. What do you think ? 2013/6/6 Gleb Smirnoff > On Thu, Jun 06, 2013 at 03:24:10PM +0300, Mikolaj Golub wrote: > M> > >> -VNET_DEFINE(u_long, pf_srchashsize); > M> > >> -#define V_pf_srchashsize VNET(pf_srchashsize) > M> > >> -SYSCTL_VNET_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, > CTLFLAG_RDTUN, > M> > >> - &VNET_NAME(pf_srchashsize), 0, "Size of pf(4) source nodes > hashtable"); > M> > >> +u_long pf_srchashsize; > M> > >> +SYSCTL_UINT(_net_pf, OID_AUTO, source_nodes_hashsize, > CTLFLAG_RDTUN, > M> > >> + &pf_srchashsize, 0, "Size of pf(4) source nodes hashtable"); > M> > >> > M> > > > M> > > Why do you have to devirtualize these variables? Are per vnet > M> > > hashtables sizes not useful or do they cause issues? > M> > > M> > Per VNET variables are not useful for pf_hashsize and pf_srchashsize > M> > since these values are RO and cannot be modified at runtime. > M> > M> Indeed. I missed RDTUN flag. > M> > M> > module unload is broken:( Maybe it can be fixed at a (bit) later date? > M> > M> I don't think Gleb will be happy with this. Some time ago he removed > M> some vimage related stuff to prevent crashing on module unload (see > M> r229849). Actually your patch looks like a partial revert of that > M> commit. So I think you need to think about this issue from start. At > M> least it should not crash non-vimage kernel and there should be > M> understanding how to fix it for vimage kernel. Your approach with > M> keeping only one purge thread might make it simpler. > > True. It is very much appreciated that you are working on vimage + pf, > but breaking module unload isn't an option. > > When hacking on a part of kernel, having a possibility to avoid a reboot > after each compile is very important. > > -- > Totus tuus, Glebius. > _______________________________________________ > freebsd-virtualization@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization > To unsubscribe, send any mail to " > freebsd-virtualization-unsubscribe@freebsd.org" > From owner-freebsd-jail@FreeBSD.ORG Thu Jun 6 16:47:17 2013 Return-Path: Delivered-To: freebsd-jail@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 1A36CEEC; Thu, 6 Jun 2013 16:47:17 +0000 (UTC) (envelope-from nvass@gmx.com) Received: from mout.gmx.net (mout.gmx.net [212.227.17.20]) by mx1.freebsd.org (Postfix) with ESMTP id A7C0F1D9A; Thu, 6 Jun 2013 16:47:16 +0000 (UTC) Received: from [172.31.0.42] ([85.216.121.245]) by mail.gmx.com (mrgmx001) with ESMTPSA (Nemesis) id 0M6ilI-1UPe5B3EJQ-00wXDy; Thu, 06 Jun 2013 18:47:10 +0200 Message-ID: <51B0BD05.60102@gmx.com> Date: Thu, 06 Jun 2013 18:47:01 +0200 From: Nikos Vassiliadis User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Gleb Smirnoff Subject: Re: pf + vimage patch References: <51AC84EE.6020009@gmx.com> <20130605085219.GA53217@gmail.com> <51B065F5.4080209@gmx.com> <20130606122409.GA10459@gmail.com> <20130606122855.GC14667@glebius.int.ru> In-Reply-To: <20130606122855.GC14667@glebius.int.ru> Content-Type: text/plain; charset=KOI8-R; format=flowed Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:yLPARyYTKHkkRyMtMbsVYPXLJmfa+iUwAGfyu/gWFigeoljAbek VURb4qnVbCeCSutaraVeR5+cQsGY6dqwta+KHp2Lhpkc6jc+4Ns8Ov8aCm9ARWdepLnqwvq j/Z9aJIJ/x21zUjY5JgwGE1dA4TOgBZeCHVhGulJQH3SYRQNvT0LpyINMl/H+xpcFUBVc0s jHUNEiKxYm0gHLIptgxGQ== Cc: Mikolaj Golub , "Bjoern A. Zeeb" , freebsd-jail@freebsd.org, freebsd-virtualization@freebsd.org, freebsd-pf@freebsd.org X-BeenThere: freebsd-jail@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion about FreeBSD jail\(8\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jun 2013 16:47:17 -0000 On 06/06/2013 02:28 PM, Gleb Smirnoff wrote: > M> > module unload is broken:( Maybe it can be fixed at a (bit) later date? > M> > M> I don't think Gleb will be happy with this. Some time ago he removed > M> some vimage related stuff to prevent crashing on module unload (see > M> r229849). Actually your patch looks like a partial revert of that > M> commit. So I think you need to think about this issue from start. At > M> least it should not crash non-vimage kernel and there should be > M> understanding how to fix it for vimage kernel. Your approach with > M> keeping only one purge thread might make it simpler. Unloading should be simple in the non-vimage case as well - I think. > True. It is very much appreciated that you are working on vimage + pf, > but breaking module unload isn't an option. > > When hacking on a part of kernel, having a possibility to avoid a reboot > after each compile is very important. > Good point. Thank you both for your comments. Nikos