Re: [libvirt] [libvirt PATCHv4 1/2] add DHCP snooping
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(a)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