[libvirt] [PATCH V3] nwfilter: Add support for ipset

[Stefan Berger]

On 05/14/2012 06:20 PM, Eric Blake wrote:
> On 04/23/2012 06:00 AM, Stefan Berger wrote:
>> This patch adds support for the recent ipset iptables extension
>> to libvirt's nwfilter subsystem. Ipset allows to maintain 'sets'
>> of IP addresses, ports and other packet parameters and allows for
>> faster lookup (in the order of O(1) vs. O(n)) and rule evaluation
>> to achieve higher throughput than what can be achieved with
>> individual iptables rules.
>>
>> On the command line iptables supports ipset using
>>
>> iptables ... -m set --match-set<ipset name>  <flags>  -j ...
> How will this interact with firewalld in Fedora 17?  But adding things
> incrementally is okay by me, so I'll still review this.

Firewalld's firewall-cmd has a passthrough mode where the command line 
for iptables referencing an ipset would just be written like this:

firewall-cmd --direct --passthrough ipv4 ... -m set --match-set <ipset 
name> <flagse> -j ...

So this is not the real problem. Unfortunately using firewall-cmd makes 
everything much slower, i.e., the TCK testsuite runs probably at least 8 
times slower and starting a VM referencing a filter also takes 
considerably longer to start. So the solution may be to either find an 
intermediate format for creating the commands by the ebiptables driver 
so the 'scripts' can be converted to either bash or direct Dbus 
execution or, as I have done, to parse today's generated scripts and 
convert them to DBus commands. This works fine so far as long as one 
doesn't encounter comments, which we have assign to a variable 
(comment), and has its own challenges to parse. The speedup for the TCK 
testsuite was so far maybe 40% with scripts containing comments or the 
'other' embedded scripts containing loops left to running them using 
firewall-cmd. Any comments on this? The code so far parsing the script 
doesn't look 'too bad'...

>>
>> +# define VALID_IPSETNAME \
>> +  "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:-+ "
> Brave to allow a space; are we sure it will always be properly shell-quoted?

Yes, I payed attention. ;-)

>
>> @@ -346,6 +349,44 @@ _printDataType(virNWFilterVarCombIterPtr
>>           }
>>       break;
>>
>> +    case DATATYPE_IPSETNAME:
>> +        snprintf(buf, bufsize, "%s", item->u.ipset.setname);
> Are we sure that it is safe to ignore the return value of snprintf here?
>   And a format of "%s" is generally an indication that you should really
> be using virStrncpy instead.

Right, converting it.

>
>> @@ -927,6 +975,7 @@ iptablesHandleIpHdr(virBufferPtr buf,
>>       char ipaddr[INET6_ADDRSTRLEN],
>>            number[MAX(INT_BUFSIZE_BOUND(uint32_t),
>>                       INT_BUFSIZE_BOUND(int))];
>> +    char str[200];
> Why 200?  A comment, or even a composition of #define'd macros, instead
> of a magic number, would make me feel better.

Introducing MAX_IPSET_NAME_LENGTH.

>
>> @@ -1060,4 +1068,19 @@
>>         <param name="pattern">((SYN|ACK|URG|PSH|FIN|RST)(,(SYN|ACK|URG|PSH|FIN|RST))*|ALL|NONE)/((SYN|ACK|URG|PSH|FIN|RST)(,(SYN|ACK|URG|PSH|FIN|RST))*|ALL|NONE)</param>
>>       </data>
>>     </define>
>> +
>> +<define name='ipset-type'>
>> +<choice>
>> +<ref name="variable-name-type"/>
>> +<data type="string">
>> +<param name="pattern">[a-zA-Z0-9_\.:\-\+]{1,31}</param>
> Hmm, no space in this pattern; it doesn't match your VALID_IPSETNAME
> #define above.

Ooops, I will add it.

>
>> Index: libvirt-acl/docs/formatnwfilter.html.in
>> ===================================================================
>> --- libvirt-acl.orig/docs/formatnwfilter.html.in
>> +++ libvirt-acl/docs/formatnwfilter.html.in
>> @@ -528,6 +528,10 @@
>>        <li>IPV6_MASK: IPv6 mask in numbers format (FFFF:FFFF:FC00::) or CIDR mask (0-128)</li>
>>        <li>STRING: A string</li>
>>        <li>BOOLEAN: 'true', 'yes', '1' or 'false', 'no', '0'</li>
>> +<li>IPSETFLAGS: The source and destination flags of the ipset described
>> +         by up to 6 'src' or 'dst' elements selecting features from either
>> +         the source or destination part of the packet header; example:
>> +         src,src,dst</li>
> Do we have a mapping of _which_ elements are selected?  If I say
> 'src,dst', is it always the first two elements of a packet header (and
> which two elements are they), or does the choice of which two elements
> depend on other context?

It depends on the type of ipset that is referenced by a rule, i.e., the 
ipset hash:ip,port would allow to select IP address and port in the 
source or destination part of the packet independently, while the ipset 
hash:ip,port,ip would allow IP address to be selected in the source 
and/or destination part and the port in the sort or destination part.


>
>>       </ul>
>>       <p>
>>        <br/><br/>
>> @@ -1169,6 +1173,16 @@
>>            <td>STRING</td>
>>            <td>TCP-only: format of mask/flags with mask and flags each being a comma separated list of SYN,ACK,URG,PSH,FIN,RST or NONE or ALL</td>
>>          </tr>
>> +<tr>
>> +<td>ipset<span class="since">(Since 0.9.12)</span></td>
> 0.9.13, now.

Yes, will replace.

>> +<tr>
>> +<td>ipsetflags<span class="since">(Since 0.9.12)</span></td>
>> +<td>IPSETFLAGS</td>
>> +<td>flags for the IPSet; requires ipset attributed</td>
> I think you meant:
> s/attributed/attribute/
> (several instances)
>
> Overall, the patch seems reasonable (sorry for delaying the review until
> after 0.9.12).  But I do think it's worth a v4 to rebase to latest and
> fix the nits above.
>
Thanks for the review. I will post v4 shortly.

    Stefan