Date: Thu, 2 Feb 2012 10:12:30 +0100 From: =?ISO-8859-1?Q?Ermal_Lu=E7i?= <eri@freebsd.org> To: Luigi Rizzo <rizzo@iet.unipi.it> Cc: freebsd-net <freebsd-net@freebsd.org>, freebsd-hackers@freebsd.org Subject: Re: [PATCH] multiple instances of ipfw(4) Message-ID: <CAPBZQG0v_R06C7Pxh-XtyR8e08HSATuwVaGOmkb4QGy6BcROzA@mail.gmail.com> In-Reply-To: <20120131110204.GA95472@onelab2.iet.unipi.it> References: <CAPBZQG32iyzkec4PG%2Bqay9bKfd0GiffKyRBapLkATKvHr7cVww@mail.gmail.com> <20120131110204.GA95472@onelab2.iet.unipi.it>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jan 31, 2012 at 12:02 PM, Luigi Rizzo <rizzo@iet.unipi.it> wrote: > On Mon, Jan 30, 2012 at 01:01:13PM +0100, Ermal Lu?i wrote: >> Hello, >> >> from needs on pfSense a patch for allowing multiple intances of >> ipfw(4) in kernel to co-exist was developed. >> It can be found here >> https://raw.github.com/bsdperimeter/pfsense-tools/master/patches/RELENG_= 9_0/CP_multi_instance_ipfw.diff >> >> It is used in conjuction with this tool >> https://raw.github.com/bsdperimeter/pfsense-tools/master/pfPorts/ipfw_co= ntext/files/ipfw_context.c >> It allows creation of contextes/instances and assignment of specific >> interfaces to specific contexts/instances. >> >> Surely i know that this is not the best way to implement generically >> but it gets the job done for us as it is, read below. >> >> What i would like to know is if there is interest to see such >> functionality in FreeBSD? >> >> I am asking first to see if there is some consensus about this as a >> feature, needed or not! >> If interest is shown i will transform the patch to allow: >> - ipfw(8) to manage the contextes create/destroy >> - ipfw(8) to manage interface membership. Closing the race of two >> parallell clients modifying different contextes. >> >> There is another design choice to be made about storing the membership >> of interfaces into contexts/instances, but i do not see that as >> blocking. >> >> It is quite handy feature, which can be exploited even to scale on SMP >> machines by extending it to bind a specific instance(with its >> interaces) to a specific CPU/core?! >> >> Comments/Feedback expected, > > if i understand what the patch does, i think it makes sense to be > able to hook ipfw instances to specific interfaces/sets of interfaces, > as it permits the writing of more readable rulesets. Right now the > workaround is start the ruleset with skipto rules matching on > interface names, and then use some discipline in "reserving" a range > of rule numbers to each interface. > > Before making more changes to the code, > it would help if you could give a high level description of > > 1. what the change does and how specific cases are handled. E.g. > =A0 =A0 =A0 =A0With this change you can create multiple rulesets (context= s ?) > =A0 =A0 =A0 =A0and bind one or more interfaces to a context. In man simple words it can give you multiple rulesets in ipfw(4) together with ability to restrict the ruleset to specific interfaces. As it is written today it forces you to create the relation of interface(s) and rulesets explicitly. Though its debatable you would want the freedom of one interface to more than one ruleset. The target of this patch was strictly layer2 filtering with ipfw(4) and allowing easy managebility of multiple instances of an userland application using ipfw(4) at layer2 for filtering/forwarding/QoS purposes. > =A0 =A0 =A0 =A0- what happens with outgoing packets where the context > =A0 =A0 =A0 =A0 =A0to be picked up depend on the route in effect at the t= ime > =A0 =A0 =A0 =A0 =A0of the transmission ? For this i have not given much thought but makes sense only on satetful tracking of sessions in ipfw(4). See below for a solution implemented along with this patch. > =A0 =A0 =A0 =A0- what happens with encapsulated interfaces (vlan) ? Well by the very nature of this patch is that you have to explicitly assign an interface to the ruleset. This by definition of vlan/cloned interfaces in FreeBSD means you have to assign them to the ruleset explicitly too. In pfSense for example, since ipfw(4) is used exlusivly at layer2 it was necessary to introduce another flag and be very explicit on which interface is considered for filtering in ipfw(4). Also problems were faced on determining what was considered incoming and what outgoing when calling the ipfw hooks. This was changed to be explicit as well. Otherwise you have to play a tricky game of rules and what interface a particual pattern of traffic belongs and write respective rules about it, which is no fun at all from implementations and administration point of vie= w. > =A0 =A0 =A0 =A0- can you skipto across contexts (i guess not) ? > No its not there but it can be a possible addition. > 2. how intrusive are code changes ? The kernel patch you show > =A0 seems small, which makes sense as i believe all is needed is > =A0 to start from a specific chain instead of the default one when > =A0 an interface is bound to a context. A few comments: > =A0 =A0 =A0 =A0- if you use one of the if_ispare directly, instead of > =A0 =A0 =A0 =A0 =A0renaming it to if_context, this would make backporting= and > =A0 =A0 =A0 =A0 =A0testing easier. > =A0 =A0 =A0 =A0- I think the explosion of sockopt names is a bad thing. > =A0 =A0 =A0 =A0 =A0The IP_FW3 command was introduced exactly to have a si= ngle > =A0 =A0 =A0 =A0 =A0entry point to the firewall and avoid a ton of new nam= es > =A0 =A0 =A0 =A0 =A0in raw_ip.c and in.h > =A0 =A0 =A0 =A0- can you reduce the number of global ipfw-related variabl= es ? > =A0 =A0 =A0 =A0 =A0There used to be one (layer3_chain), now you have 3 of= them. > =A0 =A0 =A0 =A0 =A0You should delete layer3_chain and replace it with ano= ther > =A0 =A0 =A0 =A0 =A0single global (could be ip_fw_contexts) which contains= the > =A0 =A0 =A0 =A0 =A0whole firewall state. > =A0 =A0 =A0 =A0- how do you handle reinjects (e.g. from dummynet or diver= t) ? > =A0 =A0 =A0 =A0 =A0I don't remember if the metadata that stores where you > =A0 =A0 =A0 =A0 =A0reinject the packet also has a pointer to the root of = the > =A0 =A0 =A0 =A0 =A0chain. As described above a change to explicitly mark a packet incoming/outgoing interface in ipfw(4) metadata(mbuf tag) was needed. This is a patch, which is a bit verbose with other changed things in ipfw https://github.com/bsdperimeter/pfsense-tools/blob/master/patches/RELE= NG_9_0/CP_speedup.diff, has also the various bits i am talking for saving the interface and direction explicitly. > =A0 =A0 =A0 =A0- i don't completely follow the connection between ip_fw_c= hain, > =A0 =A0 =A0 =A0 =A0ip_fw_ctx_iflist, ip_fw_ctxmember, ip_fw_ctx, ip_fw_ct= x_list. > =A0 =A0 =A0 =A0 =A0The way i see it: > =A0 =A0 =A0 =A0 =A0- the ip_fw_chain could be embedded in the ip_fw_ctx, = as they > =A0 =A0 =A0 =A0 =A0 =A0map 1:1 yep that is probably a better place to put the chain and easys the integration with ipfw(8) command. > =A0 =A0 =A0 =A0 =A0- why do you need ip_fw_ctx_iflist and ip_fw_ctxmember= ? > =A0 =A0 =A0 =A0 =A0 =A0You should never need to determine context members= hip > =A0 =A0 =A0 =A0 =A0 =A0during packet processing, and for sockopt calls yo= u could > =A0 =A0 =A0 =A0 =A0 =A0as well iterate over the list of interfaces; ip_fw_ctx_iflist is used for being able to reassign an interface to its configured context. This is useful on dynamic interfaces that come and go example vlan(4). Its easier to handle this in kernel than in userland, where you have to detect the event and do the various reconfiguration things. ip_fw_ctxmember is just a simple way for the various new sockopt to pass the argument. It for sure can be recycled and implemented in a more clean way. I will try to make a patch with your recommandations and integrate the add/remove/delete of interfaces/contexts in ipfw and resend again. > > cheers > luigi > _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" --=20 Ermal
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPBZQG0v_R06C7Pxh-XtyR8e08HSATuwVaGOmkb4QGy6BcROzA>