[libvirt] [libvirt PATCHv4 1/2] add DHCP snooping

[Stefan Berger]

On 10/25/2011 08:50 AM, Stefan Berger wrote:
> On 10/24/2011 07:12 PM, David L Stevens wrote:
>> This patch adds DHCP Snooping support to libvirt.
>>
>> Signed-off-by: David L Stevens<dlstevens at us.ibm.com>
>> ---
>>   examples/xml/nwfilter/no-ip-spoofing.xml |    5 +
>>   src/Makefile.am                          |    2 +
>>   src/nwfilter/nwfilter_dhcpsnoop.c        |  705 
>> ++++++++++++++++++++++++++++++
>>   src/nwfilter/nwfilter_dhcpsnoop.h        |   38 ++
>>   src/nwfilter/nwfilter_driver.c           |    5 +
>>   src/nwfilter/nwfilter_gentech_driver.c   |   51 ++-
>>   6 files changed, 793 insertions(+), 13 deletions(-)
>>   create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.c
>>   create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.h
>>
>> diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml 
>> b/examples/xml/nwfilter/no-ip-spoofing.xml
>> index b8c94c8..e5969a0 100644
>> --- a/examples/xml/nwfilter/no-ip-spoofing.xml
>> +++ b/examples/xml/nwfilter/no-ip-spoofing.xml
>> @@ -4,4 +4,9 @@
>> <rule action='drop' direction='out'>
>> <ip match='no' srcipaddr='$IP' />
>> </rule>
>> +<!-- allow DHCP requests -->
>> +<rule action='return' direction='out'>
>> +<ip match='yes' srcipaddr='0.0.0.0' protocol='udp' srcportstart='68'
>> +                  srcportend='68' />
>> +</rule>
>> </filter>
> Is this change necessary? Is it needed for the first instantiation of 
> the rules or is it needed to support the case when all IP addresses 
> have expired? I am asking because below I saw you calling
The order of the two rules as given here is wrong. As long as the IP 
variable is not set, it will not generate the filter and once the 
variable is set it will never get to the 2nd rule.
>>       virNWFilterHashTablePtr missing_vars = 
>> virNWFilterHashTableCreate(0);
>>       if (!missing_vars) {
>> @@ -655,22 +660,40 @@ virNWFilterInstantiate(virConnectPtr conn,
>>       if (rc)
>>           goto err_exit;
>>
>> +    learning = virHashLookup(vars->hashTable, "ip_learning");
>> +
>
> Can you add this as documentation to the nwfilter documentation page?
Also, in case learning is NULL, you may need to set it to 'any' (for 
backwards-compatibility) to avoid a NULL pointer crash later on. In the 
other file there's an fclose(fp) that causes a crash (fp=NULL) in case 
the lease file was not available -> move the fclose() two lines up.

Once the 2nd patch is applied and libvirt started, I see this here

2011-09-26 14:26:06.206: 21084: warning : 
networkAddGeneralIptablesRules:1270 : May need to update iptables 
package & kernel to support CHECKSUM rule.
*** glibc detected *** /usr/sbin/libvirtd: malloc(): memory corruption: 
0x00007fc5b433e010 ***

This does not occur with only the first patch applied and only occurs if 
a lease file was found. So something in the leasefile support code is 
corrupting memory.

Also it probably should re-create the filters in case libvirt is 
restarted. Somehow you should be able to use the lease files to get the 
IP address to rebuild the filters. The 'IP learning' subsytem just did 
the static IP address detection in this case but with DHCP snooping one 
shouldn't have to cycle the interfaces to cause the DHCP protocol to run.

    Stefan