Stefan Berger <stefanb@linux.vnet.ibm.com> wrote on 10/26/2011 11:32:25 AM:
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.
I've removed this in the version I'm testing now. The DHCP-only rules cover this without multiple address support. In the original, I generated all chains and added and removed rules from those chains dynamically with address addition and removal (ie, it did not use virNWFilterInstantiateLFilterLate()). I didn't see any ordering issues in either version -- don't know what you mean there, but as you pointed out, this change is unnecessary in the stripped-down single-address version.
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.
I didn't try it with NULL -- if it is not set (ie, the variable is not present at all), it defaults to "any" already. If it's set to garbage, that's an error case which I handle, unless I I've broken it in later versions -- I'll retest this in v5, where I'm incorporating your other comments now.
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.
I didn't see any problems of this sort, with no pre-existing lease file or an existing one with various leases, active, deleted and expired. I'll look at it some more and see if I can figure out what you're seeing.
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.
Yes, that's the point of the lease file. I did only simple tests (but including libvirtd restart) for this version (v4) since I don't think anything I changed affected this from previous revisions, but I'll stress it a little more and see if I can reproduce what you're seeing. I did have some issues with restarting libvirtd (primarily VM's exiting when I restarted multiple times), but those were there before I applied any of these patches; no log messages when that happened. +-DLS