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

[David Stevens]

Stefan Berger <stefanb at 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