[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]